Skip to content

lint: Use consistent out-of-tree build for python and test_runner#30499

Merged
fanquake merged 3 commits intobitcoin:masterfrom
maflcko:2407-lint-fixes
Jul 22, 2024
Merged

lint: Use consistent out-of-tree build for python and test_runner#30499
fanquake merged 3 commits intobitcoin:masterfrom
maflcko:2407-lint-fixes

Conversation

@maflcko
Copy link
Copy Markdown
Member

@maflcko maflcko commented Jul 22, 2024

Fixes #30496

Seems odd to sometimes do an out-of-tree build (via ./ci/lint_imagefile, see test/lint/README.md) and sometimes not (via Cirrus CI, see ./ci/lint_run_all.sh).

Fix it by doing an out-of-tree build consistently in the same location.

Also, fix $CI_RETRY_EXE, while touching this.

MarcoFalke added 2 commits July 22, 2024 13:20
Previous code was confusing and brittle. For example, the full import
"source ./ci/test/00_setup_env.sh" and $PATH overwrite was not needed.

Fix it by simply copying the exe to /ci_retry and use that in
$CI_RETRY_EXE.

This is also a fix, because previously ci/lint_imagefile did use an
empty $CI_RETRY_EXE.
@DrahtBot
Copy link
Copy Markdown
Contributor

DrahtBot commented Jul 22, 2024

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK paplorinc, josibake, willcl-ark

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

@DrahtBot DrahtBot changed the title lint: Use consistent out-of-tree build for python and test_runner lint: Use consistent out-of-tree build for python and test_runner Jul 22, 2024
@DrahtBot DrahtBot added the Tests label Jul 22, 2024
@DrahtBot
Copy link
Copy Markdown
Contributor

🚧 At least one of the CI tasks failed.
Debug: https://github.com/bitcoin/bitcoin/runs/27745036545

Hints

Make sure to run all tests locally, according to the documentation.

The failure may 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.

@maflcko maflcko force-pushed the 2407-lint-fixes branch 3 times, most recently from 0421a7f to fac504d Compare July 22, 2024 11:53
This mirrors the build by ./ci/lint_imagefile, which is done out-of-tree
in "/".

Otherwise, there could be errors due to a dirty tree.
@l0rinc
Copy link
Copy Markdown
Contributor

l0rinc commented Jul 22, 2024

Thanks for fixing this so quickly!
I've added #30500 to fix the unrelated warnings that made it slightly more difficult to see what the underlying problem was here.

@maflcko
Copy link
Copy Markdown
Member Author

maflcko commented Jul 22, 2024

lint is green now, sorry for the force pushes, but this can now be reviewed and tested.

Copy link
Copy Markdown
Contributor

@l0rinc l0rinc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK fa8d73e

@@ -62,3 +64,5 @@ MLC_VERSION=v0.18.0
MLC_BIN=mlc-x86_64-linux
curl -sL "https://github.com/becheran/mlc/releases/download/${MLC_VERSION}/${MLC_BIN}" -o "/usr/bin/mlc"
Copy link
Copy Markdown
Contributor

@l0rinc l0rinc Jul 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated, but shouldn't every git clone have --depth=1 throughout this file?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may do this in a follow-up

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in #30501

@josibake
Copy link
Copy Markdown
Member

utACK fa8d73e

ran DOCKER_BUILDKIT=1 docker build -t bitcoin-linter --file "./ci/lint_imagefile" ./ && docker run --rm -v $(pwd):/bitcoin -it bitcoin-linter, more as a sanity check since this isn't really testing the underlying issue this PR is fixing.

@willcl-ark
Copy link
Copy Markdown
Member

utACK fa8d73e

Thanks for addressing @maflcko, this makes things much more sanitary.

That said, our mlc lint is supposed to exclude all files/dirs which are not tracked by git, by calling git ls-files --ignored --others --exclude-standard.

It appears that the --ignored flag causes some directories/files to not appear in this listing, leading to the failure:

will@ubuntu in ~/src/bitcoin on  2407-lint-fixes [$?] : 🐍 (bitcoin)
$ git ls-files --ignored --others --exclude-standard | rg pyenv

will@ubuntu in ~/src/bitcoin on  2407-lint-fixes [$?] : 🐍 (bitcoin)
✗ git ls-files --others --exclude-standard | rg pyenv
.pyenv/readme.md

I'll try an upstream fix so that we can avoid a future recurrence (with another file/dir)

@maflcko
Copy link
Copy Markdown
Member Author

maflcko commented Jul 22, 2024

That said, our mlc lint is supposed to exclude all files/dirs which are not tracked by git, by calling git ls-files --ignored --others --exclude-standard.

Yes, I know, but someone will need to create a pull request to pull in the mlc version that fixes this, if such a version exists.

I think the consistency fixes here make sense either way. Fixing the bug is just a convenient side effect.

@fanquake fanquake merged commit 98537a0 into bitcoin:master Jul 22, 2024
@maflcko maflcko deleted the 2407-lint-fixes branch July 22, 2024 15:21
fanquake added a commit that referenced this pull request Jul 22, 2024
fa7bee1 lint: Use git clone --depth=1 (MarcoFalke)
fadb7c2 lint: Add missing docker.io prefix to ci/lint_imagefile (MarcoFalke)

Pull request description:

  Currently, the `ci/lint_imagefile` may pick the wrong (non-native) architecture due to the missing prefix.

  For example, assuming the user has previously pulled an s390x image:

  ```
  $ podman run --rm 'docker.io/s390x/debian:bookworm' dpkg --print-architecture
  exec /usr/bin/dpkg: exec format error
  ```

  Now, `debian:bookworm` will refer to the same image:

  ```
  $ podman run --rm 'debian:bookworm' dpkg --print-architecture
  exec /usr/bin/dpkg: exec format error
  ```

  However, `docker.io/debian:bookworm` works fine:

  ```
   $ podman run --rm 'docker.io/debian:bookworm' dpkg --print-architecture
  arm64
  ```

  (Also includes a nit-fix from #30499 (comment))

ACKs for top commit:
  paplorinc:
    utACK fa7bee1
  hebasto:
    ACK fa7bee1.

Tree-SHA512: 4b6d562c14c67bef984ad25f6a3a1ef7f1059dc2859c603c45083b36bcacafa3248fc74176e2e4626fdc39507e9353f458ddbc4077f805c03e970df46af02224
@bitcoin bitcoin locked and limited conversation to collaborators Jul 22, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Spurious markdown linter failure

7 participants