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.

Sign up for GitHub

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

Jump to bottom

x.py dist builds tools twice due to dependency vendoring? #93033

Closed
matthiaskrgr opened this issue Jan 18, 2022 · 5 comments · Fixed by #93047
Closed

x.py dist builds tools twice due to dependency vendoring? #93033

matthiaskrgr opened this issue Jan 18, 2022 · 5 comments · Fixed by #93047

Comments

@matthiaskrgr
Copy link
Member

There is this problem that x.py dist builds most of the tools twice.

It seems that during the initial build (which is the same as x.py build) we take the normal sources from $CARGO_HOME, but then there is some dependency vendoring taking place which seems to override some of the dependency paths to the vendored ones.

This causes fingerprints to change and some of the tools to be rebuilt:

   Vendoring xz2 v0.1.6 (/home/matthias/.cargo/registry/src/github.com-1ecc6299db9ec823/xz2-0.1.6) to vendor/xz2
   Vendoring yaml-merge-keys v0.4.1 (/home/matthias/.cargo/registry/src/github.com-1ecc6299db9ec823/yaml-merge-keys-0.4.1) to vendor/yaml-merge-keys
   Vendoring yaml-rust v0.3.5 (/home/matthias/.cargo/registry/src/github.com-1ecc6299db9ec823/yaml-rust-0.3.5) to vendor/yaml-rust-0.3.5
   Vendoring yaml-rust v0.4.4 (/home/matthias/.cargo/registry/src/github.com-1ecc6299db9ec823/yaml-rust-0.4.4) to vendor/yaml-rust
   Vendoring yansi-term v0.1.2 (/home/matthias/.cargo/registry/src/github.com-1ecc6299db9ec823/yansi-term-0.1.2) to vendor/yansi-term
To use vendored sources, add this to your .cargo/config.toml for this project:

[source.crates-io]
replace-with = "vendored-sources"

[source."https://github.com/bjorn3/rust-ar.git"]
git = "https://github.com/bjorn3/rust-ar.git"
branch = "do_not_remove_cg_clif_ranlib"
replace-with = "vendored-sources"

[source.vendored-sources]
directory = "vendor"
Dist rustc-1.60.0-dev-src
	finished in 283.091 seconds
Building stage1 tool cargo (x86_64-unknown-linux-gnu)
[2022-01-18T14:45:58Z INFO  cargo::core::compiler::fingerprint] stale: changed "/home/matthias/.cargo/registry/src/github.com-1ecc6299db9ec823/curl-sys-0.4.51+curl-7.80.0/curl"
[2022-01-18T14:45:58Z INFO  cargo::core::compiler::fingerprint]           (vs) "/home/matthias/vcs/github/rust/build/x86_64-unknown-linux-gnu/stage1-tools/x86_64-unknown-linux-gnu/release/build/curl-sys-92c84ce359fc681e/output"
[2022-01-18T14:45:58Z INFO  cargo::core::compiler::fingerprint]                FileTime { seconds: 1642516164, nanos: 851099564 } != FileTime { seconds: 1642516869, nanos: 931076291 }
[2022-01-18T14:45:58Z INFO  cargo::core::compiler::fingerprint] fingerprint error for cargo v0.60.0 (/home/matthias/vcs/github/rust/src/tools/cargo)/Build/TargetInner { ..: lib_target("cargo", ["lib"], "/home/matthias/vcs/github/rust/src/tools/cargo/src/cargo/lib.rs", Edition2021) }
[2022-01-18T14:45:58Z INFO  cargo::core::compiler::fingerprint]     err: current filesystem status shows we're outdated
[2022-01-18T14:45:58Z INFO  cargo::core::compiler::fingerprint] fingerprint error for cargo v0.60.0 (/home/matthias/vcs/github/rust/src/tools/cargo)/RunCustomBuild/TargetInner { ..: custom_build_target("build-script-build", "/home/matthias/vcs/github/rust/src/tools/cargo/build.rs", Edition2021) }
[2022-01-18T14:45:58Z INFO  cargo::core::compiler::fingerprint]     err: current filesystem status shows we're outdated
[2022-01-18T14:45:58Z INFO  cargo::core::compiler::fingerprint] fingerprint error for curl-sys v0.4.51+curl-7.80.0/RunCustomBuild/TargetInner { ..: custom_build_target("build-script-build", "/home/matthias/.cargo/registry/src/github.com-1ecc6299db9ec823/curl-sys-0.4.51+curl-7.80.0/build.rs", Edition2018) }
[2022-01-18T14:45:58Z INFO  cargo::core::compiler::fingerprint]     err: current filesystem status shows we're outdated
[2022-01-18T14:45:58Z INFO  cargo::core::compiler::fingerprint] fingerprint error for crates-io v0.33.1 (/home/matthias/vcs/github/rust/src/tools/cargo/crates/crates-io)/Build/TargetInner { ..: lib_target("crates_io", ["lib"], "/home/matthias/vcs/github/rust/src/tools/cargo/crates/crates-io/lib.rs", Edition2021) }
[2022-01-18T14:45:58Z INFO  cargo::core::compiler::fingerprint]     err: current filesystem status shows we're outdated
[2022-01-18T14:45:58Z INFO  cargo::core::compiler::fingerprint] fingerprint error for curl v0.4.41/Build/TargetInner { ..: lib_target("curl", ["lib"], "/home/matthias/.cargo/registry/src/github.com-1ecc6299db9ec823/curl-0.4.41/src/lib.rs", Edition2018) }
[2022-01-18T14:45:58Z INFO  cargo::core::compiler::fingerprint]     err: current filesystem status shows we're outdated
[2022-01-18T14:45:58Z INFO  cargo::core::compiler::fingerprint] fingerprint error for curl v0.4.41/RunCustomBuild/TargetInner { ..: custom_build_target("build-script-build", "/home/matthias/.cargo/registry/src/github.com-1ecc6299db9ec823/curl-0.4.41/build.rs", Edition2018) }
[2022-01-18T14:45:58Z INFO  cargo::core::compiler::fingerprint]     err: current filesystem status shows we're outdated
[2022-01-18T14:45:58Z INFO  cargo::core::compiler::fingerprint] fingerprint error for curl-sys v0.4.51+curl-7.80.0/Build/TargetInner { ..: lib_target("curl_sys", ["lib"], "/home/matthias/.cargo/registry/src/github.com-1ecc6299db9ec823/curl-sys-0.4.51+curl-7.80.0/lib.rs", Edition2018) }
[2022-01-18T14:45:58Z INFO  cargo::core::compiler::fingerprint]     err: current filesystem status shows we're outdated
[2022-01-18T14:45:58Z INFO  cargo::core::compiler::fingerprint] fingerprint error for git2-curl v0.14.1/Build/TargetInner { ..: lib_target("git2-curl", ["lib"], "/home/matthias/.cargo/registry/src/github.com-1ecc6299db9ec823/git2-curl-0.14.1/src/lib.rs", Edition2018) }
[2022-01-18T14:45:58Z INFO  cargo::core::compiler::fingerprint]     err: current filesystem status shows we're outdated
[2022-01-18T14:45:58Z INFO  cargo::core::compiler::fingerprint] fingerprint error for cargo v0.60.0 (/home/matthias/vcs/github/rust/src/tools/cargo)/Build/TargetInner { name: "cargo", tested: false, ..: with_path("/home/matthias/vcs/github/rust/src/tools/cargo/src/bin/cargo/main.rs", Edition2021) }
[2022-01-18T14:45:58Z INFO  cargo::core::compiler::fingerprint]     err: current filesystem status shows we're outdated
   Compiling curl-sys v0.4.51+curl-7.80.0
   Compiling curl v0.4.41
   Compiling cargo v0.60.0 (/home/matthias/vcs/github/rust/src/tools/cargo)
   Compiling crates-io v0.33.1 (/home/matthias/vcs/github/rust/src/tools/cargo/crates/crates-io)
   Compiling git2-curl v0.14.1
    Finished release [optimized] target(s) in 3m 17s
