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

Work-around lack of artifact dependencies when using cargo run -p bootstrap #94828

Closed
wants to merge 1 commit into from

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Mar 10, 2022

While working on #94806, I was very confused
because the trim() fix didn't actually do anything and still errored out.
It turns out the issue was that bootstrap had been rebuilt, but not fake rustc.
Rebuild all the fake binaries automatically from within bootstrap to avoid issues like this in the future.

…otstrap`

While working on rust-lang#94806, I was very confused
because the `trim()` fix didn't actually do anything and still errored out.
It turns out the issue was that bootstrap had been rebuilt, but not fake rustc.
Rebuild all the fake binaries automatically from within bootstrap to avoid issues like this in the future.
@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 10, 2022
@Mark-Simulacrum
Copy link
Member

Based on rust-lang/cargo#9096 (comment), it looks like this should be available in nightly as of fairly recently. That makes me fairly optimistic about landing this, since while this solution I'm not a particular fan of, the 'official' solution seems like something we could use fairly well.

Presumably using that will require us to rearchitect a little how bootstrap is constructed, unless I'm missing something in the docs -- in particular, splitting out the binaries into separate crates?

Given that we will want to migrate to bindeps in ~4 weeks after the next bootstrap bump, do you think it still makes sense to land this change? If so, happy to do that. @bors delegate+

@bors
Copy link
Contributor

bors commented Mar 13, 2022

✌️ @jyn514 can now approve this pull request

@Mark-Simulacrum Mark-Simulacrum 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 Mar 13, 2022
@jyn514
Copy link
Member Author

jyn514 commented Mar 14, 2022

Oh wow, I didn't realize the RFC was already implemented!

Given that we will want to migrate to bindeps in ~4 weeks after the next bootstrap bump, do you think it still makes sense to land this change?

It still requires nightly cargo features, right? It's -Zbindeps, I thought bootstrap shouldn't use nightly features to build. I don't mind waiting 4 weeks, but I'd rather not wait until it's stabilized, which could be quite a while.

@Mark-Simulacrum
Copy link
Member

Yeah, it's a good thing to think about. I'm not sure. I am pretty reluctant to merge this as-is, though, without a relatively fast migration to some more natural solution -- a cargo build that can't get arguments or environment variables set seems prone for problems (e.g., folks setting various linker variables, etc), and that seems unfortunate to introduce. I suppose it should be a no-op for anyone going through the official path of x.py for now, but it's still not great I think.

Let me give it some more thought and I'll try to decide on what feels like the best course of action. Right now I'm reluctant either way :) I think maybe it would be OK to land this as-is since it's gated on the non-bootstrap.py workflow, but I don't think I would want to "stabilize/recommend" that workflow until we have a path to stability for bindeps.

It may also be that we can refactor things to remove the question, for example by making rustbuild a binary that dispatches to different main functions, like rustup. That would probably not be too hard and would eliminate the need for an unstable Cargo feature.

@jyn514
Copy link
Member Author

jyn514 commented Mar 14, 2022

It may also be that we can refactor things to remove the question, for example by making rustbuild a binary that dispatches to different main functions, like rustup. That would probably not be too hard and would eliminate the need for an unstable Cargo feature.

Ooh, I like this idea :) happy to implement this, I think it should be simple as adding all 3 entrypoints to bin/main.rs and adding symlinks to target/debug/bootstrap in target/debug/{rustc,rustdoc}.

@jyn514
Copy link
Member Author

jyn514 commented Mar 20, 2022

Closing this for now, I don't think any of the current changes are helpful if we're using a shared binary. I opened #95141 to track that switch.

@jyn514 jyn514 closed this Mar 20, 2022
@jyn514 jyn514 deleted the cargo-build-bins branch March 20, 2022 15:44
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants