Add default search path to `Target::search()` #83800

xobs · 2021-04-03T06:43:25Z

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.

This enables placing a `target.json` file into the rust sysroot under the target-specific directory. Signed-off-by: Sean Cross <[email protected]>

rust-highfive · 2021-04-03T06:43:28Z

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 fixes a build issue with formatting as part of rust-lang#83800. Signed-off-by: Sean Cross <[email protected]>

davidtwco

Implementation looks good to me. Going to reassign to another team member just to confirm that this is behaviour that we want to support.

davidtwco · 2021-04-05T21:01:50Z

r? @nagisa

nagisa · 2021-04-06T09:53:49Z

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.

xobs · 2021-04-07T12:48:34Z

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.

nagisa · 2021-04-10T17:27:59Z

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_PATH would 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?

xobs · 2021-04-11T07:33:05Z

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.

nagisa · 2021-04-11T18:13:01Z

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

rfcbot · 2021-04-11T18:13:02Z

Mark-Simulacrum · 2021-04-11T19:07:42Z

compiler/rustc_target/src/spec/mod.rs

+                // 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");


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).

This concern seems to have gone unaddressed, and to my knowledge distros are precisely the case we typically see nonstandard lib paths. I would at least like to see an issue filed.

Mark-Simulacrum · 2021-04-11T19:07:50Z

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.

petrochenkov · 2021-04-18T14:29:02Z

Seems fine to keep a target spec file inside the directory corresponding to the target's "rustup component", ticked the box.

nikomatsakis · 2021-04-19T19:29:29Z

@rfcbot reviewed

bors · 2021-04-24T07:58:27Z

