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	janb84
Stale ACK	BrandonOdiwuor

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 :

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 ✅

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

hebasto added 2 commits July 17, 2025 16:38

cmake: Create subdirectories in build tree in advance

88d9092

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

cmake: Replace recursive globbing with explicit globbing in folders

fa3beb6

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

cmake: Create subdirectories in build tree in advance #32773

cmake: Create subdirectories in build tree in advance #32773

hebasto commented Jun 18, 2025 •

edited

Loading

Uh oh!

DrahtBot commented Jun 18, 2025 •

edited

Loading

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!

BrandonOdiwuor left a comment •

edited

Loading

Uh oh!

hebasto commented Jul 17, 2025

Uh oh!

janb84 left a comment

Uh oh!

Uh oh!

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 Jun 18, 2025 •

edited

Loading

DrahtBot commented Jun 18, 2025 •

edited

Loading

janb84 left a comment •

edited

Loading

BrandonOdiwuor left a comment •

edited

Loading