-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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 bootstrap and shim binaries #95164
Conversation
(rust-highfive has picked a reviewer for you, use r? to override) |
This comment has been minimized.
This comment has been minimized.
a4ecadb
to
eeb4fe9
Compare
This comment has been minimized.
This comment has been minimized.
eeb4fe9
to
2fdf200
Compare
This comment has been minimized.
This comment has been minimized.
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.
This is awesome ❤️
src/bootstrap/lib.rs
Outdated
if bootstrap_out.join("bootstrap").exists() && !bootstrap_out.join("rustc").exists() { | ||
let bin = bootstrap_out.join("bootstrap"); | ||
fs::hard_link(&bin, bootstrap_out.join("rustc")) | ||
.expect("failed to link rustc to bootstrap shim"); |
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.
Maybe use Build::copy
instead? It hardlinks and falls back to copying if it fails. Some systems may not support hardlinks.
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.
Build::copy defeats the point, it means the new files won't be updated on changes.
I don't know a good solution here :/ maybe we could do test somewhere of whether the fs supports file links and copy unconditionally if so?
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.
it means the new files won't be updated on changes
Not sure what you meant. The current implementation is as follows:
/// Copies a file from `src` to `dst`
pub fn copy(&self, src: &Path, dst: &Path) {
if self.config.dry_run {
return;
}
self.verbose_than(1, &format!("Copy {:?} to {:?}", src, dst));
if src == dst {
return;
}
let _ = fs::remove_file(&dst);
let metadata = t!(src.symlink_metadata());
if metadata.file_type().is_symlink() {
let link = t!(fs::read_link(src));
t!(symlink_file(link, dst));
} else if let Ok(()) = fs::hard_link(src, dst) {
// Attempt to "easy copy" by creating a hard link
// (symlinks don't work on windows), but if that fails
// just fall back to a slow `copy` operation.
} else {
if let Err(e) = fs::copy(src, dst) {
panic!("failed to copy `{}` to `{}`: {}", src.display(), dst.display(), e)
}
t!(fs::set_permissions(dst, metadata.permissions()));
let atime = FileTime::from_last_access_time(&metadata);
let mtime = FileTime::from_last_modification_time(&metadata);
t!(filetime::set_file_times(dst, atime, mtime));
}
}
Which is akin to an unconditional hard_link.
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.
No, because if you hit the fs::copy path, then the next time bootstrap is executed "rustc".exists() will return true and it will never recopy the binary. Which defeats the whole point of this change.
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 is true, I will just remove the check for rustc.exists()
because it was triggering a filesystem error because the dest file already existed.
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 suppose that works too. The hard link should be very cheap compared to everything else bootstrap does 👍
c94b2b3
to
52a1dc0
Compare
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.
Can we name the "primary" (i.e., the one produced by rustc) binary something less difficult to refer to? Maybe rustbuild-binary-dispatch-shim?
Please also squash the commits as you make changes, no need for "review changes" commits.
a6aa04c
to
578826a
Compare
This comment has been minimized.
This comment has been minimized.
578826a
to
57c250f
Compare
This comment has been minimized.
This comment has been minimized.
57c250f
to
812d6d9
Compare
This comment has been minimized.
This comment has been minimized.
812d6d9
to
cc29d18
Compare
This comment has been minimized.
This comment has been minimized.
cc29d18
to
cdfa1a8
Compare
☔ The latest upstream changes (presumably #95253) made this pull request unmergeable. Please resolve the merge conflicts. |
cdfa1a8
to
59d85f4
Compare
they are now dispatched based on the binary name it is called with. `target/debug/rust{c, doc}` are now hard linked to `bootstrap`.
Umm, I took a look and it looks like it already tries to remove the destination before copying: https://cs.github.com/rust-lang/rust/blob/fee75fbe11b1fad5d93c723234178b2a329a3c03/src/bootstrap/lib.rs#L1418 |
b86bec7
to
b616d7b
Compare
Windows doesn't allow deleting files that are currently open by default. I believe FILE_DISPOSITION_POSIX_SEMANTICS allows deleting open files though. |
So I tried to use |
I'm not sure that function is publicly exposed, but we can try. @bors r+ |
📌 Commit 64a8af9 has been approved by |
⌛ Testing commit 64a8af9 with merge c0220834cf72ccea19c0454ad82db04e18ea67a7... |
💔 Test failed - checks-actions |
The job Click to see the possible cause of the failure (guessed by this bot)
|
⌛ Testing commit 64a8af9 with merge 83698375db8b68e4b56f5a650443f96accc44471... |
💔 Test failed - checks-actions |
The job Click to see the possible cause of the failure (guessed by this bot)
|
This should no longer be necessary with the new plan, since no one will run Sorry to spend so much of your time on this. |
I did take a look at rustup, and it looks like they retry the deletion with timeout instead of invoking the functions once. Maybe that will resolve the issue (perhaps due to concurrency?) But if this is not needed anymore I'm happy to leave it. |
they are now dispatched based on the binary name it is called with.
target/debug/rust{c, doc}
are now hard linked tobootstrap
.This fixes #95141, cc @jyn514.