☔ The latest upstream changes (presumably #84501) made this pull request unmergeable. Please resolve the merge conflicts.

rfcbot · 2021-04-24T09:03:34Z

🔔 This is now entering its final comment period, as per the review above. 🔔

Signed-off-by: Sean Cross <[email protected]>

rfcbot · 2021-05-04T09:04:30Z

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.

nagisa · 2021-05-09T18:47:41Z

@bors r+

bors · 2021-05-09T18:47:42Z

📌 Commit f9d390d has been approved by nagisa

nagisa · 2021-05-09T18:55:15Z

Note that all of the concerns raised above are still valid. I, however, don't believe that this functionality will be utilized widely enough that adjusting or removing it will be disruptive enough to prevent changes. Among other things sysroot layout is also an unstable detail, so changing this in the future seems fairly plausible.

bors · 2021-05-09T21:19:46Z

⌛ Testing commit f9d390d with merge 6d1405bc0d4d2018f18744d522c2749c52ed714a...

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.

Dylan-DPC-zz · 2021-05-09T21:38:39Z

@bors retry yield

rust-log-analyzer · 2021-05-09T21:42:19Z

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

bors · 2021-05-09T22:01:29Z

⌛ Testing commit f9d390d with merge c55c26c...

bors · 2021-05-10T00:42:26Z

☀️ Test successful - checks-actions
Approved by: nagisa
Pushing c55c26c to master...

With this the concerns expressed in rust-lang#83800 should be addressed.

…k-Simulacrum Adjust target search algorithm for rustlib path With this the concerns expressed in rust-lang#83800 should be addressed. r? `@Mark-Simulacrum`

Pkgsrc changes: * Bump bootstrap requirements to 1.53.0. * Adjust patches, adapt to upstream changes, adjust cargo checksums * If using an external llvm, require >= 10.0 Upsteream changes: Version 1.54.0 (2021-07-29) ============================ Language ----------------------- - [You can now use macros for values in built-in attribute macros.][83366] While a seemingly minor addition on its own, this enables a lot of powerful functionality when combined correctly. Most notably you can now include external documentation in your crate by writing the following. ```rust #![doc = include_str!("README.md")] ``` You can also use this to include auto-generated modules: ```rust #[path = concat!(env!("OUT_DIR"), "/generated.rs")] mod generated; ``` - [You can now cast between unsized slice types (and types which contain unsized slices) in `const fn`.][85078] - [You can now use multiple generic lifetimes with `impl Trait` where the lifetimes don't explicitly outlive another.][84701] In code this means that you can now have `impl Trait<'a, 'b>` where as before you could only have `impl Trait<'a, 'b> where 'b: 'a`. Compiler ----------------------- - [Rustc will now search for custom JSON targets in `/lib/rustlib/<target-triple>/target.json` where `/` is the "sysroot" directory.][83800] You can find your sysroot directory by running `rustc --print sysroot`. - [Added `wasm` as a `target_family` for WebAssembly platforms.][84072] - [You can now use `#[target_feature]` on safe functions when targeting WebAssembly platforms.][84988] - [Improved debugger output for enums on Windows MSVC platforms.][85292] - [Added tier 3\* support for `bpfel-unknown-none` and `bpfeb-unknown-none`.][79608] \* Refer to Rust's [platform support page][platform-support-doc] for more information on Rust's tiered platform support. Libraries ----------------------- - [`panic::panic_any` will now `#[track_caller]`.][85745] - [Added `OutOfMemory` as a variant of `io::ErrorKind`.][84744] - [ `proc_macro::Literal` now implements `FromStr`.][84717] - [The implementations of vendor intrinsics in core::arch have been significantly refactored.][83278] The main user-visible changes are a 50% reduction in the size of libcore.rlib and stricter validation of constant operands passed to intrinsics. The latter is technically a breaking change, but allows Rust to more closely match the C vendor intrinsics API. Stabilized APIs --------------- - [`BTreeMap::into_keys`] - [`BTreeMap::into_values`] - [`HashMap::into_keys`] - [`HashMap::into_values`] - [`arch::wasm32`] - [`VecDeque::binary_search`] - [`VecDeque::binary_search_by`] - [`VecDeque::binary_search_by_key`] - [`VecDeque::partition_point`] Cargo ----- - [Added the `--prune <spec>` option to `cargo-tree` to remove a package from the dependency graph.][cargo/9520] - [Added the `--depth` option to `cargo-tree` to print only to a certain depth in the tree ][cargo/9499] - [Added the `no-proc-macro` value to `cargo-tree --edges` to hide procedural macro dependencies.][cargo/9488] - [A new environment variable named `CARGO_TARGET_TMPDIR` is available.][cargo/9375] This variable points to a directory that integration tests and benches can use as a "scratchpad" for testing filesystem operations. Compatibility Notes ------------------- - [Mixing Option and Result via `?` is no longer permitted in closures for inferred types.][86831] - [Previously unsound code is no longer permitted where different constructors in branches could require different lifetimes.][85574] - As previously mentioned the [`std::arch` instrinsics now uses stricter const checking][83278] than before and may reject some previously accepted code. - [`i128` multiplication on Cortex M0+ platforms currently unconditionally causes overflow when compiled with `codegen-units = 1`.][86063] [85574]: rust-lang/rust#85574 [86831]: rust-lang/rust#86831 [86063]: rust-lang/rust#86063 [86831]: rust-lang/rust#86831 [79608]: rust-lang/rust#79608 [84988]: rust-lang/rust#84988 [84701]: rust-lang/rust#84701 [84072]: rust-lang/rust#84072 [85745]: rust-lang/rust#85745 [84744]: rust-lang/rust#84744 [85078]: rust-lang/rust#85078 [84717]: rust-lang/rust#84717 [83800]: rust-lang/rust#83800 [83366]: rust-lang/rust#83366 [83278]: rust-lang/rust#83278 [85292]: rust-lang/rust#85292 [cargo/9520]: rust-lang/cargo#9520 [cargo/9499]: rust-lang/cargo#9499 [cargo/9488]: rust-lang/cargo#9488 [cargo/9375]: rust-lang/cargo#9375 [`BTreeMap::into_keys`]: https://doc.rust-lang.org/std/collections/struct.BTreeMap.html#method.into_keys [`BTreeMap::into_values`]: https://doc.rust-lang.org/std/collections/struct.BTreeMap.html#method.into_values [`HashMap::into_keys`]: https://doc.rust-lang.org/std/collections/struct.HashMap.html#method.into_keys [`HashMap::into_values`]: https://doc.rust-lang.org/std/collections/struct.HashMap.html#method.into_values [`arch::wasm32`]: https://doc.rust-lang.org/core/arch/wasm32/index.html [`VecDeque::binary_search`]: https://doc.rust-lang.org/std/collections/struct.VecDeque.html#method.binary_search [`VecDeque::binary_search_by`]: https://doc.rust-lang.org/std/collections/struct.VecDeque.html#method.binary_search_by [`VecDeque::binary_search_by_key`]: https://doc.rust-lang.org/std/collections/struct.VecDeque.html#method.binary_search_by_key [`VecDeque::partition_point`]: https://doc.rust-lang.org/std/collections/struct.VecDeque.html#method.partition_point

rustc: target: add sysroot to rust_target_path

6f1ac8d

This enables placing a `target.json` file into the rust sysroot under the target-specific directory. Signed-off-by: Sean Cross <[email protected]>

rust-highfive assigned davidtwco Apr 3, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 3, 2021

This comment has been minimized.

Sign in to view

compiler: run python3 ./x.py fmt

8f73fe9

This fixes a build issue with formatting as part of rust-lang#83800. Signed-off-by: Sean Cross <[email protected]>

davidtwco approved these changes Apr 5, 2021

View reviewed changes

rust-highfive assigned nagisa and unassigned davidtwco Apr 5, 2021

nagisa added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. I-nominated labels Apr 6, 2021

nagisa removed the I-nominated label Apr 10, 2021

nagisa added T-lang Relevant to the language team, which will review and decide on the PR/issue. and removed T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Apr 11, 2021

rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Apr 11, 2021

Mark-Simulacrum reviewed Apr 11, 2021

View reviewed changes

rfcbot added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Apr 24, 2021

Merge remote-tracking branch 'upstream/master' into impl-16351-nightly

f9d390d

Signed-off-by: Sean Cross <[email protected]>

rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels May 4, 2021

apiraino removed the to-announce Announce this issue on triage meeting label May 6, 2021

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 9, 2021

Dylan-DPC-zz mentioned this pull request May 9, 2021

Rollup of 6 pull requests #85125

Closed

bors added the merged-by-bors This PR was explicitly merged by bors. label May 10, 2021

bors merged commit c55c26c into rust-lang:master May 10, 2021

rustbot added this to the 1.54.0 milestone May 10, 2021

Mark-Simulacrum added the relnotes Marks issues that should be documented in the release notes of the next release. label May 10, 2021

xobs deleted the impl-16351-nightly branch May 10, 2021 03:25

nagisa added a commit to nagisa/rust that referenced this pull request May 10, 2021

Adjust target search algorithm for rustlib path

b7c5599

With this the concerns expressed in rust-lang#83800 should be addressed.

nagisa mentioned this pull request May 10, 2021

Adjust target search algorithm for rustlib path #85152

Merged

yhql mentioned this pull request Jan 31, 2023

Download and install custom target JSONs automatically LedgerHQ/cargo-ledger#18

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add default search path to `Target::search()` #83800

Add default search path to `Target::search()` #83800

xobs commented Apr 3, 2021

rust-highfive commented Apr 3, 2021

This comment has been minimized.

davidtwco left a comment

davidtwco commented Apr 5, 2021

nagisa commented Apr 6, 2021 •

edited

Loading

xobs commented Apr 7, 2021

nagisa commented Apr 10, 2021 •

edited

Loading

xobs commented Apr 11, 2021

nagisa commented Apr 11, 2021 •

edited

Loading

rfcbot commented Apr 11, 2021 •

edited by matthewjasper

Loading

Mark-Simulacrum Apr 11, 2021

Mark-Simulacrum May 10, 2021

Mark-Simulacrum commented Apr 11, 2021

petrochenkov commented Apr 18, 2021

nikomatsakis commented Apr 19, 2021

bors commented Apr 24, 2021

rfcbot commented Apr 24, 2021

rfcbot commented May 4, 2021

nagisa commented May 9, 2021

bors commented May 9, 2021

nagisa commented May 9, 2021 •

edited

Loading

bors commented May 9, 2021

Dylan-DPC-zz commented May 9, 2021

rust-log-analyzer commented May 9, 2021

bors commented May 9, 2021

bors commented May 10, 2021

Add default search path to Target::search() #83800

Add default search path to Target::search() #83800

Conversation

xobs commented Apr 3, 2021

rust-highfive commented Apr 3, 2021

This comment has been minimized.

davidtwco left a comment

Choose a reason for hiding this comment

davidtwco commented Apr 5, 2021

nagisa commented Apr 6, 2021 • edited Loading

xobs commented Apr 7, 2021

nagisa commented Apr 10, 2021 • edited Loading

xobs commented Apr 11, 2021

nagisa commented Apr 11, 2021 • edited Loading

rfcbot commented Apr 11, 2021 • edited by matthewjasper Loading

Mark-Simulacrum Apr 11, 2021

Choose a reason for hiding this comment

Mark-Simulacrum May 10, 2021

Choose a reason for hiding this comment

Mark-Simulacrum commented Apr 11, 2021

petrochenkov commented Apr 18, 2021

nikomatsakis commented Apr 19, 2021

bors commented Apr 24, 2021

rfcbot commented Apr 24, 2021

rfcbot commented May 4, 2021

nagisa commented May 9, 2021

bors commented May 9, 2021

nagisa commented May 9, 2021 • edited Loading

bors commented May 9, 2021

Dylan-DPC-zz commented May 9, 2021

rust-log-analyzer commented May 9, 2021

bors commented May 9, 2021

bors commented May 10, 2021

Add default search path to `Target::search()` #83800

Add default search path to `Target::search()` #83800

nagisa commented Apr 6, 2021 •

edited

Loading

nagisa commented Apr 10, 2021 •

edited

Loading

nagisa commented Apr 11, 2021 •

edited

Loading

rfcbot commented Apr 11, 2021 •

edited by matthewjasper

Loading

nagisa commented May 9, 2021 •

edited

Loading