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

Make the default stage for x.py configurable #76165

Closed
jyn514 opened this issue Aug 31, 2020 · 9 comments · Fixed by #76625
Closed

Make the default stage for x.py configurable #76165

jyn514 opened this issue Aug 31, 2020 · 9 comments · Fixed by #76625
Labels
C-feature-request Category: A feature request, i.e: not implemented / a PR. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)

Comments

@jyn514
Copy link
Member

jyn514 commented Aug 31, 2020

The defaults are a lot better after #73964, but they still aren't ideal for everyone. Some issues that have been mentioned:

  • x.py test does not run ui-fulldeps (and inherently can't with --stage 1): Ignore gated-plugin test on stage1 #75404 (comment)
  • x.py doc uses stage 0 by default which is not my preference, I almost always want ---stage 1. But new contributors will normally want --stage 0, so it doesn't make sense to change it for everyone.

It would be great to allow configuring the default stage on a per-subcommand level. This would both make it easier to add new commands to the test suite (e.g. #70533 (comment)), and more convenient for local contributors who want defaults other than those hardcoded into bootstrap. The existing defaults could still be left in config.toml.example, making this backwards-compatible for contributors.

@jyn514 jyn514 added T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) C-feature-request Category: A feature request, i.e: not implemented / a PR. labels Aug 31, 2020
@jyn514 jyn514 changed the title Make the default stage configurable Make the default stage for x.py configurable Aug 31, 2020
@infinity0
Copy link
Contributor

Instead of trying to second-guess what everyone's individual subjective workflow is, I think it's much better to make a CLI that is simple and predictable. That means that defaults shouldn't change randomly based on second-guessing everyone's individual subjective workflow.

What is simpler to understand, a function like f(x) = x^2, or a function like

f(x) = x^2 if x in [0..1]
       x^3 if x in [1..10]
       x^4 if x in [10..]

Why is the default stage different between test and bench? I don't know, and I don't want to have to know. I already need to know a lot of things about rust already in order to maintain it, having to know extra idiosyncracies makes the task less fun.

If someone does not understand --stage well enough to optimise their own workflow (possibly via a local-only ./y.py script) when working on rustc, should they really be working on rustc?

@infinity0
Copy link
Contributor

Alternatively, have x.py be the wrapper "newbie-friendly convenience" script that passes --stage arguments that second-guess what a typical newbie wants, and have bootstrap.py be the simple consistent CLI that works predictably and is easier to automate around.

@jyn514
Copy link
Member Author

jyn514 commented Sep 5, 2020

This sounds more like a reply to #73964 than anything else; it's not really related to this issue.

I'm sorry this broke your workflow. There was an MCP in the compiler-team repo for a couple weeks but I understand you don't have time to watch every major change.

Instead of trying to second-guess what everyone's individual subjective workflow is

I object to calling the changes second-guessing. These aren't just my personal preferences, they're what the majority of frequent contributors were using before the changes. Having the defaults being different from the recommended workflow made it much harder for new contributors to get started and more annoying for frequent contributors.

I think it's much better to make a CLI that is simple and predictable.
have bootstrap.py be the simple consistent CLI that works predictably and is easier to automate around.

I have no objection to this! It wouldn't be hard to keep it updated either since the defaults rarely change - maybe you'd be interested in a PR adding it? ;)

What is simpler to understand

You're right that the new defaults are not simpler, however I believe they're more helpful, because they require less work in the common case.

If someone does not understand --stage well enough to optimise their own workflow (possibly via a local-only ./y.py script) when working on rustc, should they really be working on rustc?

This is gatekeeping and I don't appreciate it.

@infinity0
Copy link
Contributor

Thanks for the balanced response. I will see about making a PR when I have time, but it'd be nice if whoever else pushes this issue forward, does it whilst also making the change I suggested.

I object to calling the changes second-guessing. [..] Having the defaults being different from the recommended workflow made it much harder for new contributors to get started and more annoying for frequent contributors. [..] You're right that the new defaults are not simpler, however I believe they're more helpful, because they require less work in the common case.

The very existence of this issue indicates that you understand different people have different preferences for the defaults. You may be right that these defaults would satisfy a majority of contributors, but neither of us really know for sure. So I think it's fair to call this second-guessing.

