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

Bootstrap build fails: found crate serde_derive compiled by an incompatible version of rustc #125578

Open
RalfJung opened this issue May 26, 2024 · 16 comments · Fixed by #127050
Open
Assignees
Labels
T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-cargo Relevant to the cargo team, which will review and decide on the PR/issue.

Comments

@RalfJung
Copy link
Member

I did a git pull, fixed the submodules, and now I am trying to run ./x.py check. However, that fails:

$ ./x.py check
Building bootstrap
   Compiling build_helper v0.1.0 (/home/r/src/rust/rustc.3/src/tools/build_helper)
error[E0514]: found crate `serde_derive` compiled by an incompatible version of rustc
 --> /home/r/src/rust/rustc.3/src/tools/build_helper/src/metrics.rs:1:5
  |
1 | use serde_derive::{Deserialize, Serialize};
  |     ^^^^^^^^^^^^
  |
  = note: the following crate versions were found:
          crate `serde_derive` compiled by rustc 1.79.0-beta.1 (6b544f5ff 2024-04-28): /home/r/src/rust/rustc.3/build/bootstrap/debug/deps/libserde_derive-970fa0dc5f1e06a7.so
  = help: please recompile that crate using this compiler (rustc 1.79.0-beta.6 (66eb3e404 2024-05-23)) (consider running `cargo clean` first)

[...]

This must b a fairly recent regression, I've never had to manually clean bootstrap before.

Cc @onur-ozkan

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label May 26, 2024
@onur-ozkan onur-ozkan added T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels May 26, 2024
@onur-ozkan
Copy link
Member

onur-ozkan commented May 26, 2024

This must b a fairly recent regression, I've never had to manually clean bootstrap before.

So it doesn't happen after x clean/rm -rf build?

@RalfJung
Copy link
Member Author

I removed build/boostrap and now everything works as it should yeah.

@bjorn3
Copy link
Member

bjorn3 commented May 26, 2024

#125478 updated the bootstrap compiler. Does it reproduce if you do a clean build the commit right before this PR (77d4115) and then build right after this PR (0b2f194)?

@onur-ozkan
Copy link
Member