Building stage1 tool cargo-credential-1password (x86_64-unknown-linux-gnu)
@matthiaskrgr
Copy link
Member Author

Commenting out the PlainSourceTarball step seems to make tools not be build again.

rust/src/bootstrap/builder.rs

Line 498 in 9ad5d82

dist::PlainSourceTarball,

@matthiaskrgr matthiaskrgr mentioned this issue Jan 18, 2022
Merged
@ehuss
Copy link
Contributor

ehuss commented Jan 19, 2022

@matthiaskrgr Can you show which jobs are building tools multiple times?

The only one I know of is dist-x86_64-apple due to #90100. I don't think that is related to PlainSourceTarball, since dist-x86_64-apple does not build that step.

I am looking at dist-x86_64-linux, and it is building cargo twice (once as a tool, and once for RLS). There's a check that is supposed to prevent that, so I'm curious why that is happening.

@ehuss
Copy link
Contributor

ehuss commented Jan 20, 2022

I determined why cargo is being built twice with RLS. It is due to rust-lang/cargo#10178, where CFG_COMMIT_HASH changes between the two. The way it is set causes it to change based on which tool is being built.

I'm uncertain how to fix that. I'll need to think about it.

@matthiaskrgr
Copy link
Member Author

Oh then we talked about two different issues probably 😅

So when cargo is being built, it gets its own commithash from the rls repo, but then when we built cargo again as rls dependency, the commithash comes from the rls repo and we build both cargo (again, since hash changed) and rls with the commit hash from the rls repo?

Perhaps we can hack around like:
if we know that we are building in the rust repo, the cargo and rls could have a code branch that checks a different env var CFG_COMMIT_HASH_RLS and CFG_COMMIT_HASH_CARGO and bootstrap sets these accordingly.
This way CFG_COMMIT_HASH_RLS would always be set to the commit hash of the rls submodule (or later the rust repo, if rls gets subtree'd at some point?)

bors added a commit to rust-lang-ci/rust that referenced this issue Jan 23, 2022
@bors
Auto merge of rust-lang#93047 - matthiaskrgr:defer__dist_PlainSourceT…
1e40679 
…arball, r=Mark-Simulacrum

build: dist: defer PlainSourceTarball

Apparently it changes some tool sources and invalidates their fingerprints, forcing us to build them several times (before and after vendoring sources).
I have not dug into why vendoring actually invalidates the figreprints, but moving the vendoring lower in the pipeline seems to avoid the issue.
I could imagine that we somehow write a .cargo/config somewhere which somehow makes subsequent builds use the vendored deps but I was not able to find anything.

I checked the sizes of generated archives pre and post patch and their are the same, so I hope there is no functional change.

Fixes rust-lang#93033
@bors bors closed this as completed in 5f58a78 Jan 23, 2022
@ehuss
Copy link
Contributor

ehuss commented Jan 25, 2022

I have posted rust-lang/cargo#10323 to fix the issue with cargo and rls. That should help shave a minute or two on the dist builders.

I'm still planning on looking at the issues with #90100.

@jyn514 jyn514 mentioned this issue Jul 10, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

build: dist: defer PlainSourceTarball matthiaskrgr/rust
2 participants
@ehuss @matthiaskrgr