Also I think you are mixing up "new contributors" and "frequent contributors". As someone else mentioned on #73964, these new defaults make it quicker to manually execute a debug-oriented workflow, as a frequent contributor. However imagine that all your knowledge about rustc was wiped away and that you are a new contributor. You checkout rust.git, run ./x.py build and it doesn't actually build rustc. What are you going to do next? Where is the documentation that tells you how to actually build rustc? Remember you don't know what a "stage" is. And installing sounds like a commitment that newbies typically don't want to do before trying out the thing directly from the build directory.

Generally a good principle is to make the primary API as simple and predictable as possible - ideally the overall options available should be a simple cartesian product of all the individual options. Then one can think about a secondary API with some convenient defaults. For example (as another alternative to my ./x.py suggestion) your test --stage 1 could have been implemented as another subcommand quick-test that is an alias for test --stage 1, instead of changing the behaviour of the primary API. Humans can consume the secondary API, automated scripts can consume the primary API.

@jyn514
Copy link
Member Author

jyn514 commented Sep 5, 2020

run ./x.py build and it doesn't actually build rustc

This is not the case. x.py build still builds stage 0 rustc artifacts, just not stage 1 rustc artifacts. See rust-lang/compiler-team#326 (comment) - this is one of the main reasons I wanted to change the defaults, because of the confusion around this point (maybe I should go into more detail on this in the blog post?).

our test --stage 1 could have been implemented as another subcommand quick-test that is an alias for test --stage 1, instead of changing the behaviour of the primary API. Humans can consume the secondary API, automated scripts can consume the primary API.

Hmm ... in retrospect this might have been a better idea. But I don't think it's worth changing it back now the change is already made - now automated tools and humans have to think about what the defaults are because it's really non-obvious and depends where in time you are. If you do think this is worth pursuing feel free to open an MCP: https://github.com/rust-lang/compiler-team/issues/new/choose

it'd be nice if whoever else pushes this issue forward, does it whilst also making the change I suggested.

Sure, I don't think the issues are linked but I don't mind adding a 'stable' bootstrap interface at the same time.

@infinity0
Copy link
Contributor

infinity0 commented Sep 5, 2020

This is not the case. x.py build still builds stage 0 rustc artifacts, just not stage 1 rustc artifacts. See rust-lang/compiler-team#326 (comment) - this is one of the main reasons I wanted to change the defaults, because of the confusion around this point (maybe I should go into more detail on this in the blog post?).

Yes, the situation is confusing. Because now my question is, what does ./x.py build --stage 0 do? Going by the examples on your blog post, it should build stage0-std and stage0-rustc, which should be enough to test some basic changes right? So why not just have contributors do that?

In fact I've always been a bit confused on the rust compilation stages, and why stage 0 involves building anything at all. Typically "stage 0" should just mean the bootstrap compiler, and "stage 1" is where the first build happens, and "stage 2" is where the second build happens. There is normally no need for a third compilation, which apparently is what --stage 2 actually means in the rustc build process (unless you want to check that the 2nd vs 3rd builds are identical, for reproducible builds, which is not happening currently).

@infinity0
Copy link
Contributor

So why not just have contributors do that?

Ah, I remember, stage0-rustc won't use stage0-std because it was not built by itself but by the bootstrap compiler, and the ABI could be different. OTOH since rustc uses std in its code, we need to build stage0-std first in order to build stage0-rustc.

But that suggests that this new behaviour of not building stage-N rustc artifacts should be the case for every stage N, there is no reason to special-case stage 1.

@jyn514
Copy link
Member Author

jyn514 commented Sep 5, 2020

But that suggests that this new behaviour of not building stage-N rustc artifacts should be the case for every stage N, there is no reason to special-case stage 1.

Correct, that's the case.

In fact I've always been a bit confused on the rust compilation stages, and why stage 0 involves building anything at all.

See also rust-lang/rustc-dev-guide#843, I'd be interested in your opinion on that.

@jyn514
Copy link
Member Author

jyn514 commented Sep 5, 2020

There is normally no need for a third compilation, which apparently is what --stage 2 actually means in the rustc build process (unless you want to check that the 2nd vs 3rd builds are identical, for reproducible builds, which is not happening currently).

Right, this is why src/rustc is off by default. I think the release builders might do this for rustup artifacts? But --stage 2 didn't compile stage 2 rustc artifacts even before the changes to defaults, it was only stage0 and stage1 that were doing extra work AFAIK.

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Sep 16, 2020
…acrum

Make the default stage for x.py configurable

This also allows configuring each sub-command individually.

Possibly rust-lang#76617 should land before this? I don't feel strongly either way, I don't mind waiting.