Presumably bootstrap used the outdated artifacts instead of generating them with the new compiler (#125478).

@bjorn3
Copy link
Member

bjorn3 commented May 26, 2024

I did expect cargo to clean them up when it sees that the rustc version changed. Maybe something is broken with that?

@RalfJung
Copy link
Member Author

#125478 updated the bootstrap compiler. Does it reproduce if you do a clean build the commit right before this PR (77d4115) and then build right after this PR (0b2f194)?

Yes.

I think cargo hashes the version and channel, but they are unchanged for this beta update: 1.79.0, beta. The fact that it's beta.1 vs beta.6 should still usually trigger a rebuild though I would think, it should just use the same filenames... Cc @epage @weihanglo

@onur-ozkan onur-ozkan added the T-cargo Relevant to the cargo team, which will review and decide on the PR/issue. label May 27, 2024
@weihanglo
Copy link
Member

The root cause is likely to be that Cargo's cache for rustc -vV output was wrong. The current approach uses cmd args (including program path) and env vars as the cache key. The key is not able to distinguish bootstrap rustc because the path to bootstrap rustc is always build/<host-triple>/stage0/bin/rustc.

I can think of a heavy-hammer solution: We can additionally compute the digest of rustc executable. Anyone has a lightweight alternative?

@RalfJung
Copy link
Member Author

Is rustc -vV so expensive that a cache is justified?

A cheaper alternative would be to key on the size and mtime of the binary.

@bjorn3
Copy link
Member

bjorn3 commented May 28, 2024

Is rustc -vV so expensive that a cache is justified?

For small programs it was at least. A single rustc invocation used to be at least 100ms (for some people almost 300ms) with the rustup wrapper overhead included. Rustup optimizations have since reduced this by a significant amount, but it is still about 4x slower with the rustup wrapper than without for me.

@ehuss
Copy link
Contributor

ehuss commented May 31, 2024

The key is not able to distinguish bootstrap rustc because the path to bootstrap rustc is always build/<host-triple>/stage0/bin/rustc.

I think there is more nuance here. Normally cargo can distinguish different rustc via the mtime of the executable. However, since #123246 (@Kobzol FYI), which is new in 1.79, the timestamps are always the same.

This will likely be a problem with every bootstrap bump moving forward.

I'm trying to think if this is going to be a more serious problem for other non-rustup users of cargo. I looked at the linux install.sh, and fortunately it updates the timestamps to the current time. I did not look at the macos or windows installers, can someone look and see if recent versions preserve the tarball timestamps? Can anyone think of other users that fetch and extract the tarball directly? They will likely also be impacted by #123246.

I can think of two possible solutions:

A slightly hacky solution would be to add the file size to cargo's hash.1 We can be pretty confident that the size will change, and that is cheap to acquire.

Another possibility is to have bootstrap.py delete the bootstrap directory whenever it extracts a new stage0 (presumably in the download_toolchain function). That will slightly hurt caching if you are switching back and forth between different stage0 versions, but I would expect that to be rare. On the plus side, the bootstrap directory will no longer grow without bounds.

Footnotes

  1. I've been on the fence about doing this in general in cargo, since I think it can lead to insidious rebuild problems that are even harder to detect than they are now. However, I think it wouldn't hurt to add it just for the Rustc hash to start with.

@Kobzol
Copy link
Contributor

Kobzol commented May 31, 2024

I think that we should somehow update the timestamp in the source builds, e.g. based on the time of the latest commit at the time of packaging, rather than hardcode the same time for everything. I'll take a look at it.

@workingjubilee
Copy link
Member

I have had to wipe my build dir like 8 times while cycling through different PRs, and I've accidentally rebuilt LLVM about 5 times.

@onur-ozkan
Copy link
Member

I have had to wipe my build dir like 8 times while cycling through different PRs, and I've accidentally rebuilt LLVM about 5 times.

Right..

I thought it's worth bootstrap to have workaround for this and created #125911 PR. So people don't have to waste their time finding the actual problem and checking issues/zulip.

@workingjubilee
Copy link
Member

Yeaaah. Redownloading all the bootstrap compilers and then restarting the build isn't exactly optimal, it just happened to be faster, for me, than "try to think about what the actual issue is".

Not really sure why the LLVM got rebuilt though...

...also, 11.

@RalfJung
Copy link
Member Author

RalfJung commented Jun 3, 2024

FWIW rm build/bootstrap -rf works fine for me and doesn't redownload anything nor rebuild LLVM.

fmease added a commit to fmease/rust that referenced this issue Jun 4, 2024
…bertlarsan68

delete bootstrap build before switching to bumped rustc

Technically, wiping bootstrap builds can increase the build time. But in practice, trying to manually resolve post-bump issues and even accidentally removing the entire build directory will result in a much greater loss of time. After all, the bootstrap build process is not a particularly lengthy operation.

Workaround for rust-lang#125578
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jun 5, 2024
…bertlarsan68

delete bootstrap build before switching to bumped rustc

Technically, wiping bootstrap builds can increase the build time. But in practice, trying to manually resolve post-bump issues and even accidentally removing the entire build directory will result in a much greater loss of time. After all, the bootstrap build process is not a particularly lengthy operation.

Workaround for rust-lang#125578
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Jun 5, 2024
Rollup merge of rust-lang#125911 - onur-ozkan:wipe-broken-cache, r=albertlarsan68

delete bootstrap build before switching to bumped rustc

Technically, wiping bootstrap builds can increase the build time. But in practice, trying to manually resolve post-bump issues and even accidentally removing the entire build directory will result in a much greater loss of time. After all, the bootstrap build process is not a particularly lengthy operation.

Workaround for rust-lang#125578
@Kobzol Kobzol self-assigned this Jun 27, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jun 29, 2024
…-ozkan

Make mtime of reproducible tarballs dependent on git commit

Since rust-lang#123246, our tarballs should be fully reproducible. That means that the mtime of all files and directories in the tarballs is set to the date of the first Rust commit (from 2006). However, this is causing some mtime invalidation issues (rust-lang#125578 (comment)).

Ideally, we would like to keep the mtime reproducible, but still update it with new versions of Rust. That's what this PR does. It modifies the tarball installer bootstrap invocation so that if the current rustc directory is managed by git, we will set the UTC timestamp of the latest commit as the mtime for all files in the archive. This means that the archive should be still fully reproducible from a given commit SHA, but it will also be changed with new beta bumps and `download-rustc` versions.

Note that only files are set to this mtime, directories are still set to the year 2006, because the `tar` library used by `rust-installer` doesn't allow us to selectively override mtime for directories (or at least I haven't found it). We could work around that by doing all the mtime modifications in bootstrap, but that would require more changes. I think/hope that just modifying the file mtimes should be enough. It should at least fix cargo `rustc` mtime invalidation.

Fixes: rust-lang#125578

r? `@onur-ozkan`
bors added a commit to rust-lang-ci/rust that referenced this issue Jun 29, 2024
Make mtime of reproducible tarballs dependent on git commit

Since rust-lang#123246, our tarballs should be fully reproducible. That means that the mtime of all files and directories in the tarballs is set to the date of the first Rust commit (from 2006). However, this is causing some mtime invalidation issues (rust-lang#125578 (comment)).

