search

hebasto / bitcoin

Bitcoin Core integration/staging tree
https://bitcoincore.org/en/download
MIT License
20 stars 5 forks source link

cmake: Copy tests to the build tree instead of linking them #309

Closed hebasto closed 1 month ago

hebasto commented 1 month ago

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:

  1. Less of code. Simpler code.
  2. The same code for both POSIX and Windows platforms.
  3. It prevents the source tree from being polluted with Python bytecode caches for the test framework when running functional tests. For reference, https://github.com/bitcoin/bitcoin/pull/30533 aims for the same goal for unit tests.

CONS:

  1. Approximately 4MB larger build directory. Note that it is smaller than its size just after configuring on the staging branch.

Resolves https://github.com/hebasto/bitcoin/issues/306.

hebasto commented 1 month ago

cc:

m3dwards commented 1 month ago

ACK af3b04244ef19d484169356a62e92759e1c82b98

Tested on Arm Mac and x86 Linux. Can replicate problems without and can configue/build/test with PR.

hebasto commented 1 month ago

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)

That's true. I forgot to mention it. Thanks for bringing this up.

maflcko commented 1 month ago

Actually, can you add exact steps to test this?

How is someone supposed to make clean or make distclean with this pull request. IIRC, cmake doesn't have the equivalent and it should be rm -r ./build_dir. However, if the source dir is the build dir, it requires the user to know what they are doing. For example, they could be using a git-worktree.

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 make builds (due to a missing call to make (dist)clean), so I fear unlocking this for cmake may encourage more support requests of the same build issue to happen?

fanquake commented 1 month ago

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

hebasto commented 1 month ago

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 https://github.com/hebasto/bitcoin/pull/314.

achow101 commented 1 month ago

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.

hebasto commented 1 month ago

Is it possible to allow in source builds while preserving the symlink behavior for out of tree builds?

Yes, it is.

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.

That's true.

hebasto commented 1 month ago

Rebased.

hebasto commented 1 month ago

symlinks should be kept for out-of-tree builds, otherwise it seems like a breaking change to dev experience when editing files

I agree with this point.

However, instead of adjusting this PR, I'm going to close in favour of https://github.com/hebasto/bitcoin/pull/314.

The main reason is that in-source builds with CMake are incompatible with Autotools because the Makefile files are being overwritten.

We can reconsider this idea after completing the migration to CMake.