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

Hash the remapped sysroot instead of the original. #63505

Merged
merged 2 commits into from Aug 17, 2019

Conversation

@jgalenson
Copy link

commented Aug 12, 2019

One of the reasons that rustc builds are not reproducible is because the --sysroot path is dependent on the current directory. We can fix this by hashing the remapped sysroot instead of the original when applicable.

Note that with this patch, the hash will stay the same if both the sysroot and the remapped path change. However, given that if the contents of the sysroot change the hash will also stay the same, this might be acceptable. I would appreciate feedback on the best way to do this.

This helps #34902, although it does not fix it by itself.

Joel Galenson
Hash the remapped sysroot instead of the original.
This will help reproducible builds, as the sysroot depends on the
working directory.
@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Aug 12, 2019

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @cramertj (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@cramertj

This comment has been minimized.

Copy link
Member

commented Aug 12, 2019

This LGTM, but just to be sure, r? @alexcrichton

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

Thanks for the PR! Since the sysroot argument is only used to load other libraries, which are themselves all tracked, I think we may be able to get by from just skipping the sysroot hashing entirely?

Joel Galenson
Do not track the sysroot.
As suggested by @alexcrichton, the sysroot only loads libraries that
are themselves tracked.
@jgalenson

This comment has been minimized.

Copy link
Author

commented Aug 13, 2019

I didn't know the other libraries were themselves tracked; that does make this cleaner. All the tests seemed to pass locally, so here it is.

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

@bors: r+

@bors

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2019

📌 Commit 692c0bf has been approved by alexcrichton

@jgalenson

This comment has been minimized.

Copy link
Author

commented Aug 14, 2019

There's a problem with the test runner: the Azure dashboard shows the test as having passed about 20 hours ago, but the Github dashboard doesn't see it. Can the test be restarted or something?

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

That's ok those are just PR tests, this is in the queue to be tested and merged in full.

@jgalenson

This comment has been minimized.

Copy link
Author

commented Aug 14, 2019

Ah, okay. Thanks!

Centril added a commit to Centril/rust that referenced this pull request Aug 15, 2019

Rollup merge of rust-lang#63505 - jgalenson:sysroot-hash, r=alexcrichton
Hash the remapped sysroot instead of the original.

One of the reasons that rustc builds are not reproducible is because the --sysroot path is dependent on the current directory.  We can fix this by hashing the remapped sysroot instead of the original when applicable.

Note that with this patch, the hash will stay the same if both the sysroot and the remapped path change.  However, given that if the contents of the sysroot change the hash will also stay the same, this might be acceptable.  I would appreciate feedback on the best way to do this.

This helps rust-lang#34902, although it does not fix it by itself.

bors added a commit that referenced this pull request Aug 15, 2019

Auto merge of #63603 - Centril:rollup-sdtmwk7, r=Centril
Rollup of 8 pull requests

Successful merges:

 - #63173 (Use libunwind from llvm-project submodule for musl targets)
 - #63462 (Opaque builtin derive macros)
 - #63470 (Utilize -Zbinary-dep-depinfo in rustbuild)
 - #63505 (Hash the remapped sysroot instead of the original.)
 - #63539 (Suggest Rust 2018 on `<expr>.await` with no such field)
 - #63545 (Feature gate 'yield $expr?' pre-expansion)
 - #63548 (Update rustc-demangle to 0.1.16.)
 - #63584 (libcore: more cleanups using `#![feature(associated_type_bounds)]`)

Failed merges:

r? @ghost
@Centril

This comment has been minimized.

Copy link
Member

commented Aug 16, 2019

@bors p=1

Centril added a commit to Centril/rust that referenced this pull request Aug 17, 2019

Rollup merge of rust-lang#63505 - jgalenson:sysroot-hash, r=alexcrichton
Hash the remapped sysroot instead of the original.

One of the reasons that rustc builds are not reproducible is because the --sysroot path is dependent on the current directory.  We can fix this by hashing the remapped sysroot instead of the original when applicable.

Note that with this patch, the hash will stay the same if both the sysroot and the remapped path change.  However, given that if the contents of the sysroot change the hash will also stay the same, this might be acceptable.  I would appreciate feedback on the best way to do this.

This helps rust-lang#34902, although it does not fix it by itself.

bors added a commit that referenced this pull request Aug 17, 2019

Auto merge of #63653 - Centril:rollup-sdu26ew, r=Centril
Rollup of 5 pull requests

Successful merges:

 - #62737 (Override Cycle::try_fold)
 - #63269 (Serialize additional data for procedural macros)
 - #63505 (Hash the remapped sysroot instead of the original.)
 - #63559 (rustc_codegen_utils: account for 1-indexed anonymous lifetimes in v0 mangling.)
 - #63621 (Modify librustc_llvm to pass -DNDEBUG while compiling.)

Failed merges:

r? @ghost
@bors

This comment has been minimized.

Copy link
Contributor

commented Aug 17, 2019

⌛️ Testing commit 692c0bf with merge 50cc578...

bors added a commit that referenced this pull request Aug 17, 2019

Auto merge of #63505 - jgalenson:sysroot-hash, r=alexcrichton
Hash the remapped sysroot instead of the original.

One of the reasons that rustc builds are not reproducible is because the --sysroot path is dependent on the current directory.  We can fix this by hashing the remapped sysroot instead of the original when applicable.

Note that with this patch, the hash will stay the same if both the sysroot and the remapped path change.  However, given that if the contents of the sysroot change the hash will also stay the same, this might be acceptable.  I would appreciate feedback on the best way to do this.

This helps #34902, although it does not fix it by itself.

Centril added a commit to Centril/rust that referenced this pull request Aug 17, 2019

Rollup merge of rust-lang#63505 - jgalenson:sysroot-hash, r=alexcrichton
Hash the remapped sysroot instead of the original.

One of the reasons that rustc builds are not reproducible is because the --sysroot path is dependent on the current directory.  We can fix this by hashing the remapped sysroot instead of the original when applicable.

Note that with this patch, the hash will stay the same if both the sysroot and the remapped path change.  However, given that if the contents of the sysroot change the hash will also stay the same, this might be acceptable.  I would appreciate feedback on the best way to do this.

This helps rust-lang#34902, although it does not fix it by itself.
@Centril

This comment has been minimized.

Copy link
Member

commented Aug 17, 2019

@bors retry rolled up.

bors added a commit that referenced this pull request Aug 17, 2019

Auto merge of #63655 - Centril:rollup-ty1ot40, r=Centril
Rollup of 4 pull requests

Successful merges:

 - #62737 (Override Cycle::try_fold)
 - #63505 (Hash the remapped sysroot instead of the original.)
 - #63559 (rustc_codegen_utils: account for 1-indexed anonymous lifetimes in v0 mangling.)
 - #63621 (Modify librustc_llvm to pass -DNDEBUG while compiling.)

Failed merges:

r? @ghost
@RalfJung

This comment has been minimized.

Copy link
Member

commented Aug 17, 2019

Looks like the PR title / description no longer reflects the PR content?

@bors

This comment has been minimized.

Copy link
Contributor

commented Aug 17, 2019

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

@bors bors merged commit 692c0bf into rust-lang:master Aug 17, 2019

2 of 4 checks passed

pr
Details
homu Testing commit 692c0bf4ff8d1f551850962bd8d3af62033dee39 with merge 50cc578efa01a276f3869cf28cc0aa543f3d8462...
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (LinuxTools) LinuxTools succeeded
Details

@jgalenson jgalenson deleted the jgalenson:sysroot-hash branch Aug 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.