-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Explore adding a reproducibility test to rust test infrastructure. #139793
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
base: master
Are you sure you want to change the base?
Conversation
r? @marcoieni rustbot has assigned @marcoieni. Use |
238555f
to
916f799
Compare
cfc6ce8
to
46485be
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@rustbot label -T-bootstrap |
@rustbot label +T-infra |
please write this as a comment in the code 👍 |
4babe2f
to
4f8cb89
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR looks good overall, the comments are great, thanks!
I left a few more minor comments.
I want to involve @Kobzol to ask him if he agrees with the overall strategy here. I.e.:
- do we want to add this workflow?
- does it solve the problem in the right way?
While it would be great to have some reproducibility checks on CI, I don't think that the current approach used by this PR is the right one, for a few reasons.
I think that this is something that we should ideally discuss with t-infra first to gather consensus and come up with some plan, before starting with the implementation. |
7333dce
to
db9e6e2
Compare
I'm not fully sure of this. The test seems too slow to be run on each PR. If we do this as a post merge, this would require some effort again to be fixed. Also there are reproducibility issues already with rustdoc, cargo-clippy etc. I suggest that this test be run as post merge test and reproducibility issues to be fixed before releases.
I did try adding diffoscope for this, but diffoscope takes up too much time. When I tried adding this test diffoscope always got cancelled, I think this is because the test had already crossed 5 hours.
This sounds good. We should provide infrastructure to run this test locally.
This could work. |
As I noted above, I don't expect the test to take more than 1.5 hours on Linux, if we simply do two stage 2 build with proper caching. So form this point of view I think it should be fine to run on every merge.
Could you clarify why it wouldn't be enough? |
Sorry, I missed reading the part where you moved the sources to a new location, so now this does make sense. |
Fixes #75362
Trying to add a reproducibility check on rust infrastructure itself to detect reproducibility issues.
These were the issues encountered till now:
Questions:
This is also related to issue: Rust reproducibility issue - Finding the proper fix #134589
Zulip discussion thread:
#t-infra > Add reproducibility check to detect reproducibility issues