- Use
$($(package)_cmake)instead of invokingcmakedirectly - Use well-known env vars instead of overriding CMake variables
depends: CMake invocation cleanup #19685
pull dongcarl wants to merge 5 commits into bitcoin:master from dongcarl:2020-08-depends-misc changing 3 files +11 −6-
dongcarl commented at 1:00 am on August 8, 2020: member
-
depends: Use $($(package)_cmake) instead of cmake 3ecf0eca63
-
8c7cd0c6d9
depends: More robust cmake invocation
Specify well-known env vars instead of using a workaround to split up CC and CXX.
-
dongcarl added the label Build system on Aug 8, 2020
-
MarcoFalke added the label Needs gitian build on Aug 8, 2020
-
MarcoFalke added the label Needs Guix build on Aug 8, 2020
-
DrahtBot commented at 3:50 am on August 9, 2020: member
Guix builds
-
DrahtBot removed the label Needs Guix build on Aug 9, 2020
-
DrahtBot commented at 0:29 am on August 10, 2020: member
Gitian builds
-
DrahtBot removed the label Needs gitian build on Aug 10, 2020
-
fanquake commented at 7:54 am on August 10, 2020: memberConcept ACK. @ryanofsky could you take a look re multiprocess.
-
in depends/funcs.mk:163 in 8c7cd0c6d9 outdated
156@@ -157,12 +157,12 @@ ifneq ($($(1)_ldflags),) 157 $(1)_autoconf += LDFLAGS="$$($(1)_ldflags)" 158 endif 159 160-$(1)_cmake=cmake -DCMAKE_INSTALL_PREFIX=$($($(1)_type)_prefix) 161+$(1)_cmake=env CC="$$($(1)_cc)" CXX="$$($(1)_cxx)" CFLAGS="$$($(1)_cflags)" CXXFLAGS="$$($(1)_cxxflags)" cmake -DCMAKE_INSTALL_PREFIX:PATH=$($($(1)_type)_prefix) 162 ifneq ($($(1)_type),build) 163 ifneq ($(host),$(build)) 164-$(1)_cmake += -DCMAKE_SYSTEM_NAME=$($(host_os)_cmake_system) -DCMAKE_SYSROOT=$(host_prefix)
ryanofsky commented at 4:29 pm on August 12, 2020:In commit “depends: More robust cmake invocation” (8c7cd0c6d9f295bcb6913e3c69c9dac4ce2b25ce)
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 atCMAKE_CXX_COMPILER_ARG1inCMakeCache.txtI am still unsure of when we should and shouldn’t use sysroot (https://github.com/bitcoin/bitcoin/issues/18915), but as long as values that are set are passed along that’s probably good.
ryanofsky approvedryanofsky commented at 5:06 pm on August 12, 2020: memberCode review ACK 8c7cd0c6d9f295bcb6913e3c69c9dac4ce2b25ce. 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.htmldepends: Cleanup CMake invocation 8e121e5509b3f541f618depends: Prepend CPPFLAGS to C{,XX}FLAGS for CMake
This is similar to how we do it for qt.mk.
depends: Specify LDFLAGS to cmake as well b893688357dongcarl commented at 7:41 pm on August 18, 2020: memberTook another look and found a few very simple things I can add. Sorry for invalidating reviews @ryanofskydongcarl added the label Needs gitian build on Aug 18, 2020dongcarl added the label Needs Guix build on Aug 18, 2020DrahtBot commented at 1:14 pm on August 22, 2020: memberGitian builds
DrahtBot removed the label Needs gitian build on Aug 22, 2020DrahtBot commented at 8:41 pm on August 24, 2020: memberGuix builds
DrahtBot removed the label Needs Guix build on Aug 24, 2020ryanofsky approvedryanofsky commented at 9:33 pm on August 25, 2020: memberCode review ACK b8936883573708059357a66f67fad9dc77a8bade. Only changes since last review are new commits adding whitespace, cppflags and ldflags to cmake invocationfanquake merged this on Sep 2, 2020fanquake closed this on Sep 2, 2020
sidhujag referenced this in commit 7f101d0ab8 on Sep 3, 2020ryanofsky commented at 5:27 pm on September 15, 2020: memberStill 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:
0$ readelf -d depends/x86_64-pc-linux-gnu/native/bin/mpgen | grep RUNPATH 1 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:
-
Why did travis not catch this problem when this PR was being worked on or after this PR was merged. Is there a cache that we failed to clear? Is there a cache we should be clearing when we update depends scripts in the future? (The travis error above started happening after I pushed a PR update that bumped the
depends/packages/native_libmultiprocess.mkpackage, so I think that accounts for why a cache might have been avoided in the failing run above). -
What change in this PR might be clobbering the cmake RUNPATH? If I had to guess it might be happening because the LDFLAGS environment variable was unset previously, but now it is set
fanquake commented at 4:58 am on September 20, 2020: member@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 referenced this in commit 01a7b77360 on Sep 24, 2020ryanofsky referenced this in commit da6c9b7089 on Sep 30, 2020ryanofsky referenced this in commit 9fe5c425fd on Oct 1, 2020ryanofsky referenced this in commit 429e78a909 on Oct 1, 2020ryanofsky referenced this in commit 4e99aef80d on Oct 1, 2020ryanofsky referenced this in commit 3c51b6a5d6 on Oct 1, 2020ryanofsky referenced this in commit 321aeb0bf1 on Oct 2, 2020ryanofsky referenced this in commit 7d0271b5c3 on Oct 2, 2020ryanofsky referenced this in commit 3655f304de on Nov 25, 2020fanquake referenced this in commit 17918a987a on Dec 10, 2020sidhujag referenced this in commit 7512d370e3 on Dec 10, 2020DrahtBot locked this on Feb 15, 2022
github-metadata-mirror
This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-07-03 10:13 UTC
This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-07-03 10:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me
More mirrored repositories can be found on mirror.b10c.me