-
Notifications
You must be signed in to change notification settings - Fork 4
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 PR29774 from the master branch #182
Conversation
@@ -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> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's necessary here?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checking now...
There was a problem hiding this comment.
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.
937d123
to
64c18d9
Compare
Added one more commit ported from bitcoin#30017. |
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
This change fixes MSVC level-3 warning C4334. See: https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-3-c4334
Port PR29774 (1 of 2).
Port PR29774 (2 of 2).
It has just been merged to the master branch. |
lgtm |
Do we also need to include |
It has been done already: bitcoin/src/test/fuzz/util/CMakeLists.txt Lines 21 to 31 in fc91bfe
Otherwise, a linker would fail. |
This PR ports bitcoin#29774 after the recent sync/rebase PR.
Also included changes from bitcoin#29983.