cmake: Create subdirectories in build tree in advance #32773

pull hebasto wants to merge 2 commits into bitcoin:master from hebasto:250618-mkdir changing 1 files +62 −14
  1. hebasto commented at 7:52 PM on June 18, 2025: member

    While reviewing #32697, I noticed that symlink creation fails when the target subdirectory does not exist. In such cases, file(CREATE_LINK ... COPY_ON_ERROR SYMBOLIC) falls back to copying, which implicitly creates the required path. As a result, a single file is copied instead of symlinked for subdirectories in test/functional.

    This PR ensures that necessary subdirectories are created in advance, so that subsequent symlink creation does not fail due to missing paths.

    For example:

    • on the master branch:
    $ cmake -B build
$ ls -l build/test/functional/mocks/
total 8
-rwxrwxr-x 1 hebasto hebasto 2683 Jul  3 14:11 invalid_signer.py
lrwxrwxrwx 1 hebasto hebasto   64 Jul  3 14:11 multi_signers.py -> /home/hebasto/dev/bitcoin/test/functional/mocks/multi_signers.py
lrwxrwxrwx 1 hebasto hebasto   60 Jul  3 14:11 no_signer.py -> /home/hebasto/dev/bitcoin/test/functional/mocks/no_signer.py
lrwxrwxrwx 1 hebasto hebasto   57 Jul  3 14:11 signer.py -> /home/hebasto/dev/bitcoin/test/functional/mocks/signer.py
    • with this PR:
    $ cmake -B build
$ ls -l build/test/functional/mocks/
total 4
lrwxrwxrwx 1 hebasto hebasto 65 Jul  3 13:51 invalid_signer.py -> /home/hebasto/dev/bitcoin/test/functional/mocks/invalid_signer.py
lrwxrwxrwx 1 hebasto hebasto 64 Jul  3 13:51 multi_signers.py -> /home/hebasto/dev/bitcoin/test/functional/mocks/multi_signers.py
lrwxrwxrwx 1 hebasto hebasto 60 Jul  3 13:51 no_signer.py -> /home/hebasto/dev/bitcoin/test/functional/mocks/no_signer.py
lrwxrwxrwx 1 hebasto hebasto 57 Jul  3 13:51 signer.py -> /home/hebasto/dev/bitcoin/test/functional/mocks/signer.py
  2. hebasto added the label Build system on Jun 18, 2025
  3. hebasto added the label Tests on Jun 18, 2025
  4. DrahtBot commented at 7:52 PM on June 18, 2025: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--006a51241073e994b41acfe9ec718e94-->

    Code Coverage & Benchmarks

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

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK m3dwards, sedited, janb84
    Stale ACK BrandonOdiwuor

    If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    No conflicts as of last run.

    <!--5faf32d7da4f0f540f40219e4f7537a3-->

  5. maflcko commented at 8:01 PM on June 18, 2025: member

    would it make sense to at least replace the recursive glob with a normal glob now, iterating over the given folders, given that they are listed anyway?

  6. hebasto commented at 9:31 AM on June 19, 2025: member

    would it make sense to at least replace the recursive glob with a normal glob now, iterating over the given folders, given that they are listed anyway?

    Sure! Added a commit.

  7. janb84 commented at 5:37 PM on June 23, 2025: contributor

    Any hints how to verify / test this ?

    Have tried to see any difference between this PR and master but it's not clear to me what difference I should see. When used on #32697 there is a difference that without this PR build/test/functional/data/util/* is populated with this PR the util directory is not being created (looking at the source this is logical but unsure if there needs to be things done after 32697 is merged or not having the directory is intentional )

  8. purpleKarrot commented at 10:01 AM on June 24, 2025: contributor

    Is this setting up the environment for tests to run? Ideally, things like that should be done as test fixture and not executed during build system generation.

  9. DrahtBot added the label Needs rebase on Jun 30, 2025
  10. hebasto force-pushed on Jul 3, 2025
  11. hebasto commented at 1:16 PM on July 3, 2025: member

    Rebased. @janb84

    Any hints how to verify / test this ?

    Have tried to see any difference between this PR and master but it's not clear to me what difference I should see. When used on #32697 there is a difference that without this PR build/test/functional/data/util/* is populated with this PR the util directory is not being created (looking at the source this is logical but unsure if there needs to be things done after 32697 is merged or not having the directory is intentional )

    I've updated the PR description with the relevant example. @purpleKarrot

    Is this setting up the environment for tests to run? Ideally, things like that should be done as test fixture and not executed during build system generation.

    At this moment, functional tests are not managed by ctest/cmake.

  12. janb84 commented at 2:20 PM on July 3, 2025: contributor

    ACK 0b7d038972315b5537cc42038094902ebd5dd8cf

    This PR changes :

    • Recursive Glob -> 'explicit' non recursive GLOB
    • Subdirectories now are created in advance, so that there aren't any problems with missing paths for sym-linking.

    The PR fixes a "bug" and the code changes make the code communicate the intentions of the code better.

    Can reproduce the successful subfolder creation and therefor file sym-linking :

    <details> Master (fails see `invalid_signer.py` ):

    $ ls -l
total 4
-rwxr-xr-x 1 jan staff 2683 Jul  3 15:57 invalid_signer.py
lrwxr-xr-x 1 jan staff   73 Jul  3 15:57 multi_signers.py -> /Users/jan/Projects/bitcoin/test/functional/mocks/multi_signers.py
lrwxr-xr-x 1 jan staff   69 Jul  3 15:57 no_signer.py -> /Users/jan/Projects/bitcoin/test/functional/mocks/no_signer.py
lrwxr-xr-x 1 jan staff   66 Jul  3 15:57 signer.py -> /Users/jan/Projects/bitcoin/test/functional/mocks/signer.py

    This PR (works see invalid_signer.py):

    ls -l
total 0
lrwxr-xr-x 1 jan staff 74 Jul  3 16:05 invalid_signer.py -> /Users/jan/Projects/bitcoin/test/functional/mocks/invalid_signer.py
lrwxr-xr-x 1 jan staff 73 Jul  3 16:05 multi_signers.py -> /Users/jan/Projects/bitcoin/test/functional/mocks/multi_signers.py
lrwxr-xr-x 1 jan staff 69 Jul  3 16:05 no_signer.py -> /Users/jan/Projects/bitcoin/test/functional/mocks/no_signer.py
lrwxr-xr-x 1 jan staff 66 Jul  3 16:05 signer.py -> /Users/jan/Projects/bitcoin/test/functional/mocks/signer.py

    </details>

    • code review ✅
    • building & tests ✅ @hebasto Thank you for supplying the additional information how to test the PR.
  13. DrahtBot removed the label Needs rebase on Jul 3, 2025
  14. DrahtBot added the label Needs rebase on Jul 10, 2025
  15. BrandonOdiwuor commented at 6:06 PM on July 11, 2025: contributor

    Tested ACK 0b7d038972315b5537cc42038094902ebd5dd8cf

    Confirmed that the subdirectories are created in the build tree before the symlinks preventing accidental COPY_ON_ERRORs like invalid_signer.py

    Commit: 0b7d038972315b5537cc42038094902ebd5dd8cf <img width="1274" height="103" alt="Screenshot 2025-07-11 at 20 51 32" src="https://github.com/user-attachments/assets/16f03539-b0e1-4d62-941b-33a34061355a" />

    Branch: Master <img width="1311" height="110" alt="Screenshot 2025-07-11 at 20 50 40" src="https://github.com/user-attachments/assets/0cc42fda-b6a6-46fc-9916-c720937346f0" />

    I also agree that using explicit file(GLOB ...) for specific file types (e.g., *.py, *.json, *.csv, *.html) is more robust than the original file(GLOB_RECURSE ...)

  16. cmake: Create subdirectories in build tree in advance
    This change ensures that subsequent symlink creation does not fail due
to a missing path.
    88d9092571
  17. hebasto force-pushed on Jul 17, 2025
  18. hebasto commented at 3:43 PM on July 17, 2025: member

    Rebased to resolve conflict with merged #32881.

  19. DrahtBot removed the label Needs rebase on Jul 17, 2025
  20. janb84 commented at 8:44 AM on July 18, 2025: contributor

    re ACK fa3beb6316d426b7e388d71fa2f09aed4d2da3d2

    Changes sinds last ACK:

    • Solved Merge conflict.

    Retested, still worked as described. ✅ Code review ✅

  21. DrahtBot requested review from BrandonOdiwuor on Jul 18, 2025
  22. achow101 removed review request from BrandonOdiwuor on Oct 22, 2025
  23. achow101 requested review from m3dwards on Oct 22, 2025
  24. in test/CMakeLists.txt:77 in fa3beb6316 
      86 | +    functional/test_framework/crypto/*.csv
  87 | +    functional/test_framework/crypto/*.py
  88 | +  )
  89 | +
  90 | +  file(MAKE_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/fuzz)
  91 | +  file(MAKE_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/util)
    m3dwards commented at 5:09 PM on November 4, 2025:

    I think this line can be dropped now test/util is no longer there since #32697?

    hebasto commented at 9:09 PM on November 4, 2025:

    I'd say since the following #32881 :)

    hebasto commented at 9:13 PM on November 4, 2025:

    Thanks! Fixed.

  25. DrahtBot requested review from BrandonOdiwuor on Nov 4, 2025
  26. cmake: Replace recursive globbing with explicit globbing in folders 76dae5d691
  27. hebasto force-pushed on Nov 4, 2025
  28. hebasto commented at 9:14 PM on November 4, 2025: member

    The feedback from @m3dwards has been addressed.

  29. m3dwards commented at 12:37 PM on November 7, 2025: contributor

    ACK 76dae5d6911b600fafa3a417a740f14b299284f3

    Tested on arm Mac.

  30. DrahtBot requested review from janb84 on Nov 7, 2025
  31. fanquake commented at 12:39 PM on November 7, 2025: member

    cc @purpleKarrot

  32. purpleKarrot commented at 3:48 PM on November 7, 2025: contributor

    My position is that code like this is not something that belongs in a CMakeLists.txt file.

    Ideally, tests should have their environment set up in a way that they find necessary files in the source tree, without any symlinks or copies.

    If directories/symlinks are needed by tests that are executed with ctest, a temporary workaround could be to create them in a test fixture, like I mentioned here.

    As long as functional tests are launched by some other means (like a shell script) the directories and symlinks should be created by that script.

  33. maflcko commented at 4:24 PM on November 7, 2025: member

    Ideally, tests should have their environment set up in a way that they find necessary files in the source tree, without any symlinks or copies.

    I think there is no explanation in the code, but the reason why the copies/symlinks are created is that it keeps the source dir clean of .pyc files.

    If directories/symlinks are needed by tests that are executed with ctest, a temporary workaround could be to create them in a test fixture, like I mentioned here.

    They are not executed with ctest, so a fixture can not be used.

    As long as functional tests are launched by some other means (like a shell script) the directories and symlinks should be created by that script.

    The functional tests are launched by a runner. However, it is also possible to run individual tests. So they are stand-alone scripts and so they can't create their own symlink.

  34. purpleKarrot commented at 5:11 PM on November 7, 2025: contributor

    I think there is no explanation in the code, but the reason why the copies/symlinks are created is that it keeps the source dir clean of .pyc files.

    The generation of .pyc files can be suppressed with python's -B option or PYTHONDONTWRITEBYTECODE env var. See: https://docs.python.org/3/using/cmdline.html#envvar-PYTHONDONTWRITEBYTECODE

  35. maflcko commented at 5:22 PM on November 7, 2025: member

    I know, but -B does not help to fix the other problems: (1) They are not run via ctest, and (2) they are stand-alone scripts, where the user/dev would have to specify it every time, or set the env var.

  36. sedited approved
  37. sedited commented at 8:26 AM on February 9, 2026: contributor

    ACK 76dae5d6911b600fafa3a417a740f14b299284f3

  38. janb84 commented at 9:16 AM on February 9, 2026: contributor

    re ACK 76dae5d6911b600fafa3a417a740f14b299284f3

    changes made since last ack:

    • dropped folder reaction following #32881

    Given we are not (yet) running these tests via CTest and the reasons given by @maflcko. This fixes some of the issues we have with this way of working, good (fix for now.) Therefor I still think we should merge and explore alternatives later.

  39. fanquake merged this on Feb 9, 2026
  40. fanquake closed this on Feb 9, 2026
  41. hebasto deleted the branch on Feb 9, 2026
Contributors
hebastoDrahtBotmaflckojanb84purpleKarrotBrandonOdiwuorm3dwardsfanquakesedited


BrandonOdiwuor

Labels
Build systemTests
Linked (view graph)
#32697 test: Turn util/test_runner into functional test
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: 2026-04-24 21:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me