Closes rust-lang#76165.
r? @Mark-Simulacrum
@bors bors closed this as completed in 1e11660 Sep 16, 2020
jyn514 added a commit to jyn514/rust that referenced this issue Sep 16, 2020
Previously, `x.py` would unconditionally run `x.py build` to get the
help message. After rust-lang#76165,
when checking the CI stage was moved into `Config`, that would cause an
assertion failure (but only only in CI!):

```
thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `1`,
 right: `2`', src/bootstrap/config.rs:619:49
```

This changes bootstrap to only generate a help message when it needs
to (when someone passes `--help`).
jyn514 added a commit to jyn514/rust that referenced this issue Sep 16, 2020
Previously, `x.py` would unconditionally run `x.py build` to get the
help message. After rust-lang#76165,
when checking the CI stage was moved into `Config`, that would cause an
assertion failure (but only only in CI!):

```
thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `1`,
 right: `2`', src/bootstrap/config.rs:619:49
```

This changes bootstrap to only generate a help message when it needs
to (when someone passes `--help`).
RalfJung added a commit to RalfJung/rust that referenced this issue Sep 19, 2020
Don't generate bootstrap usage unless it's needed

Previously, `x.py` would unconditionally run `x.py build` to get the
help message. After rust-lang#76165,
when checking the CI stage was moved into `Config`, that would cause an
assertion failure (but only only in CI!):

```
thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `1`,
 right: `2`', src/bootstrap/config.rs:619:49
```

This changes bootstrap to only generate a help message when it needs
to (when someone passes `--help`).

r? @Mark-Simulacrum
This should fix the CI failures in rust-lang#76797 and rust-lang#75991.
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Sep 19, 2020
Don't generate bootstrap usage unless it's needed

Previously, `x.py` would unconditionally run `x.py build` to get the
help message. After rust-lang#76165,
when checking the CI stage was moved into `Config`, that would cause an
assertion failure (but only only in CI!):

```
thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `1`,
 right: `2`', src/bootstrap/config.rs:619:49
```

This changes bootstrap to only generate a help message when it needs
to (when someone passes `--help`).

r? @Mark-Simulacrum
This should fix the CI failures in rust-lang#76797 and rust-lang#75991.
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Sep 19, 2020
Don't generate bootstrap usage unless it's needed

Previously, `x.py` would unconditionally run `x.py build` to get the
help message. After rust-lang#76165,
when checking the CI stage was moved into `Config`, that would cause an
assertion failure (but only only in CI!):

```
thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `1`,
 right: `2`', src/bootstrap/config.rs:619:49
```

This changes bootstrap to only generate a help message when it needs
to (when someone passes `--help`).

r? @Mark-Simulacrum
This should fix the CI failures in rust-lang#76797 and rust-lang#75991.
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Sep 19, 2020
Don't generate bootstrap usage unless it's needed

Previously, `x.py` would unconditionally run `x.py build` to get the
help message. After rust-lang#76165,
when checking the CI stage was moved into `Config`, that would cause an
assertion failure (but only only in CI!):

```
thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `1`,
 right: `2`', src/bootstrap/config.rs:619:49
```

This changes bootstrap to only generate a help message when it needs
to (when someone passes `--help`).

r? @Mark-Simulacrum
This should fix the CI failures in rust-lang#76797 and rust-lang#75991.
RalfJung added a commit to RalfJung/rust that referenced this issue Sep 20, 2020
Don't generate bootstrap usage unless it's needed

Previously, `x.py` would unconditionally run `x.py build` to get the
help message. After rust-lang#76165,
when checking the CI stage was moved into `Config`, that would cause an
assertion failure (but only only in CI!):

```
thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `1`,
 right: `2`', src/bootstrap/config.rs:619:49
```

This changes bootstrap to only generate a help message when it needs
to (when someone passes `--help`).

r? @Mark-Simulacrum
This should fix the CI failures in rust-lang#76797 and rust-lang#75991.
RalfJung added a commit to RalfJung/rust that referenced this issue Sep 20, 2020
Don't generate bootstrap usage unless it's needed

Previously, `x.py` would unconditionally run `x.py build` to get the
help message. After rust-lang#76165,
when checking the CI stage was moved into `Config`, that would cause an
assertion failure (but only only in CI!):

```
thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `1`,
 right: `2`', src/bootstrap/config.rs:619:49
```

This changes bootstrap to only generate a help message when it needs
to (when someone passes `--help`).

r? @Mark-Simulacrum
This should fix the CI failures in rust-lang#76797 and rust-lang#75991.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-feature-request Category: A feature request, i.e: not implemented / a PR. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants