Github Add default search path to `Target::search()` by xobs · Pull Request #838...
source link: https://github.com/rust-lang/rust/pull/83800
Go to the source link to view the article. You can view the picture content, updated content and better typesetting reading experience. If the link is broken, please click the button below to view the snapshot at that time.
The function Target::search() accepts a target triple and returns a Target struct defining the requested target.
There is a // FIXME 16351: add a sane default search path? comment that indicates it is desirable to include some sort of default. This was raised in #16351 which was closed without any resolution.
#31117 was proposed, however that has platform-specific logic that is unsuitable for systems without /etc/.
This patch implements the suggestion raised in #16351 (comment) where a target.json file may be placed in $(rustc --print sysroot)/lib/rustlib/<target-triple>/target.json. This allows shipping a toolchain distribution as a single file that gets extracted to the sysroot.
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @davidtwco (or someone else) soon.
If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.
Please see the contribution instructions for more information.
This comment has been hidden.
Implementation looks good to me. Going to reassign to another team member just to confirm that this is behaviour that we want to support.
Nominating for a brief look at the T-compiler meeting. It isn't entirely obvious to me if we are comfortable with enable further ability to transparently use adhoc target json specs given how unstable they are in practice.
OTOH fetching them from sysroot sounds like a pretty good middle-ground here.
An earlier version of this patch didn't look for $(rustc --print sysroot)/lib/rustlib/<target-triple>/target.json, but instead looked for $(rustc --print sysroot)/<target-triple>.json, but I changed that after reading #16351 (comment)
I could always change it back if desired.
This was briefly discussed during the compiler meeting this Thursday. and then further in this thread.
In summary the consensus seems to be that there's missing context motivating this change. Neither the TODO comment nor the linked issue really provides it. It would be great to gather some. I looked at couple other places. Here's what I found so far:
- The original RFC introducing the target mechanism used today specified
RUST_TARGET_PATHwould default to some directory. This has been since removed; - The PR removing this default from the RFC says such an addition would require much more scrutiny than what was afforded to the RFC in the original place;
- There isn't any discussion about the default or the environment variable in the RFC issue or meeting minutes discussing this RFC… or anywhere else really;
- So far 7 years have passed and there weren't really any major complaints.
While we probably won't make anybody write any RFCs for a change like this, this will at least involve an FCP vote on this PR, and having a motivation documented is a great way to set such a vote up for success.
@xobs do you have any motivating use-cases that made you submit this PR?
My motivation is the same as the one in #16351 (comment) -- to be able to distribute a custom toolchain for a new platform.
I have a new operating system I'm working on, and this includes a libstd. This is bundled as patches on top of this repository, with the goal of porting them forward with each new version of Rust. I don't want to force users to use nightly, because many crates detect if they're building on nightly and enable special features. Owing to the shifting nature of the nightly compiler, this means that as soon as we require nightly we bind ourselves to a specific version of the compiler. stable fixes this problem thanks to the amazing work of the Rust team.
The current approach is to distribute a .tar.gz with the root, and have users do export RUST_TARGET_PATH=$(rustc --print sysroot). This allows them to compile software with cargo build --target riscv32imac-unknown-xous-elf. Without setting this environment variable, a hash is appended to the compiled target name and I become unable to distribute the toolchain.
Alternatives to this approach are to patch rustc for every host to include the toolchain file, which requires me to maintain both libstd as well as rustc itself. I can also force users to use nightly, which would open us up to the version mismatches I described above.
My current approach is described at https://github.com/betrusted-io/rust#rust-stable-for-xous. I feel like I'm breaking quite a few rules here, and would welcome descriptions of alternate approaches I could take.
Do note that the target files are an implementation detail and don't confer any stability guarantees, regardless of whether it is a stable rustc or not.
Anyway, given the motivation above:
@rfcbot fcp merge
Team member @nagisa has proposed to merge this. The next step is review by the rest of the tagged team members:
No concerns currently listed.
Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!
See this document for info about what commands tagged team members can give me.
// Additionally look in the sysroot under `lib/rustlib/<triple>/target.json`
// as a fallback.
let p =
sysroot.join("lib").join("rustlib").join(&target_triple).join("target.json");
Mark-Simulacrum 26 days ago
Member
This should likely consider the possibility of lib being defined differently, perhaps by using the filesearch facilities in rustc_session (though that may be painful, as the two crates already have a dependency edge in the wrong direction, so some amount of code duplication or movement would be necessary).
It seems reasonable to support this, but I am wondering if there has been some background research on what other compilers do, particularly those that like rustc are always capable of cross-compiling. This solution seems reasonable, but it seems plausible that there may be pitfalls others have encountered that we have not addressed here. While I believe we are not committing to this being the behavior indefinitely (after all, target.json's contents are unstable), changing the search path will still be relatively disruptive, so doing some research first seems good.
Seems fine to keep a target spec file inside the directory corresponding to the target's "rustup component", ticked the box.
@rfcbot reviewed
The latest upstream changes (presumably #84501) made this pull request unmergeable. Please resolve the merge conflicts.
Copy link
rfcbot commented 13 days ago
This is now entering its final comment period, as per the review above.
Copy link
rfcbot commented 3 days ago
The final comment period, with a disposition to merge, as per the review above, is now complete.
As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.
The RFC will be merged soon.
Recommend
About Joyk
Aggregate valuable and interesting links.
Joyk means Joy of geeK