Unify sysroot_target_{bin,lib}dir handling
#132723
Conversation
|
rustbot has assigned @albertlarsan68. Use |
rustbot
added
S-waiting-on-review
| .join("lib"); | ||
| // Avoid deleting the rustlib/ directory we just copied | ||
| // (in `impl Step for Sysroot`). | ||
| let sysroot = builder.sysroot(self.compiler).join(lib).join("rustlib"); |
There was a problem hiding this comment.
Remark [COM 1/3]: This is the rustlib "parent"
| @@ -1212,9 +1206,21 @@ impl<'a> Builder<'a> { | |||
| sysroot | |||
There was a problem hiding this comment.
Remark [COM 2/3]: ... and return the rustlib "parent" instead of rustlib/$target/lib which is the "sysroot target libdir", ...
| /// Returns the bindir for a compiler's sysroot. | ||
| pub fn sysroot_target_bindir(&self, compiler: Compiler, target: TargetSelection) -> PathBuf { | ||
| self.ensure_sysroot_target_dir(compiler, target).join(target).join("bin") | ||
| } | ||
|
|
||
| /// Returns the libdir where the standard library and other artifacts are | ||
| /// found for a compiler's sysroot. | ||
| pub fn sysroot_target_libdir(&self, compiler: Compiler, target: TargetSelection) -> PathBuf { | ||
| self.ensure_sysroot_target_dir(compiler, target).join(target).join("lib") | ||
| } |
There was a problem hiding this comment.
Remark [COM 3/3]: ... so that sysroot_target_bindir can construct a path with child directories without having to backtrack, and they now clearly look related.
|
r? @onur-ozkan (inactivity, but also you might know more about this logic as per #132782) |
I'll slightly adjust this to split the side-effecting ensure directories exist / don't exist or installing dist llvm logic separate from the path calculation to make any kind of test coverage remotely possible. |
jieyouxu
added
S-waiting-on-author
I am aware of that. |
jieyouxu
force-pushed
the
sysroot-dance-dance-revolution
branch
from
770a80f
to
7610aa5
Compare
|
I tried to add a test for the path calculation by adjusting the logic such that the side-effecting logic is not run in self-check dry run mode, but I feel like it's not really testing much, and I'm not sure how else to test this. Let me know what you think. @rustbot ready |
rustbot
added
S-waiting-on-review
I think it seems quite good, thanks! r=me once CI is green. |
|
@bors r+ |
bors
added
S-waiting-on-bors
…iaskrgr Rollup of 6 pull requests Successful merges: - rust-lang#132723 (Unify `sysroot_target_{bin,lib}dir` handling) - rust-lang#133041 (Print name of env var in `--print=deployment-target`) - rust-lang#133325 (Reimplement `~const` trait specialization) - rust-lang#133395 (Add simd_relaxed_fma intrinsic) - rust-lang#133517 (Deeply normalize when computing implied outlives bounds) - rust-lang#133785 (Add const evaluation error UI test.) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#132723 - jieyouxu:sysroot-dance-dance-revolution, r=onur-ozkan Unify `sysroot_target_{bin,lib}dir` handling Follow-up to rust-lang#131405 (comment) where `sysroot_target_bindir` had to do some dancing because the sysroot ensure logic embedded in `sysroot_target_libdir` returned `$sysroot/$relative_lib/rustlib/$target/lib` and not the `rustlib` parent `$sysroot/$relative_lib/rustlib/`. This PR pulls out the sysroot ensure logic into a helper, and return `$sysroot/$relative_lib/rustlib/` instead so `sysroot_target_bindir` doesn't have to do parent traversal from the path returned from `sysroot_target_libdir`, and also make them easier to follow in that they are now clearly closely related based on the common target sysroot ensure logic.
Follow-up to #131405 (comment) where
sysroot_target_bindirhad to do some dancing because the sysroot ensure logic embedded insysroot_target_libdirreturned$sysroot/$relative_lib/rustlib/$target/liband not therustlibparent$sysroot/$relative_lib/rustlib/.This PR pulls out the sysroot ensure logic into a helper, and return
$sysroot/$relative_lib/rustlib/instead sosysroot_target_bindirdoesn't have to do parent traversal from the path returned fromsysroot_target_libdir, and also make them easier to follow in that they are now clearly closely related based on the common target sysroot ensure logic.