Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[POC, DO NOT MERGE] cmake: Switch to CMake utilities repository #1552

Closed
wants to merge 2 commits into from

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Jun 25, 2024

This PR proposes reducing code repetition by reusing CMake utilities (functions and macros) shared between this project and Bitcoin Core. Both projects use similar functionality, including:

  1. Defining a default build configuration.
  2. Modifying configuration-specific flags.
  3. Checking compiler and linker flags.
  4. Reporting a summary.

To achieve this, I suggest creating a dedicated repository under https://github.com/bitcoin-core to house these utilities. Such a repository could also benefit other projects like https://github.com/sipa/minisketch and https://github.com/chaincodelabs/libmultiprocess. Additionally, the CMake utilities repository could include CI tests for the provided utilities.

Seeking Concept (N)ACKs for this proposal. Please note that only one function is replaced in this PR.

@fanquake
Copy link
Member

fanquake commented Jun 26, 2024

Concept NACK - It's hard to evaluate how much benefit this would actually bring (without a full example), and there's a number of downsides / other things that'd need to be considered before doing anything like this:

  • Given that the Core CMake build system hasn't even landed, and should be considered in flux for some time after that, I'd say it's far too early to be trying to extract "reused" code into a separate repo, as it's not even clear if that code will exist / will be duplicated in the future.
  • I'm not a huge fan of having the build system do networking, and we'd have to consider how this works in isolated build envs
    • I guess we'd have to preload the CMake content pre-Guix build, similar to depends, but can that be done without a full configure?
    • If you get our tarball can you compile it without internet?
    • Can nobody compile Core if Github happens to be down?
    • How does it work with subprojects etc? Do they share a single copy of the utils, if they happen to be using the same version? Or would we end up with multiple copies of different versions of the utils. i.e Core has a copy of the repo at version X, while secp256k1 has it's own copy of the utils at version Y etc.
  • I'm not sure if we actually want to couple all these projects together, and doing so may just lead to more hassle when trying to change the shared utils, as project requirements change.
  • I'm guessing (for us) this'd lead to more backporting / maintenance complications. i.e would the utils repo be versioned the same way we maintain our various branches? Otherwise what happens when you need to fix an issue in a util that effects all maintained versions, but you can't bump the subtree in an older branch because of newer changes?
  • This would also introduce a new bottleneck / annoyance for code review / changing anything in our repo, because to fix /change something in the build system, you might have to go to a different repo with even less reviewers and change something (which hopefully doesn't break something for all other projects) and then bump (essentially) a subtree before fixing something.

@real-or-random real-or-random added build meta/development processes, conventions, developer documentation, etc. labels Jun 26, 2024
@real-or-random
Copy link
Contributor

I get a bit anxious when I see duplicated code, and I think sharing code is, in general, pretty desirable. But I can't argue with @fanquake's very convincing list of disadvantages, and so I agree with the Concept NACK.

@hebasto hebasto closed this Jun 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build meta/development processes, conventions, developer documentation, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants