ci: Allow build dir on CI host #31428

pull maflcko wants to merge 1 commits into bitcoin:master from maflcko:2412-ci-build changing 2 files +11 −7
  1. 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.

  2. 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, davidgumberg

    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.

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

  5. DrahtBot added the label CI failed on Dec 5, 2024
  6. maflcko force-pushed on Dec 5, 2024
  7. maflcko force-pushed on Dec 5, 2024
  8. DrahtBot removed the label CI failed on Dec 5, 2024
  9. hebasto commented at 11:19 am on December 6, 2024: member
    Concept ACK.
  10. maflcko force-pushed on Dec 18, 2024
  11. 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"
  12. 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.
  13. hebasto commented at 11:15 am on December 20, 2024: member
    Approach ACK faf15ef3a78f6ee5ef5fb9468e9de04b5fca6b21, tested with #31176 built on top of this PR.
  14. maflcko force-pushed on Dec 20, 2024
  15. maflcko commented at 12:06 pm on December 20, 2024: member
    force pushed with some minor style-only fixups
  16. hebasto approved
  17. hebasto commented at 12:09 pm on December 20, 2024: member
    ACK fac0ab3286888c2eab674f6814ba8006e3a850a8.
  18. in ci/test/02_run_container.sh:103 in fac0ab3286 outdated 
     99@@ -97,6 +100,7 @@ if [ -z "$DANGER_RUN_CI_ON_HOST" ]; then
100                   --mount "${CI_DEPENDS_MOUNT}" \
101                   --mount "${CI_DEPENDS_SOURCES_MOUNT}" \
102                   --mount "${CI_PREVIOUS_RELEASES_MOUNT}" \
103+                  ${CI_BUILD_MOUNT} \
    davidgumberg commented at 4:57 am on December 25, 2024:
    feel-free-to-disregard: Is there a reason to make $CI_BUILD_MOUNT inconsistent (including --mount) with the other mount vars?
    maflcko commented at 7:43 am on December 25, 2024:
    Yes, it is not possible without major changes, see #31428 (review)
  19. ci: Allow build dir on CI host 8888ee4403
  20. in ci/test/02_run_container.sh:47 in fac0ab3286 outdated 
    43@@ -44,18 +44,21 @@ if [ -z "$DANGER_RUN_CI_ON_HOST" ]; then
44   CI_DEPENDS_MOUNT="type=volume,src=${CONTAINER_NAME}_depends,dst=$DEPENDS_DIR/built"
45   CI_DEPENDS_SOURCES_MOUNT="type=volume,src=${CONTAINER_NAME}_depends_sources,dst=$DEPENDS_DIR/sources"
46   CI_PREVIOUS_RELEASES_MOUNT="type=volume,src=${CONTAINER_NAME}_previous_releases,dst=$PREVIOUS_RELEASES_DIR"
47+  CI_BUILD_MOUNT=""
    davidgumberg commented at 5:29 am on December 25, 2024:

    feel-free-to-disregard-followup: After thinking more about my comment below, (https://github.com/bitcoin/bitcoin/pull/31428/files#r1897104374) It is because we might not use any volume mount for the build dir, and we want to be able to leave it blank here.

    But I’m wondering why the build directory doesn’t get it’s own volume, why doesn’t the same reasoning from #27033 apply here?

    maflcko commented at 7:46 am on December 25, 2024:

    But I’m wondering why the build directory doesn’t get it’s own volume, why doesn’t the same reasoning from #27033 apply here?

    The build output is not really a cache for the next build, but rather the result of the build, so a volume doesn’t help. In fact it can only be overwritten by DANGER_CI_ON_HOST_FOLDERS, where the goal is to use a folder, not a volume.

  21. maflcko force-pushed on Jan 6, 2025
  22. maflcko commented at 9:51 am on January 6, 2025: member
    rebased for fresh CI, no code change.
  23. hebasto approved
  24. hebasto commented at 10:41 am on January 6, 2025: member

    re-ACK 8888ee4403fa62972c49e18752695d15fd32c0b0.

    Only rebased since my recent review. Verified with:

    0git range-diff master fac0ab3286888c2eab674f6814ba8006e3a850a8 8888ee4403fa62972c49e18752695d15fd32c0b0
  25. maflcko requested review from davidgumberg on Jan 13, 2025
  26. davidgumberg commented at 4:46 pm on January 13, 2025: contributor
    lgtm ACK https://github.com/bitcoin/bitcoin/commit/8888ee4403fa62972c49e18752695d15fd32c0b0


maflcko DrahtBot hebasto davidgumberg


davidgumberg

Labels
Tests

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-01-21 06:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me