Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

default flexible targets path to /etc/rustc/ #31117

Closed
wants to merge 1 commit into from

Conversation

cardoe
Copy link
Contributor

@cardoe cardoe commented Jan 22, 2016

In RFC 131, the default RUST_TARGET_PATH if one is not set should be
/etc/rustc/ however the existing behavior does not match the RFC.

Signed-off-by: Doug Goldstein cardoe@cardoe.com

In RFC 131, the default RUST_TARGET_PATH if one is not set should be
/etc/rustc/ however the existing behavior does not match the RFC.

Signed-off-by: Doug Goldstein <cardoe@cardoe.com>
@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @arielb1 (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.

@cardoe
Copy link
Contributor Author

cardoe commented Jan 22, 2016

So I'll admit I'm not actually in love with this behavior but I'm just trying to get Rust to respect the RFC. IMHO, the path needs to respect the sysroot in some fashion but I'm not sure if a change like that needs another RFC or a patch to the RFC first? That would be related to https://internals.rust-lang.org/t/perfecting-rust-packaging-the-plan/2767

@brson
Copy link
Contributor

brson commented Jan 22, 2016

We shouldn't merge this. It's the only place the compiler cares directly about /etc and there's no comparable windows solution.

Edit: But thanks for finding the descrepancy and making the patch!

@brson
Copy link
Contributor

brson commented Jan 22, 2016

cc @cmr This is an interesting patch rooting out problems in the target specs RFC.

@brson
Copy link
Contributor

brson commented Jan 22, 2016

cc @rust-lang/tools

@brson
Copy link
Contributor

brson commented Jan 22, 2016

@cardoe Since you say this is packaging related, does that mean a distro needs to have some default custom target installed? Can you give more details about the scenario?

@cardoe
Copy link
Contributor Author

cardoe commented Jan 22, 2016

I agree this is not a good solution. But the point of this was to highlight that Rust is not respecting RFC 131. There needs to be an improved fix and I'm not sure how to go about that process to fix the RFC and then fix up Rust.

@alexcrichton
Copy link
Member

I feel the same as @brson in both regards:

  • First, thanks for the PR and pointing out the discrepancy!
  • Second, I agree that this is likely not what we'd like the compiler to do for now.

I'd personally prefer to update the RFC to elide the mention of /etc. In general custom target specs were implemented pretty far before 1.0 and we never really came back and truly scrutinized them for stabilization, and I highly doubt that this aspect would have survived such scrutinization.

@cardoe
Copy link
Contributor Author

cardoe commented Jan 22, 2016

@brson So in the case of Yocto, they install their "native" and "cross" builds into a sysroot relative to your build directory. So for example on x86_64, it will go into build/tmp/sysroots/x86_64-linux/ and the rustc binary will be build/tmp/sysroots/x86_64-linux/usr/bin/rustc. Yes their triple in this case is x86_64-linux I don't understand why. The actual target system is x86_64-poky-linux. I won't claim to understand why. Currently from the Yocto side I'm using the following patch https://github.com/starlab-io/meta-rust/blob/fido/recipes-devtools/rust/files/rust-1.3.0/0002-Target-add-default-target.json-path-libdir-rust-targ.patch and installing x86_64-poky-linux.json into the rustlib directory and setting that to the default search path with the patch.

cardoe added a commit to cardoe/rfcs that referenced this pull request Jan 22, 2016
The RFC specifies that if RUST_TARGET_PATH is unset then the default is
/etc/rustc but this won't work on all systems (e.g. Windows) and the
Rust compiler never actually implemented this behavior so remove it from
the RFC. closes rust-lang/rust#31117

Signed-off-by: Doug Goldstein <cardoe@cardoe.com>
@brson
Copy link
Contributor

brson commented Jan 22, 2016

@cardoe thanks for the info. Since you are patching the source to solve your problem, I think we should come up with some solution to this, since it seems like it's quite common for distros to want to customize target specs.

Maybe putting a default search path in rustlib just so distros can hide something there is the right way.

Relatedly, if distros could override builtin host target spec with a custom one in the sysroot that may be useful to let distros like Debian downgrade which cpu spec is being used on the default i686 target, which is one of the topics under discussion here.

@emberian
Copy link
Member

@brson I think it'd be a good idea to have a root target.json in rustlib/lib/$target

@emberian
Copy link
Member

instead of having a separate directory for target specs

@cardoe
Copy link
Contributor Author

cardoe commented Jan 22, 2016

@cmr That's roughly what I've been doing in the Yocto builds.

$ find build/tmp/sysroots -name x86_64-poky-linux.json
build/tmp/sysroots/x86_64-linux/usr/lib/x86_64-poky-linux/rustlib/x86_64-poky-linux.json

When you said $target did you envision that as the actual JSON file or a directory? I just ask because looking at my Homebrew installed Rust I have rustlib/x86_64-apple-darwin/lib and looking at my rustup.sh installed Rust I have rustlib/x86_64-unknown-linux-gnu/lib. So it seems like you're saying rustlib/x86_64-poky-linux/lib/x86_64-poky-linux.json or rustlib/i586-unknown-linux-gnu/lib/i586-unknown-linux-gnu/lib/i586-unknown-linux-gnu.json.

I'm willing to do the heavy lifting here by making the RFC and supplying the patches, I just want to make sure I do the right thing.

@cardoe
Copy link
Contributor Author

cardoe commented Jan 22, 2016

I have build/tmp/sysroots/x86_64-linux/usr/lib/x86_64-poky-linux/rustlib/x86_64-linux/lib/in my Yocto builds as well.

@emberian
Copy link
Member

Whoops, got it backwards - I envision rustlib/x86_64-unknown-linux-gnu/target.json and rustlib/x86_64-unknown-linux-gnu/lib/libcore.rlib etc.

@emberian
Copy link
Member

ie, the target name does not occur on the target spec filename, but in the directory structure.

@cardoe cardoe mentioned this pull request Apr 9, 2016
cardoe added a commit to cardoe/rust that referenced this pull request Apr 9, 2016
The path `/etc/rustc/` is not the default last entry in
RUST_TARGET_PATH. This was in RFC131 but was never implemented in rustc
so it was removed as part of rust-lang#31117 and rust-lang/rfcs#1473.

Signed-off-by: Doug Goldstein <cardoe@cardoe.com>
@cardoe cardoe deleted the rfc131-fix branch April 11, 2016 14:38
steveklabnik added a commit to steveklabnik/rust that referenced this pull request Apr 14, 2016
…hton

librustc_back: fix incorrect comment about RUST_TARGET_PATH

The path `/etc/rustc/` is not the default last entry in
RUST_TARGET_PATH. This was in RFC131 but was never implemented in rustc
so it was removed as part of rust-lang#31117 and rust-lang/rfcs#1473.

Signed-off-by: Doug Goldstein <cardoe@cardoe.com>
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request May 9, 2021
Add default search path to `Target::search()`

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 rust-lang#16351 which was closed without any resolution.

rust-lang#31117 was proposed, however that has platform-specific logic that is unsuitable for systems without `/etc/`.

This patch implements the suggestion raised in rust-lang#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.
bors added a commit to rust-lang-ci/rust that referenced this pull request May 9, 2021
Add default search path to `Target::search()`

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 rust-lang#16351 which was closed without any resolution.

rust-lang#31117 was proposed, however that has platform-specific logic that is unsuitable for systems without `/etc/`.

This patch implements the suggestion raised in rust-lang#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.
spikespaz pushed a commit to spikespaz/dotwalk-rs that referenced this pull request Aug 29, 2024
Add default search path to `Target::search()`

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 rust-lang/rust#16351 which was closed without any resolution.

rust-lang/rust#31117 was proposed, however that has platform-specific logic that is unsuitable for systems without `/etc/`.

This patch implements the suggestion raised in rust-lang/rust#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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants