-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Conversation
…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 has picked a reviewer for you, use r? to override) |
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+ |
✌️ @jyn514 can now approve this pull request |
Oh wow, I didn't realize the RFC was already implemented!
It still requires nightly cargo features, right? It's |
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. |
Ooh, I like this idea :) happy to implement this, I think it should be simple as adding all 3 entrypoints to |
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. |
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.