-
Notifications
You must be signed in to change notification settings - Fork 26
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: EXTERNAL_MPGEN cleanups #147
Conversation
Suggested by Hennadii Stepanov <[email protected]> in bitcoin-core#142 (review)
@@ -24,7 +24,7 @@ if(Libmultiprocess_ENABLE_CLANG_TIDY) | |||
set(CMAKE_CXX_CLANG_TIDY "${CLANG_TIDY_EXECUTABLE}") | |||
endif() | |||
|
|||
set(EXTERNAL_MPGEN "" CACHE STRING "Use the supplied mpgen binary rather than the one built internally") |
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.
Hmm, I purposefully avoided namespacing this because I was assuming that parent projects would always want to expose the same option and have it be automatically inherited here. Now they'll have to map it.
Not a big deal to me either way, I guess.
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.
For reference, code which sets this in depends is implemented in bitcoin/bitcoin@99b7b85 from bitcoin/bitcoin#31741 and seems to work reasonably well
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.
For depends, sure, it doesn't matter. But when doing a non-depends cross-build, ccmake shows:
CAPNPC_CXX_EXECUTABLE
CAPNP_EXECUTABLE
...
Libmultiprocess_EXTERNAL_MPGEN
which is rather inconsistent and non-obvious to builders imo. Seems to me Capnp has it right. For a Core, I wouldn't expect to see that namespace.
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 see. This suggestion just came from #142 (comment), and I'm happy to rename this to something else or make a different change. Given the other names maybe MPGEN_EXECUTABLE
would be a good name for this.
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.
@hebasto might have more ideas too.
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 see. This suggestion just came from #142 (comment), and I'm happy to rename this to something else or make a different change. Given the other names maybe
MPGEN_EXECUTABLE
would be a good name for this.
Fwiw, I originally picked EXTERNAL_CAPNP
to mirror (what I thought was) the way Capnp handles it. But I see now that Capnp's is just a bool and CAPNP_EXECUTABLE
is the actual path variable. So ACK to MPGEN_EXECUTABLE
instead if we go down this path.
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.
So ACK to
MPGEN_EXECUTABLE
instead if we go down this path.
So do I.
endif() | ||
elseif(TARGET Libmultiprocess::multiprocess) | ||
set(MPGEN_BINARY $<TARGET_FILE:Libmultiprocess::mpgen>) |
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 attempted this a bunch of different ways, and with some approach this was necessary, but I forget why. I assume this is fine, thanks for simplifying.
Thank you for the cleanups! |
Bring in a few cmake changes to improve cmake support bitcoin-core/libmultiprocess#140 build: don't clobber user/superproject c++ version bitcoin-core/libmultiprocess#142 build: add option for external mpgen binary bitcoin-core/libmultiprocess#143 cleanup: initialize vars in the EventLoop constructor in the correct order bitcoin-core/libmultiprocess#146 cmake: Suppress compiler warnings from capnproto headers bitcoin-core/libmultiprocess#147 cmake: EXTERNAL_MPGEN cleanups
Bring in a few cmake changes to improve cmake support bitcoin-core/libmultiprocess#140 build: don't clobber user/superproject c++ version bitcoin-core/libmultiprocess#142 build: add option for external mpgen binary bitcoin-core/libmultiprocess#143 cleanup: initialize vars in the EventLoop constructor in the correct order bitcoin-core/libmultiprocess#146 cmake: Suppress compiler warnings from capnproto headers bitcoin-core/libmultiprocess#147 cmake: EXTERNAL_MPGEN cleanups bitcoin-core/libmultiprocess#148 util: fix -Wpessimizing-move warning
Bring in a few cmake changes to improve cmake support bitcoin-core/libmultiprocess#140 build: don't clobber user/superproject c++ version bitcoin-core/libmultiprocess#142 build: add option for external mpgen binary bitcoin-core/libmultiprocess#143 cleanup: initialize vars in the EventLoop constructor in the correct order bitcoin-core/libmultiprocess#146 cmake: Suppress compiler warnings from capnproto headers bitcoin-core/libmultiprocess#147 cmake: EXTERNAL_MPGEN cleanups bitcoin-core/libmultiprocess#148 util: fix -Wpessimizing-move warning bitcoin-core/libmultiprocess#145 CTest: Module must be included at the top level bitcoin-core/libmultiprocess#149 Avoid `-Wundef` compiler warnings bitcoin-core/libmultiprocess#152 refactor: Fix compiler and clang-tidy warnings
Discussed bitcoin-core#147 (comment) -BEGIN VERIFY SCRIPT- git grep -l Libmultiprocess_EXTERNAL_MPGEN | xargs sed -i s/Libmultiprocess_EXTERNAL_MPGEN/MPGEN_EXECUTABLE/g -END VERIFY SCRIPT-
…CUTABLE/g 250c2ea scripted-diff: s/Libmultiprocess_EXTERNAL_MPGEN/MPGEN_EXECUTABLE/g (Ryan Ofsky) Pull request description: Discussed #147 (comment) -BEGIN VERIFY SCRIPT- git grep -l Libmultiprocess_EXTERNAL_MPGEN | xargs sed -i s/Libmultiprocess_EXTERNAL_MPGEN/MPGEN_EXECUTABLE/g -END VERIFY SCRIPT- Top commit has no ACKs. Tree-SHA512: 6e3f205fe8fc6d89883f4f513564e845bb4bedfc48ede363ab4e8c35e55ce3da8ab49ecd9d2a71d0bb6b6c08e9d55216886329582419e67f72649accfbc7aae0
Bring in a few cmake changes to improve cmake support and fix compile/lint/tidy warnings. bitcoin-core/libmultiprocess#140 build: don't clobber user/superproject c++ version bitcoin-core/libmultiprocess#142 build: add option for external mpgen binary bitcoin-core/libmultiprocess#143 cleanup: initialize vars in the EventLoop constructor in the correct order bitcoin-core/libmultiprocess#146 cmake: Suppress compiler warnings from capnproto headers bitcoin-core/libmultiprocess#147 cmake: EXTERNAL_MPGEN cleanups bitcoin-core/libmultiprocess#148 util: fix -Wpessimizing-move warning bitcoin-core/libmultiprocess#145 CTest: Module must be included at the top level bitcoin-core/libmultiprocess#149 Avoid `-Wundef` compiler warnings bitcoin-core/libmultiprocess#152 refactor: Fix compiler and clang-tidy warnings bitcoin-core/libmultiprocess#155 scripted-diff: s/Libmultiprocess_EXTERNAL_MPGEN/MPGEN_EXECUTABLE/g bitcoin-core/libmultiprocess#156 refactor: Remove locale-dependent function calls
Bring in a few cmake changes to improve cmake support and fix compile/lint/tidy warnings. bitcoin-core/libmultiprocess#140 build: don't clobber user/superproject c++ version bitcoin-core/libmultiprocess#142 build: add option for external mpgen binary bitcoin-core/libmultiprocess#143 cleanup: initialize vars in the EventLoop constructor in the correct order bitcoin-core/libmultiprocess#146 cmake: Suppress compiler warnings from capnproto headers bitcoin-core/libmultiprocess#147 cmake: EXTERNAL_MPGEN cleanups bitcoin-core/libmultiprocess#148 util: fix -Wpessimizing-move warning bitcoin-core/libmultiprocess#145 CTest: Module must be included at the top level bitcoin-core/libmultiprocess#149 Avoid `-Wundef` compiler warnings bitcoin-core/libmultiprocess#152 refactor: Fix compiler and clang-tidy warnings bitcoin-core/libmultiprocess#155 scripted-diff: s/Libmultiprocess_EXTERNAL_MPGEN/MPGEN_EXECUTABLE/g bitcoin-core/libmultiprocess#156 refactor: Remove locale-dependent function calls bitcoin-core/libmultiprocess#157 refactor: Avoid using std::format
Bring in a few cmake changes to improve cmake support and fix compile/lint/tidy warnings. bitcoin-core/libmultiprocess#140 build: don't clobber user/superproject c++ version bitcoin-core/libmultiprocess#142 build: add option for external mpgen binary bitcoin-core/libmultiprocess#143 cleanup: initialize vars in the EventLoop constructor in the correct order bitcoin-core/libmultiprocess#146 cmake: Suppress compiler warnings from capnproto headers bitcoin-core/libmultiprocess#147 cmake: EXTERNAL_MPGEN cleanups bitcoin-core/libmultiprocess#148 util: fix -Wpessimizing-move warning bitcoin-core/libmultiprocess#145 CTest: Module must be included at the top level bitcoin-core/libmultiprocess#149 Avoid `-Wundef` compiler warnings bitcoin-core/libmultiprocess#152 refactor: Fix compiler and clang-tidy warnings bitcoin-core/libmultiprocess#155 scripted-diff: s/Libmultiprocess_EXTERNAL_MPGEN/MPGEN_EXECUTABLE/g bitcoin-core/libmultiprocess#156 refactor: Remove locale-dependent function calls bitcoin-core/libmultiprocess#157 refactor: Avoid using std::format bitcoin-core/libmultiprocess#159 bugfix: Do not lock EventLoop::mutex after EventLoop is done
Bring in a few cmake changes to improve cmake support and fix compile/lint/tidy warnings. bitcoin-core/libmultiprocess#140 build: don't clobber user/superproject c++ version bitcoin-core/libmultiprocess#142 build: add option for external mpgen binary bitcoin-core/libmultiprocess#143 cleanup: initialize vars in the EventLoop constructor in the correct order bitcoin-core/libmultiprocess#146 cmake: Suppress compiler warnings from capnproto headers bitcoin-core/libmultiprocess#147 cmake: EXTERNAL_MPGEN cleanups bitcoin-core/libmultiprocess#148 util: fix -Wpessimizing-move warning bitcoin-core/libmultiprocess#145 CTest: Module must be included at the top level bitcoin-core/libmultiprocess#149 Avoid `-Wundef` compiler warnings bitcoin-core/libmultiprocess#152 refactor: Fix compiler and clang-tidy warnings bitcoin-core/libmultiprocess#155 scripted-diff: s/Libmultiprocess_EXTERNAL_MPGEN/MPGEN_EXECUTABLE/g bitcoin-core/libmultiprocess#156 refactor: Remove locale-dependent function calls bitcoin-core/libmultiprocess#157 refactor: Avoid using std::format bitcoin-core/libmultiprocess#159 bugfix: Do not lock EventLoop::mutex after EventLoop is done bitcoin-core/libmultiprocess#161 cmake: Avoid including CTest if not top level project
Bring in a few cmake changes to improve cmake support and fix compile/lint/tidy warnings. bitcoin-core/libmultiprocess#140 build: don't clobber user/superproject c++ version bitcoin-core/libmultiprocess#142 build: add option for external mpgen binary bitcoin-core/libmultiprocess#143 cleanup: initialize vars in the EventLoop constructor in the correct order bitcoin-core/libmultiprocess#146 cmake: Suppress compiler warnings from capnproto headers bitcoin-core/libmultiprocess#147 cmake: EXTERNAL_MPGEN cleanups bitcoin-core/libmultiprocess#148 util: fix -Wpessimizing-move warning bitcoin-core/libmultiprocess#145 CTest: Module must be included at the top level bitcoin-core/libmultiprocess#149 Avoid `-Wundef` compiler warnings bitcoin-core/libmultiprocess#152 refactor: Fix compiler and clang-tidy warnings bitcoin-core/libmultiprocess#155 scripted-diff: s/Libmultiprocess_EXTERNAL_MPGEN/MPGEN_EXECUTABLE/g bitcoin-core/libmultiprocess#156 refactor: Remove locale-dependent function calls bitcoin-core/libmultiprocess#157 refactor: Avoid using std::format bitcoin-core/libmultiprocess#159 bugfix: Do not lock EventLoop::mutex after EventLoop is done bitcoin-core/libmultiprocess#161 cmake: Avoid including CTest if not top level project
Bump libmultiprocess library to include bugfix bitcoin-core/libmultiprocess#159 which should fix intermittent CI failure reported bitcoin#31921 This change is bumping the libmultiprocess version instead of cherry picking the bugfix. It could cherry-pick the bugfix instead, but there are reasons to prefer just bumping the version: - Bugfix might interact with earlier PRs, and the latest version is better tested with testing done in many CI configurations in bitcoin#30975 and bitcoin#31802 - Even though we are in feature freeze for a release, the MULTIPROCESS=1 option is currently not enabled for release, so this PR only affect CI builds and local builds, not the release build. bitcoin-core/libmultiprocess#140 build: don't clobber user/superproject c++ version bitcoin-core/libmultiprocess#142 build: add option for external mpgen binary bitcoin-core/libmultiprocess#143 cleanup: initialize vars in the EventLoop constructor in the correct order bitcoin-core/libmultiprocess#146 cmake: Suppress compiler warnings from capnproto headers bitcoin-core/libmultiprocess#147 cmake: EXTERNAL_MPGEN cleanups bitcoin-core/libmultiprocess#148 util: fix -Wpessimizing-move warning bitcoin-core/libmultiprocess#145 CTest: Module must be included at the top level bitcoin-core/libmultiprocess#149 Avoid `-Wundef` compiler warnings bitcoin-core/libmultiprocess#152 refactor: Fix compiler and clang-tidy warnings bitcoin-core/libmultiprocess#155 scripted-diff: s/Libmultiprocess_EXTERNAL_MPGEN/MPGEN_EXECUTABLE/g bitcoin-core/libmultiprocess#156 refactor: Remove locale-dependent function calls bitcoin-core/libmultiprocess#157 refactor: Avoid using std::format bitcoin-core/libmultiprocess#159 bugfix: Do not lock EventLoop::mutex after EventLoop is done bitcoin-core/libmultiprocess#161 cmake: Avoid including CTest if not top level project bitcoin-core/libmultiprocess#164 Bump minimum required cmake to 3.12
Bump libmultiprocess library to include bugfix bitcoin-core/libmultiprocess#159 which should fix intermittent CI failures reported in bitcoin#31921 This change is bumping the libmultiprocess version instead of cherry picking the bugfix. It could cherry-pick the bugfix instead, but there are reasons to prefer bumping the version: - Bugfix might interact with earlier PRs, and the latest version is better tested with testing done in many CI configurations in bitcoin#30975 and bitcoin#31802 - Even though we are in feature freeze for a release, the MULTIPROCESS=1 option is currently not enabled for release, so this PR only affect CI builds and local builds, not the release build. This update brings in the following changes: bitcoin-core/libmultiprocess#140 build: don't clobber user/superproject c++ version bitcoin-core/libmultiprocess#142 build: add option for external mpgen binary bitcoin-core/libmultiprocess#143 cleanup: initialize vars in the EventLoop constructor in the correct order bitcoin-core/libmultiprocess#146 cmake: Suppress compiler warnings from capnproto headers bitcoin-core/libmultiprocess#147 cmake: EXTERNAL_MPGEN cleanups bitcoin-core/libmultiprocess#148 util: fix -Wpessimizing-move warning bitcoin-core/libmultiprocess#145 CTest: Module must be included at the top level bitcoin-core/libmultiprocess#149 Avoid `-Wundef` compiler warnings bitcoin-core/libmultiprocess#152 refactor: Fix compiler and clang-tidy warnings bitcoin-core/libmultiprocess#155 scripted-diff: s/Libmultiprocess_EXTERNAL_MPGEN/MPGEN_EXECUTABLE/g bitcoin-core/libmultiprocess#156 refactor: Remove locale-dependent function calls bitcoin-core/libmultiprocess#157 refactor: Avoid using std::format bitcoin-core/libmultiprocess#159 bugfix: Do not lock EventLoop::mutex after EventLoop is done bitcoin-core/libmultiprocess#161 cmake: Avoid including CTest if not top level project bitcoin-core/libmultiprocess#164 Bump minimum required cmake to 3.12
Bump libmultiprocess library to include bugfix bitcoin-core/libmultiprocess#159 which should fix intermittent CI failures reported in bitcoin#31921 This change is bumping the libmultiprocess version instead of cherry picking the bugfix. It could cherry-pick the bugfix instead, but there are reasons to prefer bumping the version: - Bugfix might interact with earlier PRs, and the latest version is better tested with testing done in many CI configurations in bitcoin#30975 and bitcoin#31802 - Even though we are in feature freeze for a release, the MULTIPROCESS=1 option is currently not enabled for release, so this PR only affect CI builds and local builds, not the release build. This update brings in the following changes: bitcoin-core/libmultiprocess#140 build: don't clobber user/superproject c++ version bitcoin-core/libmultiprocess#142 build: add option for external mpgen binary bitcoin-core/libmultiprocess#143 cleanup: initialize vars in the EventLoop constructor in the correct order bitcoin-core/libmultiprocess#146 cmake: Suppress compiler warnings from capnproto headers bitcoin-core/libmultiprocess#147 cmake: EXTERNAL_MPGEN cleanups bitcoin-core/libmultiprocess#148 util: fix -Wpessimizing-move warning bitcoin-core/libmultiprocess#145 CTest: Module must be included at the top level bitcoin-core/libmultiprocess#149 Avoid `-Wundef` compiler warnings bitcoin-core/libmultiprocess#152 refactor: Fix compiler and clang-tidy warnings bitcoin-core/libmultiprocess#155 scripted-diff: s/Libmultiprocess_EXTERNAL_MPGEN/MPGEN_EXECUTABLE/g bitcoin-core/libmultiprocess#156 refactor: Remove locale-dependent function calls bitcoin-core/libmultiprocess#157 refactor: Avoid using std::format bitcoin-core/libmultiprocess#159 bugfix: Do not lock EventLoop::mutex after EventLoop is done bitcoin-core/libmultiprocess#161 cmake: Avoid including CTest if not top level project bitcoin-core/libmultiprocess#164 Bump minimum required cmake to 3.12
Bump libmultiprocess library to include bugfix bitcoin-core/libmultiprocess#159 which should fix intermittent CI failures reported in #31921 This change is bumping the libmultiprocess version instead of cherry picking the bugfix. It could cherry-pick the bugfix instead, but there are reasons to prefer bumping the version: - Bugfix might interact with earlier PRs, and the latest version is better tested with testing done in many CI configurations in #30975 and #31802 - Even though we are in feature freeze for a release, the MULTIPROCESS=1 option is currently not enabled for release, so this PR only affect CI builds and local builds, not the release build. This update brings in the following changes: bitcoin-core/libmultiprocess#140 build: don't clobber user/superproject c++ version bitcoin-core/libmultiprocess#142 build: add option for external mpgen binary bitcoin-core/libmultiprocess#143 cleanup: initialize vars in the EventLoop constructor in the correct order bitcoin-core/libmultiprocess#146 cmake: Suppress compiler warnings from capnproto headers bitcoin-core/libmultiprocess#147 cmake: EXTERNAL_MPGEN cleanups bitcoin-core/libmultiprocess#148 util: fix -Wpessimizing-move warning bitcoin-core/libmultiprocess#145 CTest: Module must be included at the top level bitcoin-core/libmultiprocess#149 Avoid `-Wundef` compiler warnings bitcoin-core/libmultiprocess#152 refactor: Fix compiler and clang-tidy warnings bitcoin-core/libmultiprocess#155 scripted-diff: s/Libmultiprocess_EXTERNAL_MPGEN/MPGEN_EXECUTABLE/g bitcoin-core/libmultiprocess#156 refactor: Remove locale-dependent function calls bitcoin-core/libmultiprocess#157 refactor: Avoid using std::format bitcoin-core/libmultiprocess#159 bugfix: Do not lock EventLoop::mutex after EventLoop is done bitcoin-core/libmultiprocess#161 cmake: Avoid including CTest if not top level project bitcoin-core/libmultiprocess#164 Bump minimum required cmake to 3.12
01f7715 depends: Update libmultiprocess library to fix CI failure (Ryan Ofsky) Pull request description: Bump libmultiprocess library to include bugfix bitcoin-core/libmultiprocess#159 which should fix intermittent CI failures reported in #31921 This change is bumping the libmultiprocess version instead of cherry picking the bugfix. It could cherry-pick the bugfix instead, but there are reasons to prefer bumping the version: - Bugfix might interact with earlier PRs, and the latest version is better tested with testing done in many CI configurations in [#30975](#30975) and [#31802](#31802) - Even though we are in feature freeze for a release, the MULTIPROCESS=1 option is currently not enabled for release, so this PR only affect CI builds and local builds, not the release build. This update brings in the following changes: bitcoin-core/libmultiprocess#140 build: don't clobber user/superproject c++ version bitcoin-core/libmultiprocess#142 build: add option for external mpgen binary bitcoin-core/libmultiprocess#143 cleanup: initialize vars in the EventLoop constructor in the correct order bitcoin-core/libmultiprocess#146 cmake: Suppress compiler warnings from capnproto headers bitcoin-core/libmultiprocess#147 cmake: EXTERNAL_MPGEN cleanups bitcoin-core/libmultiprocess#148 util: fix -Wpessimizing-move warning bitcoin-core/libmultiprocess#145 CTest: Module must be included at the top level bitcoin-core/libmultiprocess#149 Avoid `-Wundef` compiler warnings bitcoin-core/libmultiprocess#152 refactor: Fix compiler and clang-tidy warnings bitcoin-core/libmultiprocess#155 scripted-diff: s/Libmultiprocess_EXTERNAL_MPGEN/MPGEN_EXECUTABLE/g bitcoin-core/libmultiprocess#156 refactor: Remove locale-dependent function calls bitcoin-core/libmultiprocess#157 refactor: Avoid using std::format bitcoin-core/libmultiprocess#159 bugfix: Do not lock EventLoop::mutex after EventLoop is done bitcoin-core/libmultiprocess#161 cmake: Avoid including CTest if not top level project bitcoin-core/libmultiprocess#164 Bump minimum required cmake to 3.12 --- This PR is part of the [process separation project](#28722). ACKs for top commit: fanquake: ACK 01f7715 Tree-SHA512: a6a795e4d4e13e9d35c9346f3c83d5da817f1452bdc4a9412aeadc4c652ad6e5047f4c77756570594a70ec9095cc786772a0469e306dc19bb5a508fd515c37f4
Suggested by hebasto in #142 (review)