bitcoin icon indicating copy to clipboard operation
bitcoin copied to clipboard

ci: run in worktrees

Open willcl-ark opened this issue 10 months ago • 7 comments

Fixes: #30028

  • 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.

willcl-ark avatar Feb 03 '25 16:02 willcl-ark

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.

DrahtBot avatar Feb 03 '25 16:02 DrahtBot

Is this still meant to be a draft?

fanquake avatar Feb 20 '25 19:02 fanquake

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.

maflcko avatar Feb 20 '25 19:02 maflcko

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

DrahtBot avatar Mar 18 '25 05:03 DrahtBot

@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?

willcl-ark avatar Mar 19 '25 10:03 willcl-ark

For the lint task, the docker wrapper is nice, because it takes care of installing all the exact versions: https://github.com/bitcoin/bitcoin/blob/e568c1dd134e0318c46113cb7dfc23b40e2829e8/ci/lint/04_install.sh#L49-L65

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.

maflcko avatar Mar 19 '25 16:03 maflcko

⌛ 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.

DrahtBot avatar Jun 16 '25 00:06 DrahtBot

Closing due to lack of interest.

willcl-ark avatar Jun 16 '25 08:06 willcl-ark

Looks like this was closed, so I opened a trivial line-neutral +4-4 fix in https://github.com/bitcoin/bitcoin/pull/32767

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

maflcko avatar Jun 17 '25 18:06 maflcko