depends: Amend handling flags environment variables

hebasto commented at 11:06 am on July 18, 2024: member

The {C,CXX,CPP,LD}FLAGS are build type-agnostic. Therefore, if any of them is specified, it should be assigned to a non-type-specific variable.

This PR is split from #30454. It is required because CMake distinguishes build type-specific variables. For instance, CMake assumes that CXXFLAGS affects the content of the CMAKE_CXX_FLAGS variable. However, without this change, the CMAKE_CXX_FLAGS_RELEASE or CMAKE_CXX_FLAGS_DEBUG will be affected instead.

No behaviour change for packages in depends:

on the master branch @ efbf4e71ce8e3cd49ccdfb5e55e14fa4b338453c

0$ make -C depends print-libevent_cxxflags CXXFLAGS="-std=c++20 -O0 -Wall" 2>&1 | grep libevent
1libevent_cxxflags= -std=c++20 -O0 -Wall
2$ make -C depends print-libevent_cxxflags CXXFLAGS="-std=c++20 -O0 -Wall" DEBUG=1 2>&1 | grep libevent
3libevent_cxxflags= -std=c++20 -O0 -Wall

with this PR:

0$ make -C depends print-libevent_cxxflags CXXFLAGS="-std=c++20 -O0 -Wall" 2>&1 | grep libevent
1libevent_cxxflags=-std=c++20 -O0 -Wall 
2$ make -C depends print-libevent_cxxflags CXXFLAGS="-std=c++20 -O0 -Wall" DEBUG=1 2>&1 | grep libevent
3libevent_cxxflags=-std=c++20 -O0 -Wall

Also there is zero diff for the generated share/config.site file.

depends: Amend handling flags environment variables

If any of {C,CXX,CPP,LD}FLAGS is specified it should be assigned to
a non-type-specific variable.

d438b501f8

hebasto added the label Build system on Jul 18, 2024

DrahtBot commented at 11:06 am on July 18, 2024: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	pablomartin4btc, vasild, TheCharlatan

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

fanquake commented at 11:18 am on July 18, 2024: member

It would be good to show an example of what this actually fixes.

No behaviour change for Autotools:

Libevent in depends is built with CMake, so if this is fixing something for CMake, shouldn’t this have some effect in master (for the libevent build done with CMake) regardless of if Autotools is used later for the main configure?

hebasto commented at 11:27 am on July 18, 2024: member

Libevent in depends is built with CMake, so if this is fixing something for CMake, shouldn’t this have some effect in master (for the libevent build done with CMake) regardless of if Autotools is used later for the main configure?

In depends, the $(package)_{c,cxx,ld,cpp}flags variables are used for both build systems, Autotools and CMake.

It is required for a new toolchain.cmake files generated in #30454: https://github.com/bitcoin/bitcoin/blob/2b7c0f4fdb81ec1cf0579fa71b37f56672934ab5/depends/Makefile#L277-L279

hebasto commented at 12:55 pm on July 18, 2024: member

It would be good to show an example of what this actually fixes.

It is required only for the main CMake-based build system.

For instance, CMake assumes that CXXFLAGS affects the content of the CMAKE_CXX_FLAGS variable. However, without this change, the CMAKE_CXX_FLAGS_RELEASE or CMAKE_CXX_FLAGS_DEBUG will be affected instead.

The PR description has been updated.

pablomartin4btc commented at 4:14 pm on August 2, 2024: member

tACK d438b501f859befa4f758efcff463c0d647bc033

Checked other packages as well.

vasild approved

vasild commented at 3:41 pm on August 13, 2024: contributor

ACK d438b501f859befa4f758efcff463c0d647bc033

TheCharlatan approved

TheCharlatan commented at 10:05 am on August 15, 2024: contributor

ACK d438b501f859befa4f758efcff463c0d647bc033

The important change here is that when used with cmake, the user-defined flags currently get placed in the toolchain file like this (running make CXXFLAGS="-O1" ):

 0 32 if(NOT DEFINED CMAKE_CXX_FLAGS_INIT)
 1 33   set(CMAKE_CXX_FLAGS_INIT "")
 2 34   set(CMAKE_OBJCXX_FLAGS_INIT "")
 3 35 endif()
 4 36 if(NOT DEFINED CMAKE_CXX_FLAGS_RELWITHDEBINFO_INIT)
 5 37   set(CMAKE_CXX_FLAGS_RELWITHDEBINFO_INIT "-O1")
 6 38   set(CMAKE_OBJCXX_FLAGS_RELWITHDEBINFO_INIT "-O1")
 7 39 endif()
 8 40 if(NOT DEFINED CMAKE_CXX_FLAGS_DEBUG_INIT)
 9 41   set(CMAKE_CXX_FLAGS_DEBUG_INIT "")