Ideally, we would like to keep the mtime reproducible, but still update it with new versions of Rust. That's what this PR does. It modifies the tarball installer bootstrap invocation so that if the current rustc directory is managed by git, we will set the UTC timestamp of the latest commit as the mtime for all files in the archive. This means that the archive should be still fully reproducible from a given commit SHA, but it will also be changed with new beta bumps and `download-rustc` versions.

Note that only files are set to this mtime, directories are still set to the year 2006, because the `tar` library used by `rust-installer` doesn't allow us to selectively override mtime for directories (or at least I haven't found it). We could work around that by doing all the mtime modifications in bootstrap, but that would require more changes. I think/hope that just modifying the file mtimes should be enough. It should at least fix cargo `rustc` mtime invalidation.

Fixes: rust-lang#125578

r? `@onur-ozkan`

try-job: x86_64-gnu-distcheck
bors added a commit to rust-lang-ci/rust that referenced this issue Jul 2, 2024
…zkan

Make mtime of reproducible tarballs dependent on git commit

Since rust-lang#123246, our tarballs should be fully reproducible. That means that the mtime of all files and directories in the tarballs is set to the date of the first Rust commit (from 2006). However, this is causing some mtime invalidation issues (rust-lang#125578 (comment)).

Ideally, we would like to keep the mtime reproducible, but still update it with new versions of Rust. That's what this PR does. It modifies the tarball installer bootstrap invocation so that if the current rustc directory is managed by git, we will set the UTC timestamp of the latest commit as the mtime for all files in the archive. This means that the archive should be still fully reproducible from a given commit SHA, but it will also be changed with new beta bumps and `download-rustc` versions.

Note that only files are set to this mtime, directories are still set to the year 2006, because the `tar` library used by `rust-installer` doesn't allow us to selectively override mtime for directories (or at least I haven't found it). We could work around that by doing all the mtime modifications in bootstrap, but that would require more changes. I think/hope that just modifying the file mtimes should be enough. It should at least fix cargo `rustc` mtime invalidation.

Fixes: rust-lang#125578

r? `@onur-ozkan`

try-job: x86_64-gnu-distcheck
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jul 2, 2024
…-ozkan

Make mtime of reproducible tarballs dependent on git commit

Since rust-lang#123246, our tarballs should be fully reproducible. That means that the mtime of all files and directories in the tarballs is set to the date of the first Rust commit (from 2006). However, this is causing some mtime invalidation issues (rust-lang#125578 (comment)).

Ideally, we would like to keep the mtime reproducible, but still update it with new versions of Rust. That's what this PR does. It modifies the tarball installer bootstrap invocation so that if the current rustc directory is managed by git, we will set the UTC timestamp of the latest commit as the mtime for all files in the archive. This means that the archive should be still fully reproducible from a given commit SHA, but it will also be changed with new beta bumps and `download-rustc` versions.

Note that only files are set to this mtime, directories are still set to the year 2006, because the `tar` library used by `rust-installer` doesn't allow us to selectively override mtime for directories (or at least I haven't found it). We could work around that by doing all the mtime modifications in bootstrap, but that would require more changes. I think/hope that just modifying the file mtimes should be enough. It should at least fix cargo `rustc` mtime invalidation.

Fixes: rust-lang#125578

r? `@onur-ozkan`

try-job: x86_64-gnu-distcheck
@bors bors closed this as completed in 444a0ff Jul 3, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Jul 3, 2024
Rollup merge of rust-lang#127050 - Kobzol:reproducibility-git, r=onur-ozkan

Make mtime of reproducible tarballs dependent on git commit

Since rust-lang#123246, our tarballs should be fully reproducible. That means that the mtime of all files and directories in the tarballs is set to the date of the first Rust commit (from 2006). However, this is causing some mtime invalidation issues (rust-lang#125578 (comment)).

Ideally, we would like to keep the mtime reproducible, but still update it with new versions of Rust. That's what this PR does. It modifies the tarball installer bootstrap invocation so that if the current rustc directory is managed by git, we will set the UTC timestamp of the latest commit as the mtime for all files in the archive. This means that the archive should be still fully reproducible from a given commit SHA, but it will also be changed with new beta bumps and `download-rustc` versions.

Note that only files are set to this mtime, directories are still set to the year 2006, because the `tar` library used by `rust-installer` doesn't allow us to selectively override mtime for directories (or at least I haven't found it). We could work around that by doing all the mtime modifications in bootstrap, but that would require more changes. I think/hope that just modifying the file mtimes should be enough. It should at least fix cargo `rustc` mtime invalidation.

Fixes: rust-lang#125578

r? ``@onur-ozkan``

try-job: x86_64-gnu-distcheck
@onur-ozkan
Copy link
Member

Seems like this isn't fixed yet (see #129924 (comment)).

@onur-ozkan onur-ozkan reopened this Sep 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-cargo Relevant to the cargo team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants