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

cmake: Port PR25972 from the master branch #188

Merged
merged 3 commits into from
May 8, 2024
Merged

Conversation

hebasto
Copy link
Owner

@hebasto hebasto commented May 4, 2024

This PR ports bitcoin#25972 after the recent sync/rebase PR.

hebasto added 2 commits May 4, 2024 18:06
Bump clang to version 16 in the "Linux 32-bit" CI job to avoid false
positive `-Wunreachable-code` warnings. Link to libatomic is still
required.
Switch to Ubuntu 24.04 that provides GCC 13 by default, which fixes
false positive `-Wmaybe-uninitialized` warnings in the "Linux 64-bit,
multiprocess" job.
@hebasto hebasto marked this pull request as ready for review May 4, 2024 20:36
exe_extension: '.exe'
- name: 'MinGW-w64, debug'
triplet: 'x86_64-w64-mingw32'
packages: 'g++-mingw-w64-x86-64-posix'
depends_options: 'DEBUG=1'
configure_options: '-DCMAKE_BUILD_TYPE=Debug'
configure_options: '-DCMAKE_BUILD_TYPE=Debug -DWERROR=ON -DCMAKE_CXX_FLAGS="-Wno-error=maybe-uninitialized"'
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we not supposed to use the APPEND_CXXFLAGS for this?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are not merged into the staging branch yet.

Copy link

@vasild vasild left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK b748fbf

-DCMAKE_CXX_FLAGS=... would have to be adjusted after APPEND_CXXFLAGS is done. Maybe postpone this PR until then or drop the last commit from this PR.

@hebasto hebasto force-pushed the 240504-cmake-ET branch from b748fbf to 1689b48 Compare May 8, 2024 16:47
@hebasto
Copy link
Owner Author

hebasto commented May 8, 2024

-DCMAKE_CXX_FLAGS=... would have to be adjusted after APPEND_CXXFLAGS is done. Maybe postpone this PR until then or drop the last commit from this PR.

The last commit has been dropped.

Copy link

@vasild vasild left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 1689b48

@hebasto hebasto merged commit 9857482 into cmake-staging May 8, 2024
56 checks passed
@fanquake
Copy link

fanquake commented May 8, 2024

I don't see how the last commit can be dropped, unless the CI here isn't equivalent to master? Otherwise it should now be failing with build errors?

@hebasto
Copy link
Owner Author

hebasto commented May 9, 2024

... unless the CI here isn't equivalent to master?

It is not. The .github/workflows/ci.yml was replaced with .github/workflows/cmake.yml, which aims test cases that were/are specific the CMake migration process.

The master branch CI script is to be restored in #142.

@fanquake
Copy link

fanquake commented May 9, 2024

I guess I don't understand why compilers are being changed to avoid warnings then? Why not just turn off Werror if the point isn't to match master? Rather than swap to a compiler we don't use?

@hebasto
Copy link
Owner Author

hebasto commented May 9, 2024

I guess I don't understand why compilers are being changed to avoid warnings then? Why not just turn off Werror if the point isn't to match master?

Yes. This can be done.

Rather than swap to a compiler we don't use?

The number of developers, who uses Ubuntu 24.04, is expected to grow.

Anyway, all CI stuff has to be resolved in #142.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
port from autotools Ported from the main repository
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants