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

Unify sysroot_target_{bin,lib}dir handling #132723

Merged
merged 1 commit into from
Dec 3, 2024

Conversation

jieyouxu
Copy link
Member

@jieyouxu jieyouxu commented Nov 7, 2024

Follow-up to #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.

@rustbot
Copy link
Collaborator

rustbot commented Nov 7, 2024

r? @albertlarsan68

rustbot has assigned @albertlarsan68.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Nov 7, 2024
.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");
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remark [COM 1/3]: This is the rustlib "parent"

@@ -1212,9 +1206,21 @@ impl<'a> Builder<'a> {
sysroot
Copy link
Member Author

@jieyouxu jieyouxu Nov 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remark [COM 2/3]: ... and return the rustlib "parent" instead of rustlib/$target/lib which is the "sysroot target libdir", ...

Comment on lines 1213 to 1223
/// 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")
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@jieyouxu
Copy link
Member Author

r? @onur-ozkan (inactivity, but also you might know more about this logic as per #132782)

@rustbot rustbot assigned onur-ozkan and unassigned albertlarsan68 Nov 29, 2024
Copy link
Member

@onur-ozkan onur-ozkan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By any chance, can we add a test coverage for sysroot_target_bindir and sysroot_target_libdir?

@jieyouxu
Copy link
Member Author

jieyouxu commented Nov 29, 2024

By any chance, can we add a test coverage for sysroot_target_bindir and sysroot_target_libdir?

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 jieyouxu added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 29, 2024
@onur-ozkan
Copy link
Member

By any chance, can we add a test coverage for sysroot_target_bindir and sysroot_target_libdir?

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.

I am aware of that.

@jieyouxu jieyouxu force-pushed the sysroot-dance-dance-revolution branch from 770a80f to 7610aa5 Compare December 2, 2024 05:23
@jieyouxu
Copy link
Member Author

jieyouxu commented Dec 2, 2024

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 rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 2, 2024
@onur-ozkan
Copy link
Member

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

I think it seems quite good, thanks!

r=me once CI is green.

@onur-ozkan
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Dec 2, 2024

📌 Commit 7610aa5 has been approved by onur-ozkan

It is now in the queue for this repository.

@bors 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 Dec 2, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 3, 2024
…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
@bors bors merged commit 481ca61 into rust-lang:master Dec 3, 2024
6 checks passed
@rustbot rustbot added this to the 1.85.0 milestone Dec 3, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Dec 3, 2024
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.
@jieyouxu jieyouxu deleted the sysroot-dance-dance-revolution branch December 3, 2024 18:49
@fmease
Copy link
Member

fmease commented Dec 4, 2024

sync @bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants