ci: run in worktrees #31787

willcl-ark · 2025-02-03T16:11:03Z

Run CI in worktrees by mounting the .git root target into the container
Run the linter in worktrees by mounting the .git root target into the container

AFAIK mounting the git root should not present any additional security issues, as this would already be mounted (in RO mode) when not using a worktree.

DrahtBot · 2025-02-03T16:11:06Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31787.

Reviews

See the guideline for information on the review process.
A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#31948 (ci: [lint] Use Cirrus dockerfile cache by maflcko)
#31349 (ci: detect outbound internet traffic generated while running tests by vasild)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

maflcko · 2025-02-03T16:17:17Z

ci/test/02_run_container.sh

+    --mount "${CI_DEPENDS_SOURCES_MOUNT}"
+    --mount "${CI_PREVIOUS_RELEASES_MOUNT}"
+  )
+  # Add worktree root mount, if needed


Why is any .git data needed to be copied, when an init .git is sufficient?

It would be easier to just call git init?

Hmmm, this was actually my first approach in the lint script, as it was simpler. Once I got this working in the Ci script, I backported a "matching" version to the linter script.

Looking at:

bitcoin/ci/test/01_base_install.sh

Lines 11 to 16 in 1172bc4

CFG_DONE="ci.base-install-done" # Use a global git setting to remember whether this script ran to avoid running it twice

if [ "$(git config --global ${CFG_DONE})" == "true" ]; then

echo "Skip base install"

exit 0

fi

...I mistakenly thought that .git was actually needed, but i) it's mounted RO, and ii) it's only used as above here as far as I can see in the whole CI.

Can we just touch a (any) file to persist the flag then, and drop all .git requirement?

...I mistakenly thought that .git was actually needed, but i) it's mounted RO, and ii) it's only used as above here as far as I can see in the whole CI.

It is also used in tidy via git --no-pager diff.

Can we just touch a (any) file to persist the flag then, and drop all .git requirement?

Yes, but I don't know how to do that in a one-liner cross-platform (linux+mac), taking into account the different root trees and permissions. So just using git seems simplest?

Is the (possible) running of 01_base_install.sh more than once deliberate? If so, then I'm curious why we ever do that? Is it just for the DANGER_RUN_CI_ON_HOST path or is there another use?

If not then it seems like we could just mark install as done when building the image, something like this...

It is also used in tidy via git --no-pager diff.

Does this not mean that it requires the actual .git directory to be mounted for the refs, meaning git init wouldn't work in that case?

Is the (possible) running of 01_base_install.sh more than once deliberate? If so, then I'm curious why we ever do that? Is it just for the DANGER_RUN_CI_ON_HOST path or is there another use?

It should only install once (otherwise there will be issues). Yes, it is for DANGER_RUN_CI_ON_HOST.

If not then it seems like we could just mark install as done when building the image, something like this...

lgtm. Could rename the file to /ci_base_install_done to include the ci prefix?

Does this not mean that it requires the actual .git directory to be mounted for the refs, meaning git init wouldn't work in that case?

Just for printing the diff, as long as the repo commits to the same tree (or even the staging index is the same), it should work. I see why you opted for copying .git, as it is a bit more future-proof and less confusing, given that the commit id also ends up in the binary or may otherwise be printed in the ci script? No strong opinion on how to fix it, though.

I don't think the file touch method will work for the same reasons we discussed above (needing the actual .git dir with all its objects and refs for tidy), so have not opted to go for that currently.

On a second though, I guess if someone wanted to run the CI on an extracted tarball of the repo (git-archive), it would be better to also support that. Not sure if it is as trivial as git init && git add ./, so no strong opinion.

maflcko · 2025-02-04T08:00:36Z

test/lint/run_lint.sh

+
+if [[ -n "$WORKTREE_ROOT" ]]; then
+    # If in a worktree, mount both the current directory and the git root
+    DOCKER_ARGS+=(--mount "type=bind,src=$WORKTREE_ROOT,dst=$WORKTREE_ROOT,readonly")


Would be good to explain why this is needed (with an example error log) and why this is the correct fix. My preference would be to fix it in the lint test_runner itself, so that everyone benefits from the fix, not just people running this ci bash script in a workdir.

As far as I can tell, running bash -c '( cd ./test/lint/test_runner/ && cargo fmt && cargo clippy && RUST_BACKTRACE=1 cargo run )' in a worktree already works fine for me, because the .git directory (referenced by the .git file that a worktree creates) is available to me, unlike in the containerised scenario where this path must be explicitly mounted into the container at runtime, which is what the helper script does.

I added some more comments to describe why the mount is needed in the script itself.

Apologies if I've misunderstood what you mean here.

Run CI in worktrees by mounting the .git root target into the container.

test/lint/README.md

maflcko · 2025-02-04T11:55:25Z

test/lint/run_lint.sh

+    DOCKER_ARGS+=(--mount "type=bind,src=$WORKTREE_ROOT,dst=$WORKTREE_ROOT")
+fi
+
+RUST_BACKTRACE=full docker run "${DOCKER_ARGS[@]}" bitcoin-linter


Suggested change

RUST_BACKTRACE=full docker run "${DOCKER_ARGS[@]}" bitcoin-linter

docker run "${DOCKER_ARGS[@]}" bitcoin-linter

I don't think this env var will affect anything?

Also, style-wise I wonder if the contents of this file should be put into ci/lint_run_all.sh, so that there is only one bash script, instead of multiple.

Ideally there would only be one script, used by ci and locally, but that can be a follow-up. For now, just a single large if in ci/lint_run_all.sh should be fine to switch between the cirrus_ci code paths ( current content of ci/lint_run_all.sh) and a local run (current content of this file).

Run linter in worktrees by mounting the .git root target into the container. Rename `get_git_root` to `get_git_toplevel` to avoid confusion around what this variable represents when modifying the code in the future.

fanquake · 2025-02-20T19:16:50Z

Is this still meant to be a draft?

maflcko · 2025-02-20T19:28:10Z

For reference, I can't review this, because I don't know if all the bash refactors/behavior changes here are correct and best practise, or if they could fail (and when) on error or on ancient versions of bash used in macOS.

DrahtBot · 2025-03-18T05:05:31Z

🐙 This pull request conflicts with the target branch and needs rebase.

willcl-ark · 2025-03-19T10:45:52Z

@maflcko thinking more on this after the merge of #31948 my opinion is that we should close this PR, the associated issue (#30028), and, if any action should be take it would be to document that linting in a worktree is possible by running the linter outside of a container? e.g.:

# install required python dependencies
bash -c '( cd ./test/lint/test_runner/ && cargo fmt && cargo clippy && RUST_BACKTRACE=1 cargo run )'

