cmake: Copy tests to the build tree instead of linking them #309
Conversation
hebasto
added
bug
|
cc:
|
|
ACK af3b042 Tested on Arm Mac and x86 Linux. Can replicate problems without and can configue/build/test with PR. |
There was a problem hiding this comment.
review-only ACK lgtm.
I presume there is another small difference, in that editing the symlink in the build dir will no longer modify the file in the source dir, but instead the copy of the file in the build dir. (Not sure if this is a PRO or CON, just wanted to mention it)
| file(CREATE_LINK ${CMAKE_CURRENT_SOURCE_DIR}/${script} ${CMAKE_CURRENT_BINARY_DIR}/${script} COPY_ON_ERROR ${symlink}) | ||
| endforeach() | ||
| unset(functional_tests) | ||
| file(COPY functional util/rpcauth-test.py DESTINATION ${CMAKE_CURRENT_BINARY_DIR}) |
There was a problem hiding this comment.
q: Creating a copy here, removes bitcoin#30463 again as a cmake requirement?
Just a question, not saying it should be reverted. It seems clearer and harmless either way, in case someone wants to use symbolic links, or otherwise symbolic links are use by cmake in the future again.
There was a problem hiding this comment.
q: Creating a copy here, removes bitcoin#30463 again as a cmake requirement?
Correct.
That's true. I forgot to mention it. Thanks for bringing this up. |
|
Actually, can you add exact steps to test this? How is someone supposed to If this is true, wouldn't it be better to hide this feature behind a flag to avoid anyone running into this by accident? There are too many support requests about stale/broken |
|
~0. Not sure. After a year of deciding we'll do things the "CMake way", at the last minute we're going to start allowing the thing CMake actively discourages, because of an unintuitive error message? Can't we just provide a more useful error? The PROs listed here are all pretty weak, and don't point to anything that's actually useful for a developer, doing development. It'd be good to get some example usecases of why someone might want to build in tree, that doesn't (or for some reason can't?) work with out-of-tree builds. As opposed to allowing this just because someone might "want to", and opening us up to also maintaining that can of worms (especially if it's not something we are going to actually use or test). |
There was a problem hiding this comment.
I tend towards NACK here for the downsides brought up already. I also think adding a flag for changing this behaviour and allowing in-tree builds is not a good alternative - especially absent a clear use-case. I also remember reviewing the script linking behaviour and thinking this was a good improvement.
Please consider an alternative in #314. |
|
Is it possible to allow in source builds while preserving the symlink behavior for out of tree builds? If I do an out of tree build with this PR, changes I make to the functional tests require an additional cmake invocation for those changes to appear in the build. When working on test changes, that's really annoying. |
Yes, it is.
That's true. |
Copy tests to the build tree instead of linking them.
hebasto
force-pushed
the
240806-cmake-IH
branch
from
af3b042
to
8c580b6
Compare
|
Rebased. |
I agree with this point. However, instead of adjusting this PR, I'm going to close in favour of #314. The main reason is that in-source builds with CMake are incompatible with Autotools because the We can reconsider this idea after completing the migration to CMake. |
This PR enables in-source builds, which currently fail on the staging branch.
As discussed offline, we won't restrict the user, even though CMake discourages in-source builds.
Other features of this approach:
PROS:
CONS:
Resolves #306.