Skip to content

cmake: Switch to crc32c upstream build system #30905

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

Closed
wants to merge 2 commits into from

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Sep 14, 2024

A few things still need to be addressed:

@DrahtBot
Copy link
Contributor

DrahtBot commented Sep 14, 2024

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.
A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #30595 (kernel: Introduce initial C header API by TheCharlatan)
  • #29852 ([WIP] build: remove need to test for endianness by fanquake)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@DrahtBot
Copy link
Contributor

🚧 At least one of the CI tasks failed.
Debug: https://github.com/bitcoin/bitcoin/runs/30150430662

Hints

Make sure to run all tests locally, according to the documentation.

The failure may happen due to a number of reasons, for example:

  • Possibly due to a silent merge conflict (the changes in this pull request being
    incompatible with the current code in the target branch). If so, make sure to rebase on the latest
    commit of the target branch.

  • A sanitizer issue, which can only be found by compiling with the sanitizer and running the
    affected test.

  • An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

@fanquake
Copy link
Member

A few things still need to be addressed:

This PR is also changing the flags used for code generation. Is that intentional? If yes, would be good to mention in the PR description (and why).

@hebasto
Copy link
Member Author

hebasto commented Sep 17, 2024

This PR is also changing the flags used for code generation.

Yes. The crc32c upstream build system modifies flags in many places, for example:

  # Disable C++ exceptions.
  string(REGEX REPLACE "-fexceptions" "" CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS}")
  set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fno-exceptions")


  # Disable RTTI.
  string(REGEX REPLACE "-frtti" "" CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS}")
  set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fno-rtti")

Is that intentional?

The intention isn't mine.

@fanquake
Copy link
Member

Ok. Well the current changes would be unsafe to merge, and I don't think we should be adjusting our build system to work around the subtree, if anything, we should just change the subtree directly.

This change allows overriding the cached value with local variables when
building in subtrees.
@fanquake
Copy link
Member

Closing, as the consensus between myself, @hebasto, @theuni and @TheCharlatan was to not make this change (for now, or not until we've adjusted upstream further).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants