depends: CMake invocation cleanup #19685
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Specify well-known env vars instead of using a workaround to split up CC and CXX.
Gitian builds
|
|
Concept ACK. @ryanofsky could you take a look re multiprocess. |
ryanofsky
approved these changes
Aug 12, 2020
There was a problem hiding this comment.
Code review ACK 8c7cd0c. Looks good, and I tested the native linux and linux->mac cross builds. It would have made sense to favor using CC/CXX environment variables over -DCMAKE_C_COMPILER and -DCMAKE_CXX_COMPILER for cmake cross compiles in #18677, but I didn't know at the time that cmake actually used these environment variables: https://cmake.org/cmake/help/latest/manual/cmake-env-variables.7.html
| ifneq ($($(1)_type),build) | ||
| ifneq ($(host),$(build)) | ||
| $(1)_cmake += -DCMAKE_SYSTEM_NAME=$($(host_os)_cmake_system) -DCMAKE_SYSROOT=$(host_prefix) |
There was a problem hiding this comment.
In commit "depends: More robust cmake invocation" (8c7cd0c)
Note: Seems to makes sense -DCMAKE_SYSROOT setting here can be dropped if cmake picks up sysroot from CC/CXX variables. It apparently does if I run make -C depends HOST=x86_64-apple-darwin16 MULTIPROCESS=1 NO_QT=1 V=1 libmultiprocess_built, looking at CMAKE_CXX_COMPILER_ARG1 in CMakeCache.txt
I am still unsure of when we should and shouldn't use sysroot (#18915), but as long as values that are set are passed along that's probably good.
This is similar to how we do it for qt.mk.
|
Took another look and found a few very simple things I can add. Sorry for invalidating reviews @ryanofsky |
Gitian builds
|
Guix builds
|
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Sep 3, 2020
b893688 depends: Specify LDFLAGS to cmake as well (Carl Dong) b3f541f depends: Prepend CPPFLAGS to C{,XX}FLAGS for CMake (Carl Dong) 8e121e5 depends: Cleanup CMake invocation (Carl Dong) 8c7cd0c depends: More robust cmake invocation (Carl Dong) 3ecf0ec depends: Use $($(package)_cmake) instead of cmake (Carl Dong) Pull request description: - Use `$($(package)_cmake)` instead of invoking `cmake` directly - Use well-known env vars instead of overriding CMake variables ACKs for top commit: ryanofsky: Code review ACK b893688. Only changes since last review are new commits adding whitespace, cppflags and ldflags to cmake invocation Tree-SHA512: cfcd8cc9dcd0b336cf48b82fca9fe4bbc7930ed397cb7a68a07066680eb4c1906a6a9b5bd2589b4b4999e8f16232fa30ee9b376b60f4456d0fff931fbf9cc19a
|
Still confirming but it seems like this PR might be causing an error on travis: "/home/travis/build/bitcoin/bitcoin/depends/x86_64-pc-linux-gnu/share/../native/bin/mpgen: error while loading shared libraries: libcapnpc-0.7.0.so: cannot open shared object file: No such file or directory" error in https://travis-ci.org/github/bitcoin/bitcoin/jobs/726983595#L2545 Running CI scripts locally, I'm able to reproduce the error. But if I revert the changes in this PR the error goes away. The difference seems to be caused by cmake no longer setting RUNPATH after this PR. With this PR, RUNPATH is empty, but if this PR is reverted, readelf shows: $ readelf -d depends/x86_64-pc-linux-gnu/native/bin/mpgen | grep RUNPATH
0x000000000000001d (RUNPATH) Library runpath: [/home/russ/src/bitcoin/depends/x86_64-pc-linux-gnu/native/lib]I need to dig in more but two questions I have are:
|
|
@ryanofsky thanks for following up here. Did your digging turn anything up? I've opened #19981 in the interim so this doesn't get lost. |
ryanofsky
added a commit
to ryanofsky/bitcoin
that referenced
this pull request
Sep 24, 2020
After bitcoin#19685 started setting LDFLAGS, the INSTALL_RPATH_USE_LINK_PATH cmake option used in the libmultiprocess build seems to no longer work so it is neccessary to set CMAKE_INSTALL_RPATH as a fallback. It's unclear currently whether the bad interaction between INSTALL_RPATH_USE_LINK_PATH and LDFLAGS is a bug, but the issue is reported: bitcoin#19981 (comment) https://discourse.cmake.org/t/install-rpath-use-link-path-not-working-when-cmake-exe-linker-flags-ldflags-is-set/1892
ryanofsky
added a commit
to ryanofsky/bitcoin
that referenced
this pull request
Sep 30, 2020
After bitcoin#19685 started setting LDFLAGS, the INSTALL_RPATH_USE_LINK_PATH cmake option used in the libmultiprocess build no longer works, so it is neccessary to set CMAKE_INSTALL_RPATH as a fallback. It's unclear currently whether the bad interaction between INSTALL_RPATH_USE_LINK_PATH and LDFLAGS is a bug, but the issue is reported: bitcoin#19981 (comment) https://discourse.cmake.org/t/install-rpath-use-link-path-not-working-when-cmake-exe-linker-flags-ldflags-is-set/1892 Fixes bitcoin#19981
ryanofsky
added a commit
to ryanofsky/bitcoin
that referenced
this pull request
Oct 1, 2020
After bitcoin#19685 started setting LDFLAGS, the INSTALL_RPATH_USE_LINK_PATH cmake option used in the libmultiprocess build no longer works, so it is neccessary to set CMAKE_INSTALL_RPATH as a fallback. It's unclear currently whether the bad interaction between INSTALL_RPATH_USE_LINK_PATH and LDFLAGS is a bug, but the issue is reported: bitcoin#19981 (comment) https://discourse.cmake.org/t/install-rpath-use-link-path-not-working-when-cmake-exe-linker-flags-ldflags-is-set/1892 Fixes bitcoin#19981
ryanofsky
added a commit
to ryanofsky/bitcoin
that referenced
this pull request
Oct 1, 2020
After bitcoin#19685 started setting LDFLAGS, the INSTALL_RPATH_USE_LINK_PATH cmake option used in the libmultiprocess build no longer works, so it is neccessary to set CMAKE_INSTALL_RPATH as a fallback. It's unclear currently whether the bad interaction between INSTALL_RPATH_USE_LINK_PATH and LDFLAGS is a bug, but the issue is reported: bitcoin#19981 (comment) https://discourse.cmake.org/t/install-rpath-use-link-path-not-working-when-cmake-exe-linker-flags-ldflags-is-set/1892 Fixes bitcoin#19981
ryanofsky
added a commit
to ryanofsky/bitcoin
that referenced
this pull request
Oct 1, 2020
After bitcoin#19685 started setting LDFLAGS, the INSTALL_RPATH_USE_LINK_PATH cmake option used in the libmultiprocess build no longer works, so it is neccessary to set CMAKE_INSTALL_RPATH as a fallback. It's unclear currently whether the bad interaction between INSTALL_RPATH_USE_LINK_PATH and LDFLAGS is a bug, but the issue is reported: bitcoin#19981 (comment) https://discourse.cmake.org/t/install-rpath-use-link-path-not-working-when-cmake-exe-linker-flags-ldflags-is-set/1892 Fixes bitcoin#19981
ryanofsky
added a commit
to ryanofsky/bitcoin
that referenced
this pull request
Oct 1, 2020
After bitcoin#19685 started setting LDFLAGS, the INSTALL_RPATH_USE_LINK_PATH cmake option used in the libmultiprocess build no longer works, so it is neccessary to set CMAKE_INSTALL_RPATH as a fallback. It's unclear currently whether the bad interaction between INSTALL_RPATH_USE_LINK_PATH and LDFLAGS is a bug, but the issue is reported: bitcoin#19981 (comment) https://discourse.cmake.org/t/install-rpath-use-link-path-not-working-when-cmake-exe-linker-flags-ldflags-is-set/1892 Fixes bitcoin#19981
ryanofsky
added a commit
to ryanofsky/bitcoin
that referenced
this pull request
Oct 2, 2020
After bitcoin#19685 started setting LDFLAGS, the INSTALL_RPATH_USE_LINK_PATH cmake option used in the libmultiprocess build no longer works, so it is neccessary to set CMAKE_INSTALL_RPATH as a fallback. It's unclear currently whether the bad interaction between INSTALL_RPATH_USE_LINK_PATH and LDFLAGS is a bug, but the issue is reported: bitcoin#19981 (comment) https://discourse.cmake.org/t/install-rpath-use-link-path-not-working-when-cmake-exe-linker-flags-ldflags-is-set/1892 Commands useful for building / testing this change make -C depends MULTIPROCESS=1 print-libmultiprocess_cmake make -C depends MULTIPROCESS=1 print-native_libmultiprocess_cmake make -C depends MULTIPROCESS=1 HOST=x86_64-apple-darwin16 print-libmultiprocess_cmake rm -rvf depends/x86_64-pc-linux-gnu/native depends/work/staging depends/work/build make -C depends MULTIPROCESS=1 V=1 native_libmultiprocess_staged for f in `find -name mpgen`; do echo == $f ==; readelf -d $f | grep -i path; done make -C depends MULTIPROCESS=1 V=1 native_libmultiprocess_built find -name CMakeCache.txt Fixes bitcoin#19981
ryanofsky
added a commit
to ryanofsky/bitcoin
that referenced
this pull request
Oct 2, 2020
After bitcoin#19685 started setting LDFLAGS, the INSTALL_RPATH_USE_LINK_PATH cmake option used in the libmultiprocess build no longer works, so it is neccessary to set CMAKE_INSTALL_RPATH as a fallback. It's unclear currently whether the bad interaction between INSTALL_RPATH_USE_LINK_PATH and LDFLAGS is a bug, but the issue is reported: bitcoin#19981 (comment) https://discourse.cmake.org/t/install-rpath-use-link-path-not-working-when-cmake-exe-linker-flags-ldflags-is-set/1892 Commands useful for building / testing this change make -C depends MULTIPROCESS=1 print-libmultiprocess_cmake make -C depends MULTIPROCESS=1 print-native_libmultiprocess_cmake make -C depends MULTIPROCESS=1 HOST=x86_64-apple-darwin16 print-libmultiprocess_cmake rm -rvf depends/x86_64-pc-linux-gnu/native depends/work/staging depends/work/build make -C depends MULTIPROCESS=1 V=1 native_libmultiprocess_staged for f in `find -name mpgen`; do echo == $f ==; readelf -d $f | grep -i path; done make -C depends MULTIPROCESS=1 V=1 native_libmultiprocess_built find -name CMakeCache.txt Fixes bitcoin#19981
ryanofsky
added a commit
to ryanofsky/bitcoin
that referenced
this pull request
Nov 25, 2020
depends: Set CMAKE_INSTALL_RPATH for native packages After bitcoin#19685 started setting LDFLAGS, the INSTALL_RPATH_USE_LINK_PATH cmake option used in the libmultiprocess build no longer works, so it is neccessary to set CMAKE_INSTALL_RPATH as a fallback. It's unclear currently whether the bad interaction between INSTALL_RPATH_USE_LINK_PATH and LDFLAGS is a bug, but the issue is reported: bitcoin#19981 (comment) https://discourse.cmake.org/t/install-rpath-use-link-path-not-working-when-cmake-exe-linker-flags-ldflags-is-set/1892 Commands useful for building / testing this change make -C depends MULTIPROCESS=1 print-libmultiprocess_cmake make -C depends MULTIPROCESS=1 print-native_libmultiprocess_cmake make -C depends MULTIPROCESS=1 HOST=x86_64-apple-darwin16 print-libmultiprocess_cmake rm -rvf depends/x86_64-pc-linux-gnu/native depends/work/staging depends/work/build make -C depends MULTIPROCESS=1 V=1 native_libmultiprocess_staged for f in `find -name mpgen`; do echo == $f ==; readelf -d $f | grep -i path; done make -C depends MULTIPROCESS=1 V=1 native_libmultiprocess_built find -name CMakeCache.txt Fixes bitcoin#19981
fanquake
added a commit
that referenced
this pull request
Dec 10, 2020
7d0271b depends: Set CMAKE_INSTALL_RPATH for native packages (Russell Yanofsky) Pull request description: This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/projects/10). --- After #19685 started setting `LDFLAGS`, the `INSTALL_RPATH_USE_LINK_PATH` cmake option used in the libmultiprocess build no longer works, so it is neccessary to set `CMAKE_INSTALL_RPATH` as a fallback. It's unclear currently whether the bad interaction between `INSTALL_RPATH_USE_LINK_PATH` and `LDFLAGS` is a bug, but the issue is reported: - #19981 (comment) - https://discourse.cmake.org/t/install-rpath-use-link-path-not-working-when-cmake-exe-linker-flags-ldflags-is-set/1892 Fixes #19981 ACKs for top commit: fanquake: ACK 7d0271b - I haven't looked in depth, but I've re-read through #19981 and checked the failure by testing #19160 (with this reverted): dongcarl: ACK 7d0271b Looked into this a bit, it makes sense that for the things we build in depends, we want the library search to start in depends. It seems reasonable to expect this to happen automatically when `CMAKE_INSTALL_PREFIX` and `INSTALL_RPATH_USE_LINK_PATH` are set, but oh well... Tree-SHA512: 97cc5801c3204c14cd33004423631456ca0701e2127ee5146810a76e2f4aac9de1f4b5437402a4329cda54e022dc99270fee7e38c2995765f36b3848215fa78e
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Dec 10, 2020
…ages 7d0271b depends: Set CMAKE_INSTALL_RPATH for native packages (Russell Yanofsky) Pull request description: This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/projects/10). --- After bitcoin#19685 started setting `LDFLAGS`, the `INSTALL_RPATH_USE_LINK_PATH` cmake option used in the libmultiprocess build no longer works, so it is neccessary to set `CMAKE_INSTALL_RPATH` as a fallback. It's unclear currently whether the bad interaction between `INSTALL_RPATH_USE_LINK_PATH` and `LDFLAGS` is a bug, but the issue is reported: - bitcoin#19981 (comment) - https://discourse.cmake.org/t/install-rpath-use-link-path-not-working-when-cmake-exe-linker-flags-ldflags-is-set/1892 Fixes bitcoin#19981 ACKs for top commit: fanquake: ACK 7d0271b - I haven't looked in depth, but I've re-read through bitcoin#19981 and checked the failure by testing bitcoin#19160 (with this reverted): dongcarl: ACK 7d0271b Looked into this a bit, it makes sense that for the things we build in depends, we want the library search to start in depends. It seems reasonable to expect this to happen automatically when `CMAKE_INSTALL_PREFIX` and `INSTALL_RPATH_USE_LINK_PATH` are set, but oh well... Tree-SHA512: 97cc5801c3204c14cd33004423631456ca0701e2127ee5146810a76e2f4aac9de1f4b5437402a4329cda54e022dc99270fee7e38c2995765f36b3848215fa78e
hebasto
reviewed
Jun 9, 2022
| $(1)_cmake += -DCMAKE_SYSTEM_NAME=$($(host_os)_cmake_system) -DCMAKE_SYSROOT=$(host_prefix) | ||
| $(1)_cmake += -DCMAKE_C_COMPILER_TARGET=$(host) -DCMAKE_C_COMPILER=$(firstword $($($(1)_type)_CC)) -DCMAKE_C_FLAGS="$(wordlist 2,1000,$($($(1)_type)_CC))" | ||
| $(1)_cmake += -DCMAKE_CXX_COMPILER_TARGET=$(host) -DCMAKE_CXX_COMPILER=$(firstword $($($(1)_type)_CXX)) -DCMAKE_CXX_FLAGS="$(wordlist 2,1000,$($($(1)_type)_CXX))" | ||
| $(1)_cmake += -DCMAKE_SYSTEM_NAME=$($(host_os)_cmake_system) |
There was a problem hiding this comment.
Cross posting @fanquake's comment:
The cmake docs say that
CMAKE_SYSTEM_VERSIONmust also be set explicitly when cross-compiling:When the CMAKE_SYSTEM_NAME variable is set explicitly to enable cross compiling then the value of CMAKE_SYSTEM_VERSION must also be set explicitly to specify the target system version.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
$($(package)_cmake)instead of invokingcmakedirectly