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

Mock environment testing for bootstrap #102563

Open
tmandry opened this issue Oct 2, 2022 · 7 comments
Open

Mock environment testing for bootstrap #102563

tmandry opened this issue Oct 2, 2022 · 7 comments
Assignees
Labels
A-technical-debt Area: Internal cleanup work A-testsuite Area: The testsuite used to check the correctness of rustc C-enhancement Category: An issue proposing an enhancement or a PR with one. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)

Comments

@tmandry
Copy link
Member

tmandry commented Oct 2, 2022

There are lots of config knobs in bootstrap, and it is hard to understand how they interact. Consider two recent examples off the top of my head:

We should really catch these issues in tests, not review, and avoid relying too much on specific reviewers who know parts of the code.

Since bootstrap does I/O and interacts with external tooling, it is inherently difficult to test. The strategy I know involves mocking out the build environment (enough to make tests run in <~1s). From there you can record what the implementation does and take two approaches for the test:

  • Assert on specific conditions, like "the flag -D FOO was passed to LLVM cmake" or "foo/bar/baz was moved to foo/bar/quux".
  • Record everything in a golden file for some "representative" set of configurations. You can update these with --bless. This helps guard against unexpected behavior changes.

I think both are helpful. In fact, combining the two is exactly what we do in ui tests.

cc @jyn514 @Mark-Simulacrum

@tmandry tmandry added A-testsuite Area: The testsuite used to check the correctness of rustc C-enhancement Category: An issue proposing an enhancement or a PR with one. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) A-technical-debt Area: Internal cleanup work labels Oct 2, 2022
@tmandry
Copy link
Member Author

tmandry commented Oct 2, 2022

Implementation notes: Mocking can be implemented with traits (hopefully trait objects to avoid having so many generics). But for any "reading" from the environment you must specify some mock data that can be used in tests. For example, when reading from a file you must provide mock file contents that are used in tests.

Tip: Virtualizing the filesystem itself is not actually necessary. If it's easier, for example, we could run in a sandbox build directory set up for that test. For each external command we'd specify the expected output files with mock outputs and actually create them on the filesystem, then allow directly manipulating them (move/copy/read/etc.) within bootstrap in the test sandbox. I could see this plugging in nicely to the existing traits we have.

@Mark-Simulacrum
Copy link
Member

We already try to test some of this via --dry-run which intends to run really quickly (it actually runs on every invocation before starting the real build, and we verify that the Steps executed are the same between both IIRC).

I'm not sure how much extra is needed beyond that -- certainly it supports configuration. I think in order to test things like passing specific flags, we'd want to be dumping/preserving more state than we do today, but that doesn't seem particularly hard (just needs some plumbing to keep vectors of commands around or something).

@jyn514
Copy link
Member

jyn514 commented Feb 2, 2023

I'm not sure how much extra is needed beyond that -- certainly it supports configuration. I think in order to test things like passing specific flags, we'd want to be dumping/preserving more state than we do today, but that doesn't seem particularly hard (just needs some plumbing to keep vectors of commands around or something).

I don't think it's that simple - if nothing else, output doesn't support dry_run today, so anything using output is special-casing dry_run and we can't see any command it would normally try to run. and in general I think there are lots of blocks that are wholly skipped when dry_run is enabled because it was simpler.

@jyn514
Copy link
Member

jyn514 commented Feb 2, 2023

; rg 'if .*dry_run\(' src/bootstrap | wc -l
      68

🙃

@jyn514
Copy link
Member

jyn514 commented Mar 17, 2023

@oli-obk asked me recently "why is this hard?" and I want to record those answers somewhere. A non-exhaustive things I'd like to test:

  1. Detecting src and out: add bootstrap tests for detecting src and out #109120
    • Including when run from a different machine
    • Including when the current working directory isn't inside the source directory
      • Including when CWD is a subdirectory of another unrelated git repo
    • Including when CWD is a subdirectory of the source directory, or inside out
  2. "What if src is read-only?"
  3. A laundry list of things to do with the current git state
    • The rust-lang/rust remote is called origin
    • the remote is called something that's not origin
    • the remote doesn't exist, there's only a remote that points to a fork
    • should we download llvm?
      • including if this commit modified LLVM
      • including if this commit modified LLVM and is also running in CI
      • do bootstrap's self tests still pass in all those cases (yes this was a real bug!)
    • can we figure out where to download llvm/rustc?
      • including if this is stable/beta
      • including if llvm assertions are enabled
        • including if this is a tier 2 target that only has the version without assertions
    • can we figure out wtf happened in Add tidy check to deny merge commits #105058 such that it passed a bors merge but failed all subsequent commits?
  4. Can we test the NixOS support? Right now we rely on people fixing it themselves after the fact.
  5. The bootstrap invocation itself
    • python2 vs python3. we did have a test for that at one point but it regressed in use problem matchers for tidy CI #106085 (I guess because it wasn't clear that it was testing bootstrap itself in addition to tidy).
    • the shell scripts
    • the src/tools/x wrapper
    • the rust binary without the python wrapper
  6. As a stretch goal, can we test "taking the output of one bootstrap invocation and using it as the stage0 compiler for another invocation"?

