Pipeline defined Bazel version #1141
Conversation
.buildkite/bindists-pipeline
Outdated
| echo "common:ci --build_tag_filters -requires_lz4,-requires_proto,-requires_zlib,-requires_doctest,-requires_c2hs,-requires_threaded_rts,-dont_test_with_bindist" > .bazelrc.local | ||
| # XXX: See .bazelrc [backward compatible options] for the the rational behind this flag | ||
| echo "build --incompatible_use_python_toolchains=false" >> .bazelrc.local | ||
| echo "build:ci --experimental_strict_action_env" >> .bazelrc.local | ||
| ./tests/run-start-script.sh --use-bindists | ||
| bazel build --config ci //tests/... |
There was a problem hiding this comment.
This bit is copied from .buildkite/pipeline.yml (except for --experimental_strict_action_env)
.buildkite/pipeline.yml
Outdated
| @@ -5,6 +5,8 @@ steps: | |||
| echo "build:ci --host_platform=@rules_haskell//haskell/platforms:linux_x86_64_nixpkgs" > .bazelrc.local | |||
| nix-shell --arg docTools false --pure --run ' | |||
| set -e | |||
| # Ensure that the Nixpkgs and bindists Bazel versions match. | |||
There was a problem hiding this comment.
what about
| # Ensure that the Nixpkgs and bindists Bazel versions match. | |
| # Ensure that the Nixpkgs bazel version matches with what's specified in .buildkite/bazel-version |
?
There was a problem hiding this comment.
Do we need to run the same test in .buildkite/bindists-pipeline too?
There was a problem hiding this comment.
I mean, we don't have to, because we do the fetching there, so maybe update the comment to
| # Ensure that the Nixpkgs and bindists Bazel versions match. | |
| # Ensure that the Nixpkgs bazel version matches with what's specified in .buildkite/bazel-version and fetched in `.buildkite/bindists-pipeline` for the bindists version |
There was a problem hiding this comment.
The version is used to construct the URL from which Bazel is fetch, so checking the version against that file once again is redundant.
I've expanded the comment as suggested.
| @@ -0,0 +1,14 @@ | |||
| #!/usr/bin/env bash | |||
There was a problem hiding this comment.
This could have a comment stating it calls the fetch-bazel-bindist script, configures bazelrc, and adds fetched bazel to $PATH.
There was a problem hiding this comment.
I've added a comment to that effect.
| # converted to using `rules_sh`'s POSIX toolchain. | ||
| echo "build:ci --experimental_strict_action_env" >> .bazelrc.local | ||
| ./tests/run-start-script.sh --use-bindists | ||
| bazel build --config ci //tests/... |
There was a problem hiding this comment.
If the environment changes propagate to the next buildkite step, we probably should move this to pipeline.yml, like in the nixpkgs case.
There was a problem hiding this comment.
I tried that initially and found that
- You have to be careful to escape variables. E.g.
\$PATHinstead of$PATH. export PATH=...didn't seem to be picked up andbazelcalled the globally installed version instead.
So, putting the steps into a dedicated script made things much easier and less error prone.
FWIW the examples recommend the same thing.
There was a problem hiding this comment.
I'm mostly concerned about maintaining those three lines:
./tests/run-start-script.sh --use-(bindists,nix)
bazel build --config ci //tests:run-tests
./bazel-ci-bin/tests/run-tests
in two places.
What about moving that into a build.sh script, which assumes a right bazel to be in $PATH, and calling it from both pipelines?
We could have build.sh accepting the --use-? as a parameter.
There was a problem hiding this comment.
Those lines are different between nixpkgs and bindists.
In the nixpkgs pipeline they are:
./tests/run-start-script.sh --use-nix
bazel build --config ci //tests:run-tests
./bazel-ci-bin/tests/run-tests
bazel coverage //tests/... --config ci --build_tag_filters "coverage-compatible" --test_tag_filters "coverage-compatible" --test_output=all
And in the bindists pipeline they are:
./tests/run-start-script.sh --use-bindists
bazel build --config ci //tests/...
Once we've fixed what's missing for the bindist so it can run the same steps as nixpkgs then we should indeed unify these steps into something like build.sh.
There was a problem hiding this comment.
Can you create a followup issue for that?
|
I'd suggest to squash the commits into one, as the first one failed CI. |
aherrmann
force-pushed
the
pipeline-bazel-version
branch
from
d982e79 to
e562d98
Compare
- The bindists pipeline now defines the Bazel version it will use.
Bazel is fetched as a single binary from GitHub.
- The nixpkgs pipeline confirms that its Bazel version matches.
- The bindists pipeline is defined in its own script file.
This is to avoid shell escaping issues.
- Enables strict action env in the bindists pipeline.
This avoids changes in `$PATH` invalidating cached builds with
`use_default_action_env`. This works with bindists targets, but the
nixpkgs pipeline still contains some targets that are incompatible
with strict action env.
jwiegley
force-pushed
the
pipeline-bazel-version
branch
from
e562d98 to
63218cd
Compare
Bazel is fetched as a single binary from GitHub.
This is to avoid shell escaping issues.
This avoids changes in
$PATHinvalidating cached builds withuse_default_action_env. This works with bindists targets, but thenixpkgs pipeline still contains some targets that are incompatible
with strict action env.