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

Support downloading bootstrap from CI in shell scripts #102282

Closed
wants to merge 4 commits into from

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Sep 25, 2022

  • Download the beta compiler toolchain in bootstrap if it doesn't yet exist

    This is needed for when the shell scripts bypass python altogether and run the downloaded bootstrap directly

  • Add a new bootstrap-shim program and distribute it on nightly

  • Support downloading bootstrap from CI in the shell script entrypoints

    This completely bypasses the python scripts when bootstrap hasn't been modified.

I think we're going to need a way to opt-out of this, probably a new option in config.toml.

Helps with #94829.

r? @Mark-Simulacrum

@jyn514 jyn514 added the S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. label Sep 25, 2022
@rustbot rustbot added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Sep 25, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 25, 2022
@jyn514 jyn514 force-pushed the no-python-in-shell-scripts branch 2 times, most recently from 0c8350d to 366399a Compare September 25, 2022 22:56
@rust-log-analyzer

This comment has been minimized.

@jyn514
Copy link
Member Author

jyn514 commented Oct 1, 2022

huh, that worked quicker than I expected

@jyn514 jyn514 changed the title [wip] Support downloading bootstrap from CI in shell scripts Support downloading bootstrap from CI in shell scripts Oct 1, 2022
@jyn514 jyn514 removed the S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. label Oct 1, 2022
@jyn514 jyn514 assigned Mark-Simulacrum and unassigned jyn514 Oct 1, 2022
@Mark-Simulacrum
Copy link
Member

Mark-Simulacrum commented Oct 1, 2022

I'm feeling like maybe we really should have a shim that manages much of the complexity here.

