-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Change build-std to use --sysroot #7421
Changes from 3 commits
e9dd306
bc5c441
53a3db0
d0ab04a
581da42
3afb5d7
abe92bc
9d86d86
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -116,6 +116,10 @@ pub struct Layout { | |
examples: PathBuf, | ||
/// The directory for rustdoc output: `$root/doc` | ||
doc: PathBuf, | ||
/// The local sysroot for the build-std feature. | ||
sysroot: Option<PathBuf>, | ||
/// The "lib" directory within `sysroot`. | ||
sysroot_libdir: Option<PathBuf>, | ||
/// The lockfile for a build (`.cargo-lock`). Will be unlocked when this | ||
/// struct is `drop`ped. | ||
_lock: FileLock, | ||
|
@@ -139,18 +143,21 @@ impl Layout { | |
// Flexible target specifications often point at json files, so interpret | ||
// the target triple as a Path and then just use the file stem as the | ||
// component for the directory name in that case. | ||
if let Some(triple) = triple { | ||
let triple = Path::new(triple); | ||
if triple.extension().and_then(|s| s.to_str()) == Some("json") { | ||
root.push( | ||
triple | ||
.file_stem() | ||
let triple_path = if let Some(s) = triple { | ||
let p = Path::new(s); | ||
let tp = if p.extension().and_then(|s| s.to_str()) == Some("json") { | ||
Path::new( | ||
p.file_stem() | ||
.ok_or_else(|| failure::format_err!("invalid target"))?, | ||
); | ||
) | ||
} else { | ||
root.push(triple); | ||
} | ||
} | ||
p | ||
}; | ||
root.push(tp); | ||
Some(tp) | ||
} else { | ||
None | ||
}; | ||
let dest = root.join(dest); | ||
// If the root directory doesn't already exist go ahead and create it | ||
// here. Use this opportunity to exclude it from backups as well if the | ||
|
@@ -167,6 +174,16 @@ impl Layout { | |
let root = root.into_path_unlocked(); | ||
let dest = dest.into_path_unlocked(); | ||
|
||
// Compute the sysroot path for the build-std feature. | ||
let build_std = ws.config().cli_unstable().build_std.as_ref(); | ||
let (sysroot, sysroot_libdir) = if let Some(tp) = build_std.and(triple_path) { | ||
let sysroot = dest.join(".sysroot"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd have a mild preference for not hiding this directory by default, but I'm curious why you added the leading period? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The issue is that named profiles will be taking over this namespace (each named profile has a directory with its name). I'd like to avoid potential conflicts. An alternative would be to reserve "sysroot" and not allow it as a profile name (which won't be much of an option once named profiles stabilizes). Or we could come up with some other structure or naming convention. Any preferences? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah ok sounds good to me! Can you add that to a comment indicating why there's a leading dot? |
||
let sysroot_libdir = sysroot.join("lib").join("rustlib").join(tp).join("lib"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So technically this isn't going to work on all compilers digging in a bit. The sysroot finding function in rustc takes into account Now that's pretty esoteric though and only really relevant for Linux distributions, but it may be worth keeping in mind that this may run afoul of that at some point. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh… I'll try to investigate. There doesn't seem to be a way to discover the actual path. |
||
(Some(sysroot), Some(sysroot_libdir)) | ||
} else { | ||
(None, None) | ||
}; | ||
|
||
Ok(Layout { | ||
deps: dest.join("deps"), | ||
build: dest.join("build"), | ||
|
@@ -176,6 +193,8 @@ impl Layout { | |
doc: root.join("doc"), | ||
root, | ||
dest, | ||
sysroot, | ||
sysroot_libdir, | ||
_lock: lock, | ||
}) | ||
} | ||
|
@@ -223,6 +242,16 @@ impl Layout { | |
pub fn build(&self) -> &Path { | ||
&self.build | ||
} | ||
/// The local sysroot for the build-std feature. | ||
/// | ||
/// Returns None if build-std is not enabled or this is the Host layout. | ||
pub fn sysroot(&self) -> Option<&Path> { | ||
self.sysroot.as_ref().map(|p| p.as_ref()) | ||
} | ||
/// The "lib" directory within `sysroot`. | ||
pub fn sysroot_libdir(&self) -> Option<&Path> { | ||
self.sysroot_libdir.as_ref().map(|p| p.as_ref()) | ||
} | ||
} | ||
|
||
#[cfg(not(target_os = "macos"))] | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,9 +1,8 @@ | ||
[workspace] | ||
members = [ | ||
"src/liballoc", | ||
"src/libcore", | ||
"src/libproc_macro", | ||
"src/libstd", | ||
"src/libtest", | ||
] | ||
|
||
[patch.crates-io] | ||
rustc-std-workspace-std = { path = 'src/tools/rustc-std-workspace-std' } | ||
rustc-std-workspace-core = { path = 'src/tools/rustc-std-workspace-core' } | ||
rustc-std-workspace-alloc = { path = 'src/tools/rustc-std-workspace-alloc' } |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1,11 @@ | ||
pub fn custom_api() {} | ||
#![feature(staged_api)] | ||
#![stable(since = "1.0.0", feature = "dummy")] | ||
|
||
extern crate alloc; | ||
|
||
#[stable(since = "1.0.0", feature = "dummy")] | ||
pub use alloc::*; | ||
|
||
#[stable(since = "1.0.0", feature = "dummy")] | ||
pub fn custom_api() { | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to sink this work into the unit itself? Ideally we wouldn't be doing much extra stuff here in
job_queue.rs
and it keeps all the major work on each thread cargo spawns for work.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how that would work, particularly with pipelining. The rmeta json artifact notification goes through a generic route that most conveniently ends up here in JobQueue. I could disable pipelining and move it to the Work closure instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see what you mean. Thinking more on this though, I was actually assuming that we'd just change
--out-dir
foris_std
crates. It looks like otherwise they're gonna show up both in the libdir as well as the maindeps
folder, right? If we switched--out-dir
I think it'd solve this issue naturally?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Er wait no we already talked about it and ruled it out because we want to clean out the sysroot on each build.
In any case this poses an interesting scenario where the dependencies of
std
can be loaded from either the sysroot or-Ldependency=.../deps
, they're actually all the same. I'm asking this from the point of view of concurrency which I think this issue is trying to solve (making sure we do this before the next compilation starts), but maybe that's not actually necessary?Ok I want to try to work this through here in a coment...
--extern
I believe. If that's the case then I think that nothing is necessary here in terms of sequencing things right because everything is loaded fromdeps
core
, but nothing may actually be in the sysroot yet, so the compiler won't be able to find anything.Does that sound right? If so...
So presumably this would otherwise go somewhere around here (transitively), and you're right that's a "pretty generic path" which isn't a great spot to put it.
Ok if you agree with what I've said above, let's leave this here, but add a comment perhaps saying that we may want to figure out how to offload it to worker threads eventually?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be cleaner. For some reason I was thinking that was going to be harder (since the output directory is somewhat global), but it probably won't be too hard. I'll give it a shot.
EDIT: Oh, and I think I was also concerned about not atomically adding files while concurrent rustc's are running, but you mentioned that it shouldn't be too much of a problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, yea, that's the actual reason. Sorry, I'm mis-remembering things.
That makes sense. I'll add a comment.
I was previously trying to think of a way that
OutputFile::hard_link
(or something like it) could be used, but it still runs into the problem with rmeta files needing to be available, and linking isn't done until after compilation finishes.