The CI currently inherits almost all env vars from the host. This was problematic in the past and causing non-determinism, e.g. the fix in commit fa12558d21aa03c22407a1458a0345d8a7ab6a4b. It is still problematic today, see e.g. #31349 (comment), or #32935
This fixes #32935 by only passing env vars documented in ./ci/test/00_setup_env.sh.
Implementation-wise, instead of cramming the python code into the python -c "" statement, just start a fresh py file, which is easier to handle.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33002.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | willcl-ark |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
🚧 At least one of the CI tasks failed.
Task lint: https://github.com/bitcoin/bitcoin/runs/46202380371
LLM reason (✨ experimental): The CI failure is caused by a Python linter warning about using open() without specifying utf8 encoding.
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
28+ settings = set(l.split("=")[0].split("export ")[1] for l in settings)
29+ # Add this one manually, because it is the only one set inside the
30+ # container that also allows external overwrites
31+ settings.add("BASE_BUILD_DIR")
32+
33+ env_file = "/tmp/env-{u}-{c}".format(
Could be worth moving the comment from 02_run_container.sh to here as this is where this format is first contructed.
0 # Append $USER to /tmp/env to support multi-user systems and $CONTAINER_NAME
1 # to allow support starting multiple runs simultaneously by the same user.
This seems ok to me.
Should we update https://github.com/bitcoin/bitcoin/blob/00604296e178b13fb1a701a84deac1e20e15259d/ci/README.md?plain=1#L27 and https://github.com/bitcoin/bitcoin/blob/00604296e178b13fb1a701a84deac1e20e15259d/ci/README.md?plain=1#L46 as we now don’t pass $HOME or $PATH in?
Should we update
It is possible to remove the env -i ... wrapping, because it is less required. However, I still somewhat prefer to explicitly and cleanly pass all CI options in a single Bash command, instead of taking anything that matches from the global env. For example, OpenSuse (or other Linux distros) may set HOST globally in the env, which is then taken by the CI system, because it is (correctly) recognized as a CI variable. The build will fail in that case. Similarly, if you exported MAKEJOBS previously, the CI will take that, and if you want to change it, it seems clearer to explicitly do it in the call. But no strong opinion. Happy to revert commit fa378bed56db66119b7f5847b749c4900b04c0a1 and push it here, if you think it is better.
ACK 3333d3f75f8917cf8c183984e9b81e2d7a447ca5
Seems reasonable to only export documented vars into the container.
I tried to think if there was a “cleaner” way to do this without creating a new file, but couldn’t think of anything so maintainable as this simple python.
Labels
Tests