@onur-ozkan
Copy link
Member

Can we test the NixOS support? Right now we rely on people fixing it themselves after the fact.

Also same for multiarch linux distros like Debian and Ubuntu

@jyn514
Copy link
Member

jyn514 commented Apr 16, 2023

oh yeah I'd love to test "do external tools using rustc_private reliably find the sysroot without hacks"; and all of these things but with download-rustc enabled.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue May 15, 2023
Add clubby789 to the bootstrap review rotation

r? `@clubby789` - thank you for volunteering!

I have been meaning for a very long time now to write up how to do reviews, but I haven't gotten around to it yet :( here is a short summary:

1. If you're not sure what the changes does or if it's ok, always feel free to ping someone else on the team, especially in the first few weeks. You can use `r? bootstrap` to get triagebot to assign someone else.
2. Bootstrap unfortunately has very few tests. Things that touch CLI or toml parsing should likely have a test in `src/bootstrap/config/tests.rs`; things that touch "core" build logic should have a test in `builder/tests.rs`, anything else kinda just slips in :( see rust-lang#102563 for ideas on how to improve the situation here.
3. "Major" changes should be documented in `src/bootstrap/CHANGELOG.md`. "Major" is up to you, but if it breaks a config option or otherwise is likely to break *someone's* build, it's probably major. If it breaks nearly *everyone*'s build, it should also update `VERSION` in `lib.rs`; this should be very rare. Please also ping me or Mark-Simulacrum for major changes (I might set up a triagebot ping for this so you don't have to remember).
4. Once you've approved the PR, tell bors it's ok - you've been contributing for a while so you know how bors works, but here's a cheatsheet just in case: https://bors.rust-lang.org

Documentation about how to use bootstrap lives at https://rustc-dev-guide.rust-lang.org/building/bootstrapping.html; internal docs live in `src/bootstrap/README.md`. The latter unfortunately is not very complete.
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue May 16, 2023
Add clubby789 to the bootstrap review rotation

r? ``@clubby789`` - thank you for volunteering!

I have been meaning for a very long time now to write up how to do reviews, but I haven't gotten around to it yet :( here is a short summary:

1. If you're not sure what the changes does or if it's ok, always feel free to ping someone else on the team, especially in the first few weeks. You can use `r? bootstrap` to get triagebot to assign someone else.
2. Bootstrap unfortunately has very few tests. Things that touch CLI or toml parsing should likely have a test in `src/bootstrap/config/tests.rs`; things that touch "core" build logic should have a test in `builder/tests.rs`, anything else kinda just slips in :( see rust-lang#102563 for ideas on how to improve the situation here.
3. "Major" changes should be documented in `src/bootstrap/CHANGELOG.md`. "Major" is up to you, but if it breaks a config option or otherwise is likely to break *someone's* build, it's probably major. If it breaks nearly *everyone*'s build, it should also update `VERSION` in `lib.rs`; this should be very rare. Please also ping me or Mark-Simulacrum for major changes (I might set up a triagebot ping for this so you don't have to remember).
4. Once you've approved the PR, tell bors it's ok - you've been contributing for a while so you know how bors works, but here's a cheatsheet just in case: https://bors.rust-lang.org

Documentation about how to use bootstrap lives at https://rustc-dev-guide.rust-lang.org/building/bootstrapping.html; internal docs live in `src/bootstrap/README.md`. The latter unfortunately is not very complete.
Noratrieb added a commit to Noratrieb/rust that referenced this issue May 16, 2023
Add clubby789 to the bootstrap review rotation

r? ```@clubby789``` - thank you for volunteering!

I have been meaning for a very long time now to write up how to do reviews, but I haven't gotten around to it yet :( here is a short summary:

1. If you're not sure what the changes does or if it's ok, always feel free to ping someone else on the team, especially in the first few weeks. You can use `r? bootstrap` to get triagebot to assign someone else.
2. Bootstrap unfortunately has very few tests. Things that touch CLI or toml parsing should likely have a test in `src/bootstrap/config/tests.rs`; things that touch "core" build logic should have a test in `builder/tests.rs`, anything else kinda just slips in :( see rust-lang#102563 for ideas on how to improve the situation here.
3. "Major" changes should be documented in `src/bootstrap/CHANGELOG.md`. "Major" is up to you, but if it breaks a config option or otherwise is likely to break *someone's* build, it's probably major. If it breaks nearly *everyone*'s build, it should also update `VERSION` in `lib.rs`; this should be very rare. Please also ping me or Mark-Simulacrum for major changes (I might set up a triagebot ping for this so you don't have to remember).
4. Once you've approved the PR, tell bors it's ok - you've been contributing for a while so you know how bors works, but here's a cheatsheet just in case: https://bors.rust-lang.org