10 42   set(CMAKE_OBJCXX_FLAGS_DEBUG_INIT "")
11 43 endif()

And with this change:

 0if(NOT DEFINED CMAKE_CXX_FLAGS_INIT)
 1  set(CMAKE_CXX_FLAGS_INIT "-O1")
 2  set(CMAKE_OBJCXX_FLAGS_INIT "-O1")
 3endif()
 4if(NOT DEFINED CMAKE_CXX_FLAGS_RELWITHDEBINFO_INIT)
 5  set(CMAKE_CXX_FLAGS_RELWITHDEBINFO_INIT "")
 6  set(CMAKE_OBJCXX_FLAGS_RELWITHDEBINFO_INIT "")
 7endif()
 8if(NOT DEFINED CMAKE_CXX_FLAGS_DEBUG_INIT)
 9  set(CMAKE_CXX_FLAGS_DEBUG_INIT "")
10  set(CMAKE_OBJCXX_FLAGS_DEBUG_INIT "")
11endif()

Which seems to be the intended behaviour according to the docs linked by hebasto #30477 (comment).

maflcko added the label DrahtBot Guix build requested on Aug 21, 2024

DrahtBot commented at 8:16 pm on August 21, 2024: contributor

Guix builds (on x86_64) [untrusted test-only build, possibly unsafe, not for production use]

File	commit 60b816439eb4bd837778d424628cd3978e0856d9(master)	commit f72e66c81ace147cfac6ec84d7da688021cf738c(master and this pull)
SHA256SUMS.part	`e1d20819f9fe7a08...`	`d3413d87a3cf1bc3...`
*-aarch64-linux-gnu-debug.tar.gz	`d590bd07c550b27e...`	`25d2a483dea71fcd...`
*-aarch64-linux-gnu.tar.gz	`14fe218ce0710623...`	`71b5293970b7227b...`
*-arm-linux-gnueabihf-debug.tar.gz	`f77e35f6875b444e...`	`ae9d2a14814c2b10...`
*-arm-linux-gnueabihf.tar.gz	`b753e4d2cbcf3170...`	`6e22664b98f4dc0c...`
*-arm64-apple-darwin-unsigned.tar.gz	`0da23890907ff0e4...`	`b60bd77e74e5c63c...`
*-arm64-apple-darwin-unsigned.zip	`03a550731d6f916d...`	`fd573063974647d2...`
*-arm64-apple-darwin.tar.gz	`0f1c6d7fd51e93aa...`	`d21e5ba061953bb8...`
*-powerpc64-linux-gnu-debug.tar.gz	`d11c0b00be5deae7...`	`0949ff14b4b518b7...`
*-powerpc64-linux-gnu.tar.gz	`004050b4e0aa297b...`	`0fae6f469dc3c55c...`
*-riscv64-linux-gnu-debug.tar.gz	`4b9974943c7fde6a...`	`a2e1e7e7a83cd83d...`
*-riscv64-linux-gnu.tar.gz	`c9c043b43ac975cc...`	`a922c42a2377bd49...`
*-x86_64-apple-darwin-unsigned.tar.gz	`0f28bd1b9bab6404...`	`66c8be7ef32fa90f...`
*-x86_64-apple-darwin-unsigned.zip	`3ca821a78bf5243a...`	`2350a95de6c815fc...`
*-x86_64-apple-darwin.tar.gz	`cb4902c99574904c...`	`1817369c807405a4...`
*-x86_64-linux-gnu-debug.tar.gz	`e3d2e19faaae8906...`	`d1a5a5a486377acc...`
*-x86_64-linux-gnu.tar.gz	`13782a4c41a807bc...`	`3550b0538f92aa1e...`
*.tar.gz	`1ef40f849dfafbd5...`	`250b7af99e5e3f9e...`
guix_build.log	`3a1f71160fcc6070...`	`8efcd2147ae07ab9...`
guix_build.log.diff		`b2336b8b107af66f...`

DrahtBot removed the label DrahtBot Guix build requested on Aug 21, 2024

hebasto closed this on Aug 28, 2024

depends: Amend handling flags environment variables #30477

Code Coverage

Reviews

Guix builds (on x86_64) [untrusted test-only build, possibly unsafe, not for production use]