Skip to content

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 subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants