Fixes and tests for reproducibility #1580
Conversation
|
You can see that the test fails in the first travis run because the PR did not have the commit for the fixes for that run. The second travis run which includes the commit for the fixes passes. Please ignore the main |
.travis.yml
Outdated
| @@ -58,6 +58,8 @@ script: | |||
| --local_resources=400,1,1.0 \ | |||
| --test_tag_filters=-dev \ | |||
| //... | |||
| - tests/core/reproducibility/test.sh | |||
There was a problem hiding this comment.
I'd like you to migrate the test to something that can be run through bazel test //.... We used to have a number of tests that were run with bash, and I don't want to go back to those days.
I think you may be able to express this as an sh_test. Make sure to use size = "large", timeout = "moderate", and tags = ["local", "bazel", "exclusive"]. If you can reuse some of the infrastructure in tests/bazel_tests.bzl, please do. I don't think you'll be able to use bazel_test directly though, since it's designed for verifying the result of a single bazel command.
There was a problem hiding this comment.
Looking into it.
tests/core/reproducibility/test.sh
Outdated
| ) | ||
| build_opts=() | ||
|
|
||
| if [[ $(uname -s) == "Linux" ]]; then |
There was a problem hiding this comment.
I'd prefer to leave this flag out to keep the test simpler, even if it speeds up the tests. This will probably eventually be the default.
tests/core/reproducibility/test.sh
Outdated
| build_opts+=("--experimental_sandbox_base=/dev/shm") | ||
| fi | ||
|
|
||
| bazel "${bazel_opts[@]}" build "${build_opts[@]+"${build_opts[@]}"}" \ |
There was a problem hiding this comment.
I'm not familiar with the syntax for expanding build_opts here. Can it just be "${build_opts[@]}"?
There was a problem hiding this comment.
Now removed, so no longer applicable. But the main reason was that empty arrays are considered unset in most bash versions. This will trigger the trap in set -u and the script fails. The syntax I had is the alternative value mechanism.
http://wiki.bash-hackers.org/syntax/pe#use_an_alternate_value
tests/core/reproducibility/test.sh
Outdated
| set -eu -o pipefail | ||
|
|
||
| function run_bazel() { | ||
| local output_base |
There was a problem hiding this comment.
Assigning "$1" to another local here would improve readability.
tests/core/reproducibility/test.sh
Outdated
| fi | ||
|
|
||
| bazel "${bazel_opts[@]}" build "${build_opts[@]+"${build_opts[@]}"}" \ | ||
| //tests/core/go_binary:all \ |
There was a problem hiding this comment.
It would be better to test against targets in this directory instead of elsewhere. //tests/core/go_binary:all may pick up some unexpected things, depending on what we put in there in the future. It's better for this test to be isolated.
There was a problem hiding this comment.
Alright. I will create some binary with cgo here.
|
This looks great! I thought I catched them all earlier but it looks like that wasn't the case! |
|
I have addressed all current comments. I also reorganized the commits so that I could run the new tests without the fixes and see how they fail. |
siddharthab
force-pushed
the
repro
branch
2 times, most recently
from
1be1596
to
56cd957
Compare
|
Two more commits to make the tests pass and some hygiene. |
|
Reorganize commits again and use bundled md5sum because macOS travis image does not have md5sum. |
|
Travis test failures are unrelated. |
Includes a reproducibility test in travis, that will test if stdlib and binaries with cgo are reproducible across multiple builds by the same user. This still does not test if the binaries are reproducible across multiple users or machines. I am relying on tests/legacy/reproducible_binary to check that.
There are two fixes included here:
Resolves #1518.
Resolves #1547.