ci: Allow build dir on CI host

maflcko commented at 2:20 pm on December 5, 2024: member

This is required to pass cross builds on to a different machine after the build.

See for example #31176, but this pull will also allow someone to implement it outside this repo.

DrahtBot commented at 2:20 pm on December 5, 2024: contributor

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/31428.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	hebasto

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#31367 (ci: limit max stack size to 512 KiB by dergoegge)

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 added the label Tests on Dec 5, 2024

DrahtBot commented at 3:31 pm on December 5, 2024: contributor

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

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.

DrahtBot added the label CI failed on Dec 5, 2024

maflcko force-pushed on Dec 5, 2024

DrahtBot removed the label CI failed on Dec 5, 2024

hebasto commented at 11:19 am on December 6, 2024: member

Concept ACK.

maflcko force-pushed on Dec 18, 2024

in .github/workflows/ci.yml:278 in faf15ef3a7 outdated

279-        run: echo "BASE_ROOT_DIR=${RUNNER_TEMP}" >> "$GITHUB_ENV"
280+      - name: Set CI directories
281+        run: |
282+          echo "CCACHE_DIR=${RUNNER_TEMP}/ccache_dir" >> "$GITHUB_ENV"
283+          echo "BASE_ROOT_DIR=${RUNNER_TEMP}" >> "$GITHUB_ENV"
284+          echo "BASE_BUILD_DIR=${RUNNER_TEMP}}/build-asan" >> "$GITHUB_ENV"

hebasto commented at 11:11 am on December 20, 2024:

An extra brace:

0          echo "BASE_BUILD_DIR=${RUNNER_TEMP}/build-asan" >> "$GITHUB_ENV"

?

hebasto commented at 11:11 am on December 20, 2024:

nit: I slightly prefer ${{ runner.temp }} over ${RUNNER_TEMP} because it expands in the log, which feels convenient.

maflcko commented at 11:27 am on December 20, 2024:

Not sure what you mean, for me it is already expanded/evaluated, see https://github.com/bitcoin/bitcoin/pull/31428/checks#step:6:11

hebasto commented at 11:37 am on December 20, 2024:

I mean, expanded in a log for the current step, like this:

0Run echo "CCACHE_DIR=/home/runner/work/_temp/ccache_dir" >> "$GITHUB_ENV"
1  echo "CCACHE_DIR=/home/runner/work/_temp/ccache_dir" >> "$GITHUB_ENV"
2  echo "BASE_ROOT_DIR=/home/runner/work/_temp" >> "$GITHUB_ENV"
3  echo "DEPENDS_DIR=/home/runner/work/_temp/depends" >> "$GITHUB_ENV"
4  echo "BASE_BUILD_DIR=/home/runner/work/_temp/build" >> "$GITHUB_ENV"

in ci/test/02_run_container.sh:55 in faf15ef3a7 outdated

51     # ensure the directories exist
52     mkdir -p "${CCACHE_DIR}"
53     mkdir -p "${DEPENDS_DIR}/built"
54     mkdir -p "${DEPENDS_DIR}/sources"
55     mkdir -p "${PREVIOUS_RELEASES_DIR}"
56+    mkdir -p "${BASE_BUILD_DIR}"

hebasto commented at 11:13 am on December 20, 2024:

Shouldn’t the definition of BASE_BUILD_DIR from https://github.com/bitcoin/bitcoin/blob/faf15ef3a78f6ee5ef5fb9468e9de04b5fca6b21/ci/test/03_test_script.sh#L118-L119

be moved before this first usage?

maflcko commented at 11:25 am on December 20, 2024:

It requires the caller to define it, which is intentional. Should I add a comment explaining it?

hebasto commented at 11:40 am on December 20, 2024:

It requires the caller to define it, which is intentional.

Why? What happens if the default value will be used? (especially, if a CI job doesn’t care about it)

Should I add a comment explaining it?

If it helps :)

maflcko commented at 11:48 am on December 20, 2024:

It requires the caller to define it, which is intentional.

Why? What happens if the default value will be used? (especially, if a CI job doesn’t care about it)

My understanding is that it would fail otherwise. I suspect the error message will be:

0root@7b4d00222340:/# mkdir -p "${BASE_BUILD_DIR}"
1mkdir: cannot create directory '': No such file or directory
2root@7b4d00222340:/# mkdir -p ""                 
3mkdir: cannot create directory '': No such file or directory

hebasto commented at 11:57 am on December 20, 2024:

Right. That’s why I suggest to move BASE_BUILD_DIR=${BASE_BUILD_DIR:-$BASE_SCRATCH_DIR/build-$HOST} before.

maflcko commented at 12:01 pm on December 20, 2024:

Options that start with DANGER_ require in-depth knowledge of the inner workings of the CI system. Trying to make the existing code harder to understand for others seems suboptimal to solve a problem that doesn’t really exist in practise. If there is a strong use-case, I am happy to reconsider, but absent that I don’t see the value in making the code more complex.

hebasto commented at 12:03 pm on December 20, 2024:

Let’s consider this resolved.

hebasto commented at 11:15 am on December 20, 2024: member

Approach ACK faf15ef3a78f6ee5ef5fb9468e9de04b5fca6b21, tested with #31176 built on top of this PR.

ci: Allow build dir on CI host fac0ab3286

maflcko force-pushed on Dec 20, 2024

maflcko commented at 12:06 pm on December 20, 2024: member

force pushed with some minor style-only fixups

hebasto approved

hebasto commented at 12:09 pm on December 20, 2024: member

ACK fac0ab3286888c2eab674f6814ba8006e3a850a8.

ci: Allow build dir on CI host #31428

Code Coverage & Benchmarks

Reviews

Conflicts