-
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
Add --run-stage and --link-stage aliases #75167
Conversation
I think @mark-i-m and @spastorino are also planning to make a PR to rustc-dev-guide using the diagram I linked: https://github.com/rust-lang/rustc-dev-guide/files/5024984/diagram.pdf |
This PR does not affect behavior around stages in any way. Instead, it adds aliases for `--stage` which represent how the stage is interpreted by bootstrap. See rust-lang/rustc-dev-guide#807 (comment) and https://rust-lang.zulipchat.com/#narrow/stream/196385-t-compiler.2Fwg-rustc-dev-guide/topic/meeting.2008.2E04.2E2020 for more discussion.
👍 to the idea of looking for more accurate names. In case this is a good idea for others, I'd suggest to deprecate/remove |
} | ||
"bench" => { | ||
opts.optmulti("", "test-args", "extra arguments", "ARGS"); | ||
link_stage(&mut opts) |
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 feels definitely wrong for bench and test to have different flags...
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.
The way I chose this is by looking at whether src/rustc
was built for that stage. Or in other words, whether it does any work when you pass it --stage 0
. So build --stage 0 src/rustc
does work; test --stage 0 src/test/ui
does nothing (it runs the beta compiler); bench --stage 0 src/rustc
does work; etc.
opts.optopt( | ||
"", | ||
"link-stage", | ||
"stage to use, in terms of what libraries it is linked to (e.g. `--link-stage 1` means use `build/stage1-<component>`). \ |
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 also feels confusing, because we never link to build/stage1-component, we always link to stageN/lib/rustlib/.../lib/... which is uplifted from stageN-components. (and in stage0 that directory is called stage0-sysroot, iirc, but I might be misremembering).
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.
Ok, how does this look then?
"stage to use, in terms of what libraries it is linked to (e.g. `--link-stage 1` means use `build/stage1-<component>`). \ | |
"stage to use, in terms of what libraries it is linked to (e.g. `--link-stage 1` means to build `build/stage1-<component>` and uplift it to `build/stage2/lib/rustlib`). \ |
I don't personally see this as helpful, but then again I feel that I have a pretty good grasp as to which arguments to pass when (understandably, given that I've been involved with rustbuild/bootstrap for ~4 years now), so maybe I'm not the best judge. It does feel like adding more names would just increase people's confusion, especially if (like they do now) they all map to the same underlying options in bootstrap. It feels like there is inherent complexity here that you need to understand if you pass --stage at all -- but the expectation after the x.py defaults change is that no one should need to do so, right? (Or maybe not quite, and we need to change some more defaults?) My hope is that the inherent complexity with staging can be hidden away from users behind good defaults and then we don't need to try and explain it. |
I think the very fact that they map to the same underlying options is confusing. If you run
Well, sort of. +1 for passing |
Also, note that until intra-doc links are more stabilized there is no good default for |
You're not wrong that this is perhaps confusing, but it doesn't seem like adding 2 new terms would be helpful here. If we want to change this such that staging always refers to the rustc, for example, then that seems like something to explore. But just adding options doesn't feel like a win, especially if we're going to end up wanting to change the meaning of those options down the line. Insofar as "you will need to pass --stage" -- I feel like this is a failing if true. Similar to how Rust as a whole tries to make it such that the vast majority of code won't need to use unsafe, the vast majority of people shouldn't need to use --stage. And, sure, if you do need to use it will take you a bit to understand the model, and maybe the model isn't entirely consistent right now; I am on board with updating our model to be more consistent. But -- IMO -- it is true that the current model can be understood; certainly we don't have good documentation, and maybe there's rough edges that can be smoothed out, but it does seem fixable.
Note that stabilization here is irrelevant, just the implementation gaining functionality is relevant. And yes, it is true that when significant functionality is added to rustdoc and that functionality is used in our documentation without |
Maybe it would help if you explained your mental model a little more? In my mind, since
I've met with a lot of resistance on this front (mostly from @eddyb :P) - and I'm not sure it's unwarranted, it's not just changing the defaults like #73964 but changing the whole mental model. So I thought that this would be good in the meantime to make it clearer what the options do currently, without changing behavior. FWIW the new arguments correspond to the new mental model I suggested for using the same
Getting a little off-topic, but that's not exactly what's going on with intra-doc links. This isn't just using a shiny feature, it's fixing links in |
Related: rust-lang/rustc-dev-guide#843. I think this should wait until the discussion there is resolved at a minimum, and then if it turns out this is a useful mental model we could consider adding these aliases. |
I'm just going to close this in that case, I don't think keeping the PR open helps personally. |
This PR does not affect behavior around stages in any way. Instead,
it adds aliases for
--stage
which represent how the stage isinterpreted by bootstrap.
See rust-lang/rustc-dev-guide#807 (comment) and https://rust-lang.zulipchat.com/#narrow/stream/196385-t-compiler.2Fwg-rustc-dev-guide/topic/meeting.2008.2E04.2E2020 for more discussion.
The
--run-stage
for--test
is technically incorrect because ofrun-make-fulldeps
, but that seems like enough of an edge case to use the more common interpretation.@Mark-Simulacrum I'm mostly interested in whether these are accurate and useful descriptions of what bootstrap does. What do you think? The hope is that this will make stages less confusing all around.
r? @Mark-Simulacrum
cc @spastorino , @mark-i-m