ci: Allow running iwyu CI in worktree#34441
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste ConflictsNo conflicts as of last run. |
fa56720 to
8888711
Compare
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
ci/test/03_test_script.sh
Outdated
| # a git-archive, a git-worktree, or a normal git repo. | ||
| # The iwyu task requires a working git repo, which may not always be | ||
| # available, so initialize one with force. | ||
| rm -rf .git |
There was a problem hiding this comment.
In commit "ci: Allow running iwyu ci in worktree" (8888711)
This seems dangerous. It should be safe to run ci commands locally without expecting your git history to be wiped away. Would suggest a tweak like:
# Initialize git if this is not a normal git directory (e.g. is a worktree)
if [[ "$RUN_IWYU" == true && ! -d .git ]]; then
[[ -e .git || -L .git ]] && mv .git .git-previous
git init
fiThere was a problem hiding this comment.
I'd doubt that this script will pass at all when run locally and it is not designed to be used that way. Maybe there should be a check in the beginning, with an early abort if DANGER_RUN_CI_ON_HOST is not set?
In any case, I've replaced this line with mv .git .git_ci_backup || true
8888711 to
aaaa717
Compare
aaaa717 to
faa028d
Compare
There was a problem hiding this comment.
In the case that someone runs this script locally, might it be prudent to add a
mv .git_ci_backup .git || trueto the end of the script to restore their .git directory to it's original state and minimise broken worktree-related disappointments where possible?
edit: sorry, just seen yours and ryanofsky's comments above!
faa028d to
fa8ed6a
Compare
No worries. I've implemented my idea from the comment now. |
fa8ed6a to
faffb20
Compare
|
The bash pipes are broken. Sigh, I guess it is time to re-write this in Python. https://github.com/bitcoin/bitcoin/actions/runs/22091425710/job/63837401348?pr=34441#step:11:2611 edit: dropped pipefail for now. This is unrelated and can be done in a follow-up. |
fa9de75 to
99997ca
Compare
willcl-ark
left a comment
There was a problem hiding this comment.
ACK 99997ca
Guarding with DANGER_RUN_CI_ON_HOST make good sense for these scripts as they modify the filesystem.
Tested locally in a worktree (but I did not test outside of a container!).
nit: tiny typo in commit message "exection"
fa1dd69 to
fab95ad
Compare
The shell scripts are inherently unsafe, because they will install new software packages, modify global configuration settings, write to the root / or $HOME, and possibly modify the git repo. The only safe way to run them is through the CI system itself, that is the ci_exec python function. The ci_exec funtion ensures that the user has set up a sandbox externally and set DANGER_RUN_CI_ON_HOST=1 at their own risk, or that a sandbox was set up with the given container_id, in which case it is safe to set DANGER_RUN_CI_ON_HOST=1 for that sandbox. Also, it is safe to set DANGER_RUN_CI_ON_HOST=1 when building the sandbox image in ci/test_imagefile. Then, the two shell scripts can reject early if unsafe execution is detected.
fab95ad to
fafdb8f
Compare
thx, fixed |
willcl-ark
left a comment
There was a problem hiding this comment.
reACK fafdb8f
Based on range-diff (excluding rebase) of:
src/core/bitcoin on pcp-loglevel [$?⇕] via △ v4.1.2 via 🐍 v3.13.11 via ❄️ impure (nix-shell-env)
❯ prw upstream 34441
From github.com:bitcoin/bitcoin
* branch refs/pull/34441/head -> FETCH_HEAD
git range-diff 35e6444fdc4068adc79082648f9889ad593e623b..99997ca18520783cbcf9eba8bac1dc5785362fb9 c8c9c1e61759f689615a304254fed33cda7f895e..fafdb8f635bc157f55e23890264d12170ecd41ae
1: fa591c49a98 ! 1: fab73e213de ci: Reject unsafe exection of shell scripts
@@ Metadata
Author: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
## Commit message ##
- ci: Reject unsafe exection of shell scripts
+ ci: Reject unsafe execution of shell scripts
The shell scripts are inherently unsafe, because they will install new
software packages, modify global configuration settings, write to the
2: 99997ca1852 = 2: fafdb8f635b ci: Allow running iwyu ci in worktree
HEAD is now at fafdb8f635b ci: Allow running iwyu ci in worktree
Currently, the iwyu CI fails to run in a git-worktree, or git-archive. This is due to the use of
git diff.Fix this by force-initializing a dummy git repo with a single dummy commit.
It may be possible to detect when
git diffis not available in the directory, and only apply the fallback when needed, but the git history is not needed and it is easier to unconditionally apply the git init.