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.

Sign up for GitHub

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

Jump to bottom

cmake: Port PR29774 from the master branch #182

Merged
merged 4 commits into from May 3, 2024
Merged

cmake: Port PR29774 from the master branch #182

merged 4 commits into from May 3, 2024

Conversation

hebasto
Copy link
Owner

@hebasto hebasto commented May 1, 2024

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

Also included changes from bitcoin#29983.

This was referenced May 1, 2024
Merged
Merged
theuni
theuni reviewed May 1, 2024
View reviewed changes
src/test/fuzz/CMakeLists.txt
@@ -56,7 +56,8 @@ add_executable(fuzz
locale.cpp
merkleblock.cpp
message.cpp
miniscript.cpp
# TODO: Fix the code and enable miniscript.cpp for MSVC.
$<$<NOT:$<CXX_COMPILER_ID:MSVC>>:miniscript.cpp>
Copy link

Choose a reason for hiding this comment

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

What's necessary here?

Copy link
Owner Author

Choose a reason for hiding this comment

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

I don't know at this moment.

Otherwise, compiling with MSVC fails as reported in bitcoin#29774 (comment).

Copy link

Choose a reason for hiding this comment

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

Probably fixed by bitcoin#28657 ?

Copy link

Choose a reason for hiding this comment

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

Did you check if it is fixed by bitcoin#28657 @hebasto?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Checking now...

Copy link
Owner Author

Choose a reason for hiding this comment

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

Probably fixed by bitcoin#28657 ?

Yes, bitcoin#28657 and bitcoin#30031 fix it.

@hebasto hebasto added the port from autotools Ported from the main repository label May 1, 2024
@hebasto hebasto marked this pull request as draft May 1, 2024 21:58
@hebasto hebasto force-pushed the 240501-cmake-EO branch 2 times, most recently from 937d123 to 64c18d9 Compare May 1, 2024 23:31
@hebasto
Copy link
Owner Author

hebasto commented May 1, 2024

Added one more commit ported from bitcoin#30017.

@hebasto hebasto mentioned this pull request May 2, 2024
Merged
hebasto added a commit that referenced this pull request May 2, 2024
@hebasto
Merge #183: cmake: Fix and improve CI
8c74f93 
729632c fixup! ci: Test CMake edge cases (Hennadii Stepanov)
107bdf8 fixup! ci: Test CMake edge cases (Hennadii Stepanov)

Pull request description:

  1. The first commit forces the build step in the native Windows jobs to fail if build fails. Otherwise, the build step might
  falsely succeed because the last command exit code determines the return status of the step.

  I've noticed such wrong behaviour while working on #182.

  2. The second commit pulled from #125.

ACKs for top commit:
  m3dwards:
    utACK 729632c

Tree-SHA512: 3530f8c2d60896665adff52caaf88460e18bf1087f9350337a4f191558023ee3484b0639673f90c48d6c24074338b640d5cfde067bb90344dfebaa2d6173f1cc
@hebasto hebasto marked this pull request as ready for review May 2, 2024 12:13
@hebasto
Copy link
Owner Author

hebasto commented May 3, 2024

Added one more commit ported from bitcoin#30017.

It has just been merged to the master branch.

@TheCharlatan
Copy link

lgtm

@m3dwards
Copy link

m3dwards commented May 3, 2024

Do we also need to include PROVIDE_FUZZ_MAIN_FUNCTION as is in https://github.com/bitcoin/bitcoin/pull/29774/files#diff-d8e47b5cc5fa47a6b1cd38960e991b73c8e059221ff0d211a738688161a29032R93 ?

@hebasto
Copy link
Owner Author

hebasto commented May 3, 2024 •
edited

Do we also need to include PROVIDE_FUZZ_MAIN_FUNCTION as is in https://github.com/bitcoin/bitcoin/pull/29774/files#diff-d8e47b5cc5fa47a6b1cd38960e991b73c8e059221ff0d211a738688161a29032R93 ?

It has been done already:

bitcoin/src/test/fuzz/util/CMakeLists.txt

Lines 21 to 31 in fc91bfe

include(CheckSourceCompilesAndLinks)
check_cxx_source_links_with_flags("${SANITIZER_LDFLAGS}" "
#include <cstdint>
#include <cstddef>
extern \"C\" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) { return 0; }
// No main() function.
" FUZZ_BINARY_LINKS_WITHOUT_MAIN_FUNCTION
)
if(NOT FUZZ_BINARY_LINKS_WITHOUT_MAIN_FUNCTION)
target_compile_definitions(test_fuzz PRIVATE PROVIDE_FUZZ_MAIN_FUNCTION)
endif()

Otherwise, a linker would fail.

@hebasto hebasto merged commit 30b681f into cmake-staging May 3, 2024
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@theuni theuni theuni left review comments

@maflcko maflcko maflcko left review comments

@TheCharlatan TheCharlatan TheCharlatan left review comments

Assignees
No one assigned
Labels
port from autotools Ported from the main repository
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@hebasto @TheCharlatan @m3dwards @theuni @maflcko