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

Use the beta compiler for building bootstrap tools when download-rustc is set #82739

Merged
merged 1 commit into from
Apr 5, 2021

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Mar 4, 2021

Motivation

This avoids having to rebuild bootstrap and tidy each time you rebase
over master. In particular, it makes rebasing and running x.py fmt on
each commit in a branch significantly faster. It also avoids having to
rebuild bootstrap after setting download-rustc = true.

Implementation

Instead of extracting the CI artifacts directly to stage0/, extract
them to ci-rustc/ instead. Continue to copy them to the proper
sysroots as necessary for all stages except stage 0.

This also requires bootstrap.py to download both stage0 and CI
artifacts and distinguish between the two when checking stamp files.

Note that since tools have to be built by the same compiler that built
rustc-dev and the standard library, the downloaded artifacts can't be
reused when building with the beta compiler. To make sure this is still
a good user experience, warn when building with the beta compiler, and
default to building with stage 2.

I tested this by rebasing this PR from edeee91 over 1c77a1f and confirming that only the bootstrap library itself had to be rebuilt, not any dependencies and not tidy. I also tested that a clean build with x.py build builds rustdoc exactly once and does no other work, and that touch src/librustdoc/lib.rs && x.py build works. x.py check still behaves as before (checks using the beta compiler, even if there are changes to compiler/).

Helps with #81930.

r? @Mark-Simulacrum

@jyn514 jyn514 added T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) A-contributor-roadblock Area: Makes things more difficult for new contributors to rust itself labels Mar 4, 2021
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 4, 2021
@jyn514 jyn514 mentioned this pull request Mar 4, 2021
13 tasks
@rust-log-analyzer

This comment has been minimized.

builder.ensure(compile::Std { compiler: build_compiler, target: target_compiler.host });
builder.ensure(compile::Rustc { compiler: build_compiler, target: target_compiler.host });
// NOTE: this implies that `download-rustc` is pretty useless when compiling with the stage0
// compiler, since you do just as much work.
Copy link
Member

Choose a reason for hiding this comment

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

There's not really many reasons to use the beta compiler though, right? Like, the only case might be if you're debugging some cfg(bootstrap) related thing, but that should be quite rare.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, in practice I don't think anyone will need to use this.

@rust-log-analyzer

This comment has been minimized.