Documentation about how to use bootstrap lives at https://rustc-dev-guide.rust-lang.org/building/bootstrapping.html; internal docs live in `src/bootstrap/README.md`. The latter unfortunately is not very complete.
Noratrieb added a commit to Noratrieb/rust that referenced this issue May 16, 2023
Add clubby789 to the bootstrap review rotation

r? `````@clubby789````` - thank you for volunteering!

I have been meaning for a very long time now to write up how to do reviews, but I haven't gotten around to it yet :( here is a short summary:

1. If you're not sure what the changes does or if it's ok, always feel free to ping someone else on the team, especially in the first few weeks. You can use `r? bootstrap` to get triagebot to assign someone else.
2. Bootstrap unfortunately has very few tests. Things that touch CLI or toml parsing should likely have a test in `src/bootstrap/config/tests.rs`; things that touch "core" build logic should have a test in `builder/tests.rs`, anything else kinda just slips in :( see rust-lang#102563 for ideas on how to improve the situation here.
3. "Major" changes should be documented in `src/bootstrap/CHANGELOG.md`. "Major" is up to you, but if it breaks a config option or otherwise is likely to break *someone's* build, it's probably major. If it breaks nearly *everyone*'s build, it should also update `VERSION` in `lib.rs`; this should be very rare. Please also ping me or Mark-Simulacrum for major changes (I might set up a triagebot ping for this so you don't have to remember).
4. Once you've approved the PR, tell bors it's ok - you've been contributing for a while so you know how bors works, but here's a cheatsheet just in case: https://bors.rust-lang.org

Documentation about how to use bootstrap lives at https://rustc-dev-guide.rust-lang.org/building/bootstrapping.html; internal docs live in `src/bootstrap/README.md`. The latter unfortunately is not very complete.
Noratrieb added a commit to Noratrieb/rust that referenced this issue May 16, 2023
Add clubby789 to the bootstrap review rotation

r? ````@clubby789```` - thank you for volunteering!

I have been meaning for a very long time now to write up how to do reviews, but I haven't gotten around to it yet :( here is a short summary:

1. If you're not sure what the changes does or if it's ok, always feel free to ping someone else on the team, especially in the first few weeks. You can use `r? bootstrap` to get triagebot to assign someone else.
2. Bootstrap unfortunately has very few tests. Things that touch CLI or toml parsing should likely have a test in `src/bootstrap/config/tests.rs`; things that touch "core" build logic should have a test in `builder/tests.rs`, anything else kinda just slips in :( see rust-lang#102563 for ideas on how to improve the situation here.
3. "Major" changes should be documented in `src/bootstrap/CHANGELOG.md`. "Major" is up to you, but if it breaks a config option or otherwise is likely to break *someone's* build, it's probably major. If it breaks nearly *everyone*'s build, it should also update `VERSION` in `lib.rs`; this should be very rare. Please also ping me or Mark-Simulacrum for major changes (I might set up a triagebot ping for this so you don't have to remember).
4. Once you've approved the PR, tell bors it's ok - you've been contributing for a while so you know how bors works, but here's a cheatsheet just in case: https://bors.rust-lang.org

Documentation about how to use bootstrap lives at https://rustc-dev-guide.rust-lang.org/building/bootstrapping.html; internal docs live in `src/bootstrap/README.md`. The latter unfortunately is not very complete.
RalfJung pushed a commit to RalfJung/miri that referenced this issue May 18, 2023
Add clubby789 to the bootstrap review rotation

r? `````@clubby789````` - thank you for volunteering!

I have been meaning for a very long time now to write up how to do reviews, but I haven't gotten around to it yet :( here is a short summary:

1. If you're not sure what the changes does or if it's ok, always feel free to ping someone else on the team, especially in the first few weeks. You can use `r? bootstrap` to get triagebot to assign someone else.
2. Bootstrap unfortunately has very few tests. Things that touch CLI or toml parsing should likely have a test in `src/bootstrap/config/tests.rs`; things that touch "core" build logic should have a test in `builder/tests.rs`, anything else kinda just slips in :( see rust-lang/rust#102563 for ideas on how to improve the situation here.
3. "Major" changes should be documented in `src/bootstrap/CHANGELOG.md`. "Major" is up to you, but if it breaks a config option or otherwise is likely to break *someone's* build, it's probably major. If it breaks nearly *everyone*'s build, it should also update `VERSION` in `lib.rs`; this should be very rare. Please also ping me or Mark-Simulacrum for major changes (I might set up a triagebot ping for this so you don't have to remember).
4. Once you've approved the PR, tell bors it's ok - you've been contributing for a while so you know how bors works, but here's a cheatsheet just in case: https://bors.rust-lang.org

Documentation about how to use bootstrap lives at https://rustc-dev-guide.rust-lang.org/building/bootstrapping.html; internal docs live in `src/bootstrap/README.md`. The latter unfortunately is not very complete.
@onur-ozkan onur-ozkan self-assigned this Sep 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-technical-debt Area: Internal cleanup work A-testsuite Area: The testsuite used to check the correctness of rustc C-enhancement Category: An issue proposing an enhancement or a PR with one. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

No branches or pull requests

4 participants