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.

Sign up for GitHub

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

Jump to bottom

Link to current nightly #55

Merged
merged 4 commits into from
Feb 24, 2020
Merged

Link to current nightly #55

merged 4 commits into from
Feb 24, 2020

Conversation

pnkfelix
Copy link
Member

Fix code that was trying reuse an already installed nightly, by making install call out to rustup toolchain link to link the currently installed nightly up to the new "bisector-nightly-xxx" that we expect.

Fix #54

@pnkfelix
Copy link
Member Author

r? @spastorino

@Mark-Simulacrum
Copy link
Member

This looks slightly wrong to me. The default toolchain could be nightly-2020-01-04, in which case our synthesis of the link path is wrong i believe. I suspect we should instead run rustc --print sysroot or so and get the path from there.

I was also concerned about removing someone's installed nightly, but it looks like remove_dir_all isn't supposed to follow symlinks (though I'm not sure what behavior is like for hardlinks, I believe rustup link doesn't create those). I guess modulo rust-lang/rust#48504 but hopefully we don't hit that.

@pnkfelix
Copy link
Member Author

The default toolchain could be nightly-2020-01-04, in which case our synthesis of the link path is wrong i believe. I suspect we should instead run rustc --print sysroot or so and get the path from there.

Yeah I just realized this while I was experimenting with this and happened to be making local changes to my rustup's default nightly. Let me see if I can fix it.

(It seems like rustup is missing some functionality here, no? I.e. I could imagine we "need" a subcommand like rustup toolchain print-install-dir <toolchain> or something along those lines...)

@pnkfelix pnkfelix changed the title Link to current nightly [WIP] Link to current nightly Feb 21, 2020
@Mark-Simulacrum
Copy link
Member

Yes, rustup isn't really ready for scripting today, semi-intentionally. Or so I recall hearing from @kinnison... I think.

spastorino
spastorino reviewed Feb 21, 2020
View reviewed changes
src/main.rs Outdated Show resolved Hide resolved
src/main.rs Show resolved Hide resolved
@pnkfelix
Fix code that was trying reuse an already installed nightly.
661b76d 
Before, we would just skip attempts to install a current nightly. (And then, we
would unconditionally fail the run as documented on issue rust-lang#54, regardless
whether of whether the regression was actually visible on that version, due to
it not being installed with the name "bisector-nightly-xxx" that we expect.)

This commit fixes that. It makes `install` call out to `rustup toolchain link`
to link the current rustc (as reported via `rustc --print sysroot`) up to the
new "bisector-nightly-xxx" that we expect.

(I also refactored the code to construct `DownloadParams`; that refactoring was
more relevant in an earlier version of the code that was linking in a different
(buggy) way.)
@pnkfelix
This updates the remove function to remove the installed directory
e819b3a 
unconditionally.

This is a sort of scary thing to do, so I added a safe-guard to double-check
that whatever directory we are removing, it is one of the ones starts with
"bisector" and thus is something cargo-bisect-rustc is implicitly in charge of.
@pnkfelix
Realized there was another case where we delete directories "blindly"…
f8f6b6e 
…: forced installs.

Extended safeguard from previous commit to cover that case too.
@pnkfelix pnkfelix force-pushed the link-to-current-nightly branch from 8ebf631 to f8f6b6e Compare February 22, 2020 01:27
@pnkfelix
Copy link
Member Author

(just forced-push a new version that uses rustc --print sysroot to figure out the directory to link to, instead of trying to (incorrectly) infer it.)

@pnkfelix @spastorino
fix typo in src/main.rs
b074ad7 
thanks @spastorino

Co-Authored-By: Santiago Pastorino <spastorino@gmail.com>
@pnkfelix pnkfelix changed the title [WIP] Link to current nightly Link to current nightly Feb 22, 2020
@pnkfelix
Copy link
Member Author

I was also concerned about removing someone's installed nightly, but it looks like remove_dir_all isn't supposed to follow symlinks

You know, a way to definitely avoid this risk is to "just" fully copy the directory tree locally, rather than use a symbolic link. And then the resulting directory structure for the bisector variants would be totally uniform, and it would also avoid the risk (when using --preserve) of a user independently removing an installed nightly that we linked to...

(it would also be wasteful of disk space, of course... but that might be a micro-optimization for something like this.)

Its at least something to consider, though I would be happy just landing this PR as is, rather than bikeshedding this question.

@spastorino spastorino merged commit 4f67573 into rust-lang:master Feb 24, 2020
@ehuss ehuss mentioned this pull request Dec 31, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@spastorino spastorino spastorino left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

nightly for rustup default breaks cargo-bisectc-rustc when start..end range overaps current nightly
3 participants
@pnkfelix @Mark-Simulacrum @spastorino