This change feels like it adds a lot of complexity, for little benefit (and I can't think of a simpler way to achieve the functionality desired).

What do you think?

maflcko · 2025-03-19T16:34:50Z

For the lint task, the docker wrapper is nice, because it takes care of installing all the exact versions:

bitcoin/ci/lint/04_install.sh

Lines 49 to 65 in e568c1d

    
           ${CI_RETRY_EXE} pip3 install \ 
        
             codespell==2.2.6 \ 
        
             lief==0.13.2 \ 
        
             mypy==1.4.1 \ 
        
             pyzmq==25.1.0 \ 
        
             ruff==0.5.5 \ 
        
             vulture==2.6 
        
           SHELLCHECK_VERSION=v0.8.0 
        
           curl -sL "https://github.com/koalaman/shellcheck/releases/download/${SHELLCHECK_VERSION}/shellcheck-${SHELLCHECK_VERSION}.linux.x86_64.tar.xz" | \ 
        
               tar --xz -xf - --directory /tmp/ 
        
           mv "/tmp/shellcheck-${SHELLCHECK_VERSION}/shellcheck" /usr/bin/ 
        
           MLC_VERSION=v0.19.0 
        
           MLC_BIN=mlc-x86_64-linux 
        
           curl -sL "https://github.com/becheran/mlc/releases/download/${MLC_VERSION}/${MLC_BIN}" -o "/usr/bin/mlc" 
        
           chmod +x /usr/bin/mlc

However, given the limited feedback, I wonder if anyone cares about running the linters in a worktree.

For the "real" CI tasks, at least on person complained on IRC IIRC, so at least there seems to be some demand.

DrahtBot · 2025-06-16T00:04:20Z

⌛ There hasn't been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

willcl-ark · 2025-06-16T08:36:44Z

Closing due to lack of interest.

maflcko · 2025-06-17T18:32:39Z

Looks like this was closed, so I opened a trivial line-neutral +4-4 fix in #32767

(Missing lint and tidy support, but this can be done in a follow-up, if needed)

DrahtBot added the Tests label Feb 3, 2025

maflcko reviewed Feb 3, 2025

View reviewed changes

DrahtBot added the CI failed label Feb 3, 2025

maflcko reviewed Feb 4, 2025

View reviewed changes

willcl-ark force-pushed the worktree-qol branch from 187ee84 to 95207aa Compare February 4, 2025 11:04

ci: run in worktrees

2c33704

Run CI in worktrees by mounting the .git root target into the container.

maflcko reviewed Feb 4, 2025

View reviewed changes

test/lint/README.md Outdated Show resolved Hide resolved

willcl-ark force-pushed the worktree-qol branch from 95207aa to 1a67538 Compare February 4, 2025 11:20

maflcko reviewed Feb 4, 2025

View reviewed changes

DrahtBot removed the CI failed label Feb 4, 2025

lint: run in worktrees

cc67e45

Run linter in worktrees by mounting the .git root target into the container. Rename `get_git_root` to `get_git_toplevel` to avoid confusion around what this variable represents when modifying the code in the future.

willcl-ark force-pushed the worktree-qol branch from 1a67538 to cc67e45 Compare February 4, 2025 12:16

This was referenced Feb 21, 2025

ci: detect outbound internet traffic generated while running tests #31349

Open

ci: [lint] Use Cirrus dockerfile cache #31948

Merged

ci: limit max stack size to 512 KiB #31367

Closed

DrahtBot added the Needs rebase label Mar 18, 2025

willcl-ark closed this Jun 16, 2025

willcl-ark mentioned this pull request Jun 20, 2025

ci: Allow running CI in worktrees #32767

Merged

This was referenced Jun 20, 2025

followup: ci: run in worktrees kevkevinpal/bitcoin#47

Closed

try to add git init ./ && git add ./ may be a easy one-line fix kevkevinpal/bitcoin#57

Open

	CFG_DONE="ci.base-install-done" # Use a global git setting to remember whether this script ran to avoid running it twice

	if [ "$(git config --global ${CFG_DONE})" == "true" ]; then
	echo "Skip base install"
	exit 0
	fi

	RUST_BACKTRACE=full docker run "${DOCKER_ARGS[@]}" bitcoin-linter
	docker run "${DOCKER_ARGS[@]}" bitcoin-linter

ci: run in worktrees #31787

ci: run in worktrees #31787

Uh oh!

Conversation

willcl-ark commented Feb 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Feb 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fanquake commented Feb 20, 2025

Uh oh!

maflcko commented Feb 20, 2025

Uh oh!

DrahtBot commented Mar 18, 2025

Uh oh!

willcl-ark commented Mar 19, 2025

Uh oh!

maflcko commented Mar 19, 2025

Uh oh!

DrahtBot commented Jun 16, 2025

Uh oh!

willcl-ark commented Jun 16, 2025

Uh oh!

maflcko commented Jun 17, 2025

Uh oh!

Uh oh!

willcl-ark commented Feb 3, 2025 •

edited

Loading

DrahtBot commented Feb 3, 2025 •

edited

Loading