I feel like my envisioning is that our shell scripts should be:

  • check if rustup is installed, if so, run rustup component add bootstrap-shim, and execute that (rustup which rustc-bootstrap-shim, run that). That shim can be from any version of Rust (obviously to start just nightly but the shim should be pretty simple).
  • the shim will take care of the git detection etc and can be compiled with a clap dependency so we can easily support --build-dir and all that stuff. I don't think there's a problem with us parsing config.toml, though it does lock down the compatibility of a subset of fields (we'll probably want to parse into dedicated shim structs that are a subset of the ones in bootstrap proper).

I think a reasonable alternative is that we expect a working Rust installation (not necessarily through rustup) and use that to compile a cargo-less src/bootstrap/entrypoint.rs that can be a shared executable and frankly is much nicer than shell or powershell (at least to my eyes), even without dependencies.

I am feeling like we ought to have a living document which we put our plan into and link from all of these PRs, because it seems to me that we are constantly re-debating these points and given my limited time I definitely am reinventing wheels since I can't keep all the context in my head.

@pietroalbini
Copy link
Member

Commit authors other than bors@rust-lang.org are not supported. Fixing this is possible, but will require jq to be installed, which I don't want to require yet. I think doing feature detection (command -v jq) shouldn't be too bad, but if @pietroalbini doesn't plan to use the shell scripts, I don't think it really matters.

This would indeed be pretty annoying :(

A solution that doesn't involve jq would be to move the configuration out of stage0.json into a plaintext file with an easy enough syntax to parse with bash, something like:

dist_server https://static.rust-lang.org
artifacts_server https://ci-artifacts.rust-lang.org/rustc-builds
artifacts_with_llvm_assertions_server https://ci-artifacts.rust-lang.org/rustc-builds-alt
git_merge_commit_email bors@rust-lang.org
nightly_branch master

@bors
Copy link
Contributor

bors commented Oct 3, 2022

☔ The latest upstream changes (presumably #100557) made this pull request unmergeable. Please resolve the merge conflicts.

x Outdated
fi

latest_commit=$(git rev-list HEAD --author=bors@rust-lang.org --max-count 1)
if git diff-index --quiet $latest_commit -- src/bootstrap; then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be conditional on a config.toml config? A distro probably doesn't want to trigger this logic even if according to git they didn't change bootstrap.

@jyn514
Copy link
Member Author

jyn514 commented Oct 5, 2022

I am feeling like we ought to have a living document which we put our plan into and link from all of these PRs, because it seems to me that we are constantly re-debating these points and given my limited time I definitely am reinventing wheels since I can't keep all the context in my head.

Sorry I keep changing the plan here. I've written up a short document with your bootstrap-shim suggestion: https://hackmd.io/@jynelson/S1Aa4cozo/edit. I'm going to implement those steps here.

If it helps, I think this should be the last PR where we ever have to rehash these details :) after this, the only steps left are to support downloadable bootstrap in python and update the documentation.

I think a reasonable alternative is that we expect a working Rust installation (not necessarily through rustup) and use that to compile a cargo-less src/bootstrap/entrypoint.rs that can be a shared executable and frankly is much nicer than shell or powershell (at least to my eyes), even without dependencies.

Can you talk about this some more? This is the first time I've heard the suggestion - I think it would be somewhat limiting since it would mean we can't use serde_toml to parse the config file. I'm also unsure what running it would look like? I guess we could have a cargo alias, but people seemed reasonably happy with the shell scripts, and now that they're going to stay quite small, I'm not sure what benefit we'd get.

@jyn514
Copy link
Member Author

jyn514 commented Oct 30, 2022

I realized that sharing the code between the shim and the main binary without compiling it twice will be annoying; I've updated the hackmd (https://hackmd.io/j-XXBeYERuajJknd6zErPA) with a plan for doing that.

@bjorn3
Copy link
Member

bjorn3 commented Nov 4, 2022

Did you see the comments I left on that hackmd?

@jyn514
Copy link
Member Author

jyn514 commented Nov 4, 2022

I didn't, thank you! Left a few responses :)

@Mark-Simulacrum
Copy link
Member

Left a comment on the HackMD -- but probably the plan seems broadly good.

@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 Nov 6, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 14, 2022
…Simulacrum

Make all download functions need only Config, not Builder

This also adds a new `mod download` instead of scattering the download code across `config.rs` and `native.rs`.

This is the simplest and also most bit-rotty part of rust-lang#102282. Opening it earlier so it's not mixed in with behavior changes and to avoid rebase hell.

cc rust-lang#94829 (which nows has the hackmd linked).

r? `@Mark-Simulacrum`
@jyn514 jyn514 force-pushed the no-python-in-shell-scripts branch 2 times, most recently from 9929975 to 6770406 Compare November 17, 2022 05:51
@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Jan 28, 2023
@jyn514
Copy link
Member Author

jyn514 commented Jan 28, 2023

@bors try

(there was a bug in the previous code that the latest commit fixes, the previous try build won't have bootstrap-shim)

@bors
Copy link
Contributor

bors commented Jan 28, 2023

⌛ Trying commit 443bbf085617bcbe313698940066e9d2d9622004 with merge 18b165dc185211a6c919bf0459866c88c35a6ca0...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Jan 30, 2023

☔ The latest upstream changes (presumably #103019) made this pull request unmergeable. Please resolve the merge conflicts.

@onur-ozkan
Copy link
Member

onur-ozkan commented Jan 30, 2023

Here is the commitment for MinimalConfig::parse and Config::parse unification onur-ozkan@4fb924f
(also fixes #102282 (comment))

How do you want to handle this PR? @jyn514 You can cherry-pick that commit if you want(I guess this looks better way to deal with), or I can open another PR.

@jyn514
Copy link
Member Author

jyn514 commented Jan 30, 2023

@ozkanonur I'm happy for you to take over this PR :) I think I would prefer that to a new PR so the previous discussion is easy to find.

@onur-ozkan
Copy link
Member

Could you please squash your commist in this PR? 🙂 I will reset my git history with your commit again (so we will have 2 commits(1 for your work, 1 for my) for the PR rather than 9 commits)

jyn514 and others added 2 commits February 1, 2023 21:47
This is needed for when the shell scripts bypass python altogether and run the downloaded bootstrap directly
- Pass `dist bootstrap-shim` explicitly in CI

 This makes it possible to run `dist bootstrap` without also building
 bootstrap-shim, and more importantly works around a bug where currently
 two steps aren't allowed to have the same path.

- Add `check::BootstrapShim`
- [wip] start unifying parsing for Config and MinimalConfig
@jyn514
Copy link
Member Author

jyn514 commented Feb 2, 2023

@ozkanonur done :) I left the first commit separate because it will also be needed if the python script starts downloading bootstrap, and I want to be able to extract it out into a different PR if necessary.

@jyn514
Copy link
Member Author

jyn514 commented Feb 2, 2023

Ok, some more thoughts on the current state:

Long-term plan

I am not sure if downloading bootstrap-shim from CI is the best approach. It seems silly to download two binaries one after the other, and not too hard to run cargo run --bin bootstrap-shim in the shell scripts. Compiling also avoids having to add bootstrap-shim to the rustup manifest, or needing to figure out exactly how old the shim can be before we force an update to a new version). Right now it doesn't actually save any time, because the shim depends on all of bootstrap's dependencies, but I think we can cut down the shim's dependencies to essentially 0 in a follow-up.

@Mark-Simulacrum do you have thoughts on the right approach here? I know this isn't the plan in the hackmd :( ... I can update it there if that's easier for you. Another thing we can do to avoid this PR bitrotting further is to merge this PR without changes to either the shell-scripts or CI, and have people use cargo run --bin bootstrap-shim manually to test out this workflow; that lets us defer the decision of exactly how to install it for later.

Bugs

  • Right now bootstrap-shim doesn't accept any arguments (oops):
; cargo r --bin bootstrap-shim --manifest-path src/bootstrap/Cargo.toml -- -v
    Finished dev [unoptimized] target(s) in 0.04s
     Running `/Users/jyn/.local/lib/cargo/target/debug/bootstrap-shim -v`
thread 'main' panicked at 'opts.parse(args) failed with Unrecognized option: 'v'', bin/bootstrap-shim.rs:12:19
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

This is non-trivial to fix with the current arg-parsing setup: either we'd have to share flags.rs between bootstrap and bootstrap-shim (which I'd rather not do to cut down on compile times), or we'd have to switch to a different arg-parsing library.

Long-term bugs

These don't have to be fixed right away, but I think we should fix them before turning this on by default.

  • The shim depends on all of bootstrap's dependencies. I think this is fixable, just really annoying because it means separating out "things bootstrap uses" from "things both bootstrap and bootstrap-shim use".
  • We should add a download-bootstrap # bool option to config.toml to tell bootstrap-shim to fall back to using python instead.

@jyn514 jyn514 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 2, 2023
Copy link
Member Author

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ozkanonur I see you made a partial start on unifying the parsing, but there's still some more logic I'd like to avoid duplicating - in particular, any option on MinimalConfig should only be set in min_config.rs, not config.rs. I see all the flags still being set in config.rs.

@jyn514 jyn514 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 Feb 2, 2023
@bors
Copy link
Contributor

bors commented Feb 16, 2023

☔ The latest upstream changes (presumably #108096) made this pull request unmergeable. Please resolve the merge conflicts.

@jyn514
Copy link
Member Author

jyn514 commented Feb 17, 2023

Closing in favor of #107812

@jyn514 jyn514 closed this Feb 17, 2023
@jyn514 jyn514 deleted the no-python-in-shell-scripts branch February 17, 2023 21:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants