cmake: Create subdirectories in build tree in advance #32773

hebasto · 2025-06-18T19:52:41Z

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

DrahtBot · 2025-06-18T19:52:44Z

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	m3dwards
Stale ACK	BrandonOdiwuor, janb84

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

Conflicts

No conflicts as of last run.

maflcko · 2025-06-18T20:01:48Z

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?

hebasto · 2025-06-19T09:31:30Z

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.

janb84 · 2025-06-23T17:37:36Z

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 )

purpleKarrot · 2025-06-24T10:01:14Z

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.

hebasto · 2025-07-03T13:16:31Z

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.

janb84

ACK 0b7d038

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

code review ✅
building & tests ✅

@hebasto Thank you for supplying the additional information how to test the PR.

BrandonOdiwuor

Tested ACK 0b7d038

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

Commit: 0b7d038

Branch: Master

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

This change ensures that subsequent symlink creation does not fail due to a missing path.

hebasto · 2025-07-17T15:43:37Z

Rebased to resolve conflict with merged #32881.

janb84

re ACK fa3beb6

Changes sinds last ACK:

Solved Merge conflict.

Retested, still worked as described. ✅
Code review ✅

test/CMakeLists.txt

hebasto · 2025-11-04T21:14:05Z

The feedback from @m3dwards has been addressed.

m3dwards · 2025-11-07T12:37:28Z

ACK 76dae5d

Tested on arm Mac.

fanquake · 2025-11-07T12:39:14Z

cc @purpleKarrot

purpleKarrot · 2025-11-07T15:48:50Z

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.

maflcko · 2025-11-07T16:24:15Z

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.

purpleKarrot · 2025-11-07T17:11:23Z

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

maflcko · 2025-11-07T17:22:17Z

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.

hebasto added Build system Tests labels Jun 18, 2025

DrahtBot mentioned this pull request Jun 19, 2025

test: Turn util/test_runner into functional test #32697

Merged

DrahtBot added the Needs rebase label Jun 30, 2025

hebasto force-pushed the 250618-mkdir branch from a1255bf to 0b7d038 Compare July 3, 2025 13:08

janb84 reviewed Jul 3, 2025

View reviewed changes

DrahtBot removed the Needs rebase label Jul 3, 2025

DrahtBot mentioned this pull request Jul 5, 2025

test: Turn rpcauth.py test into functional test #32881

Merged

DrahtBot added the Needs rebase label Jul 10, 2025

BrandonOdiwuor reviewed Jul 11, 2025

View reviewed changes

cmake: Create subdirectories in build tree in advance

88d9092

This change ensures that subsequent symlink creation does not fail due to a missing path.

hebasto force-pushed the 250618-mkdir branch from 0b7d038 to fa3beb6 Compare July 17, 2025 15:41

DrahtBot removed the Needs rebase label Jul 17, 2025

janb84 reviewed Jul 18, 2025

View reviewed changes

DrahtBot requested a review from BrandonOdiwuor July 18, 2025 08:44

achow101 requested review from m3dwards and removed request for BrandonOdiwuor October 22, 2025 15:41

m3dwards reviewed Nov 4, 2025

View reviewed changes

test/CMakeLists.txt Outdated Show resolved Hide resolved

DrahtBot requested a review from BrandonOdiwuor November 4, 2025 17:09

cmake: Replace recursive globbing with explicit globbing in folders

76dae5d

hebasto force-pushed the 250618-mkdir branch from fa3beb6 to 76dae5d Compare November 4, 2025 21:13

DrahtBot requested a review from janb84 November 7, 2025 12:37

cmake: Create subdirectories in build tree in advance #32773

Are you sure you want to change the base?

cmake: Create subdirectories in build tree in advance #32773

Conversation

hebasto commented Jun 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jun 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

maflcko commented Jun 18, 2025

Uh oh!

hebasto commented Jun 19, 2025

Uh oh!

janb84 commented Jun 23, 2025

Uh oh!

purpleKarrot commented Jun 24, 2025

Uh oh!

hebasto commented Jul 3, 2025

Uh oh!

janb84 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BrandonOdiwuor left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto commented Jul 17, 2025

Uh oh!

janb84 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hebasto commented Nov 4, 2025

Uh oh!

m3dwards commented Nov 7, 2025

Uh oh!

fanquake commented Nov 7, 2025

Uh oh!

purpleKarrot commented Nov 7, 2025

Uh oh!

maflcko commented Nov 7, 2025

Uh oh!

purpleKarrot commented Nov 7, 2025

Uh oh!

maflcko commented Nov 7, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

hebasto commented Jun 18, 2025 •

edited

Loading

DrahtBot commented Jun 18, 2025 •

edited

Loading

janb84 left a comment •

edited

Loading

BrandonOdiwuor left a comment •

edited

Loading