@@ -514,6 +514,19 @@ impl Step for Rustdoc {
// rustc compiler it's paired with, so it must be built with the previous stage compiler.
let build_compiler = builder.compiler(target_compiler.stage - 1, builder.config.build);

// When using `download-rustc` and a stage0 build_compiler, copying rustc doesn't actually
// build stage0 libstd (because the libstd in sysroot has the wrong ABI). Explicitly build
// it.
Copy link
Member

Choose a reason for hiding this comment

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

Hm, this seems suspicious - it feels like we should probably fix assemble or w/e to build it? Why are we explicitly building it here in impl step for rustdoc rather than fixing the origin of the problem (where we skip the build)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, in theory I guess you're right, but if building the stage1 sysroot required building the compiler each time it defeats the whole point of download-rustc, since building stage2 rustdoc requires having a stage1 sysroot.

This does have the downside that each tool has to add its own ensure() call, but I don't know if that's a problem in practice - like I said below, I don't think there's much use in compiling with beta instead of with master.

@jyn514
Copy link
Member Author

jyn514 commented Mar 4, 2021

tidy error: /checkout/src/bootstrap/bootstrap.py:549: TODO is deprecated; use FIXME

    # TODO: make stage0 argument required?
    def fix_bin_or_dylib(self, fname, rpath_libz=False, stage0=True):

I'd be interested to hear your opinion on this - the only thing stage0 is used for here is for the nix root:

        # Only build `stage0/.nix-deps` once.
        nix_deps_dir = self.nix_deps_dir
        if not nix_deps_dir:
            nix_deps_dir = "{}/.nix-deps".format(self.bin_root(stage0))
            if not os.path.exists(nix_deps_dir):
                os.makedirs(nix_deps_dir)
            # ... install nix dependencies ...

Should that be cached across different toolchains? Should I use the "stage0" directory unconditionally even for CI artifacts?

@crlf0710 crlf0710 added the I-needs-decision Issue: In need of a decision. label Mar 20, 2021
@jyn514
Copy link
Member Author

jyn514 commented Mar 21, 2021

@crlf0710 this is waiting on review from @Mark-Simulacrum, not a decision of whether it's a good idea or not.

@jyn514 jyn514 added T-dev-tools Relevant to the dev-tools subteam, which will review and decide on the PR/issue. and removed I-needs-decision Issue: In need of a decision. labels Mar 21, 2021
@jyn514
Copy link
Member Author

jyn514 commented Mar 22, 2021

Should that be cached across different toolchains? Should I use the "stage0" directory unconditionally even for CI artifacts?

I changed this to use the same nix root for all stages.

@jyn514
Copy link
Member Author

jyn514 commented Apr 4, 2021

ping @Mark-Simulacrum - do you know when you'll get a chance to look at this?

@Mark-Simulacrum
Copy link
Member

It's my hope to get to it today/tomorrow.

@Mark-Simulacrum
Copy link
Member

I don't know much about Nix and that code -- my memory vaguely wants to say @nagisa has helped out there in the past perhaps?

Overall, this seems good to me, I would like to make sure the choice wrt to the nix root you made is correct though. May be good to add some comments if we get additional clarity.

@nagisa
Copy link
Member

nagisa commented Apr 4, 2021

It is fine, I think, to generate .nix-deps just once, but if its going to be used for things other than just stage0, then perhaps it should be outside of the stage0 directory (i.e. at the build root)?

I'm not in a position to verify the changes in relation to nix, but I'm sure that if something does break people will fix it as a followup – nix specific support should be considered a best-effort kind of thing, not required.

@Mark-Simulacrum
Copy link
Member

Makes sense. I agree that it's a best-effort/nice to have for sure.

r=me with the nix-deps dir moved to the root (out of stage0), likely just build/ perhaps? Or build/$host, not sure - I don't recall what is in it precisely or whether it's tied to the build triple or is entirely global.

…tc` is set

 ## Motivation

This avoids having to rebuild bootstrap and tidy each time you rebase
over master. In particular, it makes rebasing and running `x.py fmt` on
each commit in a branch significantly faster. It also avoids having to
rebuild bootstrap after setting `download-rustc = true`.

 ## Implementation

Instead of extracting the CI artifacts directly to `stage0/`, extract
them to `ci-rustc/` instead. Continue to copy them to the proper
sysroots as necessary for all stages except stage 0.

This also requires `bootstrap.py` to download both stage0 and CI
artifacts and distinguish between the two when checking stamp files.

Note that since tools have to be built by the same compiler that built
`rustc-dev` and the standard library, the downloaded artifacts can't be
reused when building with the beta compiler. To make sure this is still
a good user experience, warn when building with the beta compiler, and
default to building with stage 2.
@jyn514
Copy link
Member Author

jyn514 commented Apr 4, 2021

The whole situation with nix-deps is very confusing to me - it appears to be assuming that the current directory is already build/$host and goes from there? I removed stage0/ and it should be stored in build/$host/.nix-deps now: https://github.com/rust-lang/rust/compare/f86add4ce1d8d9d1714e868565317d947108f3fc..14406df189150a1a79298dd82007c6fd6186fafc

@bors r=Mark-Simulacrum

@bors
Copy link
Contributor

bors commented Apr 4, 2021

📌 Commit 14406df has been approved by Mark-Simulacrum

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 4, 2021
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Apr 5, 2021
…rk-Simulacrum

Use the beta compiler for building bootstrap tools when `download-rustc` is set

 ## Motivation

This avoids having to rebuild bootstrap and tidy each time you rebase
over master. In particular, it makes rebasing and running `x.py fmt` on
each commit in a branch significantly faster. It also avoids having to
rebuild bootstrap after setting `download-rustc = true`.

 ## Implementation

Instead of extracting the CI artifacts directly to `stage0/`, extract
them to `ci-rustc/` instead. Continue to copy them to the proper
sysroots as necessary for all stages except stage 0.

This also requires `bootstrap.py` to download both stage0 and CI
artifacts and distinguish between the two when checking stamp files.

Note that since tools have to be built by the same compiler that built
`rustc-dev` and the standard library, the downloaded artifacts can't be
reused when building with the beta compiler. To make sure this is still
a good user experience, warn when building with the beta compiler, and
default to building with stage 2.

I tested this by rebasing this PR from edeee91 over 1c77a1f and confirming that only the bootstrap library itself had to be rebuilt, not any dependencies and not `tidy`. I also tested that a clean build with `x.py build` builds rustdoc exactly once and does no other work, and that `touch src/librustdoc/lib.rs && x.py build` works. `x.py check` still behaves as before (checks using the beta compiler, even if there are changes to `compiler/`).

Helps with rust-lang#81930.

r? `@Mark-Simulacrum`
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Apr 5, 2021
…rk-Simulacrum

Use the beta compiler for building bootstrap tools when `download-rustc` is set

 ## Motivation

This avoids having to rebuild bootstrap and tidy each time you rebase
over master. In particular, it makes rebasing and running `x.py fmt` on
each commit in a branch significantly faster. It also avoids having to
rebuild bootstrap after setting `download-rustc = true`.

 ## Implementation

Instead of extracting the CI artifacts directly to `stage0/`, extract
them to `ci-rustc/` instead. Continue to copy them to the proper
sysroots as necessary for all stages except stage 0.

This also requires `bootstrap.py` to download both stage0 and CI
artifacts and distinguish between the two when checking stamp files.

Note that since tools have to be built by the same compiler that built
`rustc-dev` and the standard library, the downloaded artifacts can't be
reused when building with the beta compiler. To make sure this is still
a good user experience, warn when building with the beta compiler, and
default to building with stage 2.

I tested this by rebasing this PR from edeee91 over 1c77a1f and confirming that only the bootstrap library itself had to be rebuilt, not any dependencies and not `tidy`. I also tested that a clean build with `x.py build` builds rustdoc exactly once and does no other work, and that `touch src/librustdoc/lib.rs && x.py build` works. `x.py check` still behaves as before (checks using the beta compiler, even if there are changes to `compiler/`).

Helps with rust-lang#81930.

r? ``@Mark-Simulacrum``
@bors
Copy link
Contributor

bors commented Apr 5, 2021

⌛ Testing commit 14406df with merge fa79128186ab36f06c89c3845b8f4b1120988a48...

jyn514 pushed a commit to jyn514/rust that referenced this pull request Apr 5, 2021
…rk-Simulacrum

Use the beta compiler for building bootstrap tools when `download-rustc` is set

 ## Motivation

This avoids having to rebuild bootstrap and tidy each time you rebase
over master. In particular, it makes rebasing and running `x.py fmt` on
each commit in a branch significantly faster. It also avoids having to
rebuild bootstrap after setting `download-rustc = true`.

 ## Implementation

Instead of extracting the CI artifacts directly to `stage0/`, extract
them to `ci-rustc/` instead. Continue to copy them to the proper
sysroots as necessary for all stages except stage 0.

This also requires `bootstrap.py` to download both stage0 and CI
artifacts and distinguish between the two when checking stamp files.

Note that since tools have to be built by the same compiler that built
`rustc-dev` and the standard library, the downloaded artifacts can't be
reused when building with the beta compiler. To make sure this is still
a good user experience, warn when building with the beta compiler, and
default to building with stage 2.

I tested this by rebasing this PR from edeee91 over 1c77a1f and confirming that only the bootstrap library itself had to be rebuilt, not any dependencies and not `tidy`. I also tested that a clean build with `x.py build` builds rustdoc exactly once and does no other work, and that `touch src/librustdoc/lib.rs && x.py build` works. `x.py check` still behaves as before (checks using the beta compiler, even if there are changes to `compiler/`).

Helps with rust-lang#81930.

r? `@Mark-Simulacrum`
@jyn514
Copy link
Member Author

jyn514 commented Apr 5, 2021

@bors retry yield to #83876

@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 5, 2021
Rollup of 8 pull requests

Successful merges:

 - rust-lang#81922 (Let `#[allow(unstable_name_collisions)]` work for things other than function)
 - rust-lang#82483 (Use FromStr trait for number option parsing)
 - rust-lang#82739 (Use the beta compiler for building bootstrap tools when `download-rustc` is set)
 - rust-lang#83650 (Update Source Serif to release 4.004)
 - rust-lang#83826 (List trait impls before deref methods in doc's sidebar)
 - rust-lang#83831 (Add `#[inline]` to IpAddr methods)
 - rust-lang#83863 (Render destructured struct function param names as underscore)
 - rust-lang#83865 (Don't report disambiguator error if link would have been ignored)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit ca9cbea into rust-lang:master Apr 5, 2021
@rustbot rustbot added this to the 1.53.0 milestone Apr 5, 2021
@jyn514 jyn514 deleted the separate-stage0-stage1 branch April 5, 2021 13:49
@jyn514 jyn514 mentioned this pull request Apr 10, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 11, 2021
…ulacrum

Fix NixOS patching

Moving the `.nix-deps` has resulted in rpath links being broken and
therefore bootstrap on NixOS broken entirely.

This PR still produces a `.nix-deps` but only for the purposes of
producing a gc root. We rpath a symlink-resolved result instead.

For purposes of simplicity we also use joinSymlink to produce a single
merged output directory so that we don't need to update multiple
locations every time we add a library or something.

Fixes a regression from rust-lang#82739.
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request May 10, 2021
…acrum

Remove outdated FIXME for download-rustc

rust-lang#82739 has been merged.
JohnTitor added a commit to JohnTitor/rust that referenced this pull request May 11, 2021
…acrum

Remove outdated FIXME for download-rustc

rust-lang#82739 has been merged.
@jyn514 jyn514 added the A-download-rustc Area: The `rust.download-rustc` build option. label Jun 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-contributor-roadblock Area: Makes things more difficult for new contributors to rust itself A-download-rustc Area: The `rust.download-rustc` build option. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-dev-tools Relevant to the dev-tools subteam, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants