depends: Split boost into build/host packages + bump + cleanup

dongcarl commented at 9:15 pm on August 18, 2020: member

This PR improves the robustness of our boost package in depends, most notably:

Bumps boost from 1.70.0 to 1.71.0, because 1.71.0:
1. Removes the need to patch out the unused variable. https://github.com/bitcoin/bitcoin/blob/f8462a6d2794be728cf8550f45d19a354aae59cf/depends/packages/boost.mk#L36 Upstream boost patched it out in https://github.com/boostorg/process/commit/d20b64cf37a773e452b84ce619752e3785be756c, which was first included in the 1.71.0 release
2. Comes packaged with a version of b2 which allows us to override its CXX and CXXFLAGS. Previously, choosing a toolset while building b2 such as clang or gcc would force b2’s build system to invoke the compiler as a bare, hardcoded clang or gcc. However, our depends build system often want to customize this behaviour, adding extra flags or invoking the compiler by an alternate name. So this is useful.
  1. Commit where CXX was introduced: https://github.com/boostorg/build/commit/374f96516a6210687cdf63c987710501634bcac9
  2. Commit where CXXFLAGS was introduced: https://github.com/boostorg/build/commit/5d49abc1f291573d4bdcd2ee647b05a66f9c6497
The boost package is now split into native_b2 and boost, better representing what actually happens.
- In our depends build system, we have a distinction between native packages and non-native packages. The output of native packages are meant to be used on the machine that’s performing the build, and the output of non-native packages are meant to be used on/for the machine that will ultimately be running bitcoin. Previously, boost existed in depends as a non-native package, but that’s partly inaccurate because the ./bootstrap.sh invocation in its $(package)_config_cmds stage actually produced a binary called b2, which is run on the machine that’s performing the build. This means that b2 is a native package which is being built in an environment set up for the non-native package boost. This reveals a hidden unintended behavior in our depends build system: for linux->darwin cross builds, we use gcc for native packages, and clang for non-native packages. But b2 was actually being built using clang, since it was being built in an environment set up for non-native packages.

theuni you might be interested in taking a look

dongcarl added the label Build system on Aug 18, 2020

dongcarl added the label Needs gitian build on Aug 18, 2020

dongcarl added the label Needs Guix build on Aug 18, 2020

Empact commented at 3:52 pm on August 19, 2020: member

Could improve the documentation of these changes by including why each change was made, in addition to what was done. https://chris.beams.io/posts/git-commit/#why-not-how

Speaking as someone who is only so familiar with the build system who would like to learn more.

in depends/packages/boost.mk:31 in 94c18d5aa8 outdated

29@@ -33,18 +30,17 @@ endef
30 
31 # Fix unused variable in boost_process, can be removed after upgrading to 1.72

fanquake commented at 6:09 am on August 20, 2020:

Can drop this comment along with the sed.

dongcarl commented at 4:42 pm on August 25, 2020:

Dropped!

in depends/packages/boost.mk:32 in 94c18d5aa8 outdated

29@@ -33,18 +30,17 @@ endef
30 
31 # Fix unused variable in boost_process, can be removed after upgrading to 1.72
32 define $(package)_preprocess_cmds
33-  sed -i.old "s/int ret_sig = 0;//" boost/process/detail/posix/wait_group.hpp && \
34-  echo "using $($(package)_toolset_$(host_os)) : : $($(package)_cxx) : <cxxflags>\"$($(package)_cxxflags) $($(package)_cppflags)\" <linkflags>\"$($(package)_ldflags)\" <archiver>\"$($(package)_archiver_$(host_os))\" <striper>\"$(host_STRIP)\"  <ranlib>\"$(host_RANLIB)\" <rc>\"$(host_WINDRES)\" : ;" > user-config.jam
35+  echo "using $($(package)_toolset_$(host_os)) : : $($(package)_cxx) : <cflags>\"$($(package)_cflags)\" <cxxflags>\"$($(package)_cxxflags)\" <compileflags>\"$($(package)_cppflags)\" <linkflags>\"$($(package)_ldflags)\" <archiver>\"$($(package)_ar)\" <striper>\"$(host_STRIP)\"  <ranlib>\"$(host_RANLIB)\" <rc>\"$(host_WINDRES)\" : ;" > user-config.jam

fanquake commented at 6:13 am on August 20, 2020:

You’ve dropped _archiver_, but haven’t replaced it with _ar until the following commit (94c18d5aa8ca7325365c1dd4ab310961383dde64), which means 0fc10d153ea1eaa05c39f9706369f11f5ab3944d doesn’t compile.

dongcarl commented at 4:42 pm on August 25, 2020:

Resolved!

in depends/packages/boost.mk:12 in c9adf9d1b4 outdated

 8@@ -9,7 +9,7 @@ define $(package)_set_vars
 9 $(package)_config_opts_release=variant=release
10 $(package)_config_opts_debug=variant=debug
11 $(package)_config_opts=--layout=tagged --build-type=complete --user-config=user-config.jam
12-$(package)_config_opts+=threading=multi link=static -sNO_BZIP2=1 -sNO_ZLIB=1
13+$(package)_config_opts+=threading=multi link=static -sNO_COMPRESSION=1

fanquake commented at 6:29 am on August 20, 2020:

Even though this option shouldn’t affect us, given we don’t build iostreams, this is a nice simplification. Easier to pass sNO_COMPRESSION and not worry about new types being added in future, as we were already missing NO_LZMA and NO_ZSTD.

in depends/packages/native_b2.mk:7 in 94c18d5aa8 outdated

0@@ -0,0 +1,19 @@
1+package=native_b2
2+$(package)_version=$(boost_version)
3+$(package)_download_path=$(boost_download_path)
4+$(package)_file_name=$(boost_file_name)
5+$(package)_sha256_hash=$(boost_sha256_hash)
6+$(package)_build_subdir=tools/build/src/engine
7+$(package)_toolset_$(host_os)=gcc
8+ifneq (,$(findstring clang,$($(package)_cxx)))

fanquake commented at 6:48 am on August 20, 2020:

This doesn’t seem to be working as expected. When I pass CC=clang CXX=clang++, native_b2 is still built using the GCC toolset (Boost is built with Clang). I modified this to use a similar change as you’ve made above, i.e:

 0diff --git a/depends/packages/native_b2.mk b/depends/packages/native_b2.mk
 1index 184c203d9..aaa37cdcf 100644
 2--- a/depends/packages/native_b2.mk
 3+++ b/depends/packages/native_b2.mk
 4@@ -4,9 +4,10 @@ $(package)_download_path=$(boost_download_path)
 5 $(package)_file_name=$(boost_file_name)
 6 $(package)_sha256_hash=$(boost_sha256_hash)
 7 $(package)_build_subdir=tools/build/src/engine
 8-$(package)_toolset_$(host_os)=gcc
 9 ifneq (,$(findstring clang,$($(package)_cxx)))
10-   $(package)_toolset_$(host_os)=clang
11+$(package)_toolset_$(host_os)=clang
12+else
13+$(package)_toolset_$(host_os)=gcc
14 endif

However that doesn’t seem to work either.

If I just set $(package)_toolset_$(host_os)=clang, then it spits out:

0Building native_b2...
1###
2###
3### Using 'clang' toolset.
4###
5###
6g++ --version
7g++ (Debian 10.1.0-6) 10.1.0

but given the version number that still looks like it’s GCC. I might be missing something.

dongcarl commented at 4:38 pm on August 25, 2020:

Ah, the reason why setting CC=clang and CXX=clang++ doesn’t work is because CC and CXX only control the host_{CC,CXX}, and since native_b2 is a native package, its $(package)_cxx derives its value from build_CXX. So setting build_CXX=clang should work.

As for the case of just setting $(package)_toolset_$(host_os)=clang, $(package)_toolset_$(host_os) strictly depends on $(package)_cxx, so changing that by itself won’t influence $(package)_cxx. Since it’s a strict dependency, we should probably remove this variable at some point in the future and replace it with a simple $(if $(findstring clang,$($(package)_cxx)),clang,gcc) to avoid confusion.

fanquake commented at 5:59 am on October 1, 2020:

Yep, thanks for clarifying.

fanquake commented at 6:52 am on August 20, 2020: member

Concept ACK. Looks like a good cleanup. Bit of a bummer (for slower systems) that we’ll have to extract Boost twice now. Do you think using bcp is on the roadmap after this?

DrahtBot commented at 7:51 pm on August 20, 2020: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#19245 ([WIP DONOTMERGE] Replace boost::filesystem with std::filesystem (in c++17) by kiminuo)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot removed the label Needs gitian build on Aug 21, 2020

DrahtBot removed the label Needs Guix build on Aug 23, 2020

dongcarl force-pushed on Aug 25, 2020

dongcarl commented at 4:43 pm on August 25, 2020: member

@Empact I updated the description with more justification and context, please let me know if anything’s unclear!

DrahtBot added the label Needs rebase on Aug 27, 2020

theuni commented at 8:06 pm on August 27, 2020: member

Concept ACK.

Bit of a bummer (for slower systems) that we’ll have to extract Boost twice now. Do you think using bcp is on the roadmap after this?

Very true, I had completely forgotten about this overhead when we discussed this. +1 for looking into reducing the size of the contents of the tarballs.

Also, it’s worth investigating switching from the boost .tar.bz2 to the .tar.gz. Maybe that can make up for some of the slowdown at the cost of a size increase?

in depends/funcs.mk:189 in d25e0e308f outdated

185@@ -186,7 +186,7 @@ $($(1)_preprocessed): | $($(1)_extracted)
186 	$(AT)$(foreach patch,$($(1)_patches),cd $(PATCHES_PATH)/$(1); cp $(patch) $($(1)_patch_dir) ;)
187 	$(AT)cd $$(@D); $(call $(1)_preprocess_cmds, $(1))
188 	$(AT)touch $$@
189-$($(1)_configured): | $($(1)_dependencies) $($(1)_preprocessed)
190+$($(1)_configured): | $($(1)_all_dependencies) $($(1)_preprocessed)

theuni commented at 8:18 pm on August 27, 2020:

What does this fix?

dongcarl commented at 0:41 am on August 28, 2020:

$($(1)_dependencies) doesn’t include $($($(1)_type)_native_toolchain) $($($(1)_type)_native_binutils), meaning that:

0$ make -C depends/ HOST=x86_64-apple-darwin16 print-zlib_dependencies
1make: Entering directory '/home/dongcarl/src/bitcoin/master/depends'
2zlib_dependencies =
3make: Leaving directory '/home/dongcarl/src/bitcoin/master/depends'

while

0$ make -C depends/ HOST=x86_64-apple-darwin16 print-zlib_all_dependencies
1make: Entering directory '/home/dongcarl/src/bitcoin/master/depends'
2zlib_all_dependencies = native_cctools
3make: Leaving directory '/home/dongcarl/src/bitcoin/master/depends'

This fixes: #19799

I’d be happy to leave this for another PR, and figure out what the right fix is.

hebasto commented at 10:44 am on September 4, 2020:

Please see an alternative #19868.

hebasto commented at 8:17 am on September 16, 2020: member

Concept ACK. It would be nice to document native_* packages usage in packages.md (maybe in a follow up pull).

MarcoFalke referenced this in commit 43305e9810 on Sep 23, 2020

sidhujag referenced this in commit c283427bc1 on Sep 23, 2020

dongcarl force-pushed on Sep 23, 2020

dongcarl commented at 7:11 pm on September 23, 2020: member

Rebased over #19868 and dropped my solution.

practicalswift commented at 8:18 pm on September 23, 2020: contributor

Concept ACK

DrahtBot removed the label Needs rebase on Sep 23, 2020

ryanofsky approved

ryanofsky commented at 4:02 pm on September 25, 2020: member

Basic code review ACK 68d53e8892c71083f4e91931e519c2ac82dfd7a7. I was looking at this and didn’t dig into all the details of all the changes, but did think they all look safe and sane and were well motivated.

I guess most ideally the boost build system would accept different build and host toolchains in its configuration, and use the right tools for the right things, so everything could be built in one package. But I guess its bootstrap script isn’t really written to work this way, so the separate native b2 package makes sense and is a reasonable way to get the right compiler used.

MarcoFalke deleted a comment on Sep 26, 2020

fanquake changes_requested

fanquake commented at 6:03 am on October 1, 2020: member

This looks good, great PR description 👍 . However in 6cba51ea9fc9c7265a7afa3e77ac6c20937e822b you’ve forgotten to remove the unused_var_in_process.patch file.

dongcarl force-pushed on Nov 6, 2020

dongcarl commented at 3:38 pm on November 6, 2020: member

:facepalm: Rebased and removed the patch! Should be good to go!

MarcoFalke added the label Needs gitian build on Nov 7, 2020

MarcoFalke added the label Needs Guix build on Nov 7, 2020

DrahtBot commented at 7:05 am on November 9, 2020: member

Guix builds

File	commit 7e373294a5ae819099c39d9d03d1f5a311d63cfc(master)	commit be7cd174e98a82f0d62c6382760e895a040a931a(master and this pull)
*-aarch64-linux-gnu-debug.tar.gz	`09a0121d77ec4be2...`	`8bf4e65878ae7e6d...`
*-aarch64-linux-gnu.tar.gz	`61ebe7e7457e8e62...`	`da5cd139dc74ee97...`
*-arm-linux-gnueabihf-debug.tar.gz	`c6391f6e756c2c54...`	`a276ab14cbab22d0...`
*-arm-linux-gnueabihf.tar.gz	`20ca716878c7b7e9...`	`e47328028673d2aa...`
*-riscv64-linux-gnu-debug.tar.gz	`042869d65b80f71d...`	`42b934c25b59b9b5...`
*-riscv64-linux-gnu.tar.gz	`c14c36a76ca17ab1...`	`ea647cf7ac494427...`
*-win-unsigned.tar.gz	`19a8a9eb258b28b0...`	`a319cff79be57cad...`
*-win64-debug.zip	`67d403931ddf1a25...`	`1fa6816e2878b843...`
*-win64-setup-unsigned.exe	`2dba9dcd4c6dcbe1...`	`5893786f286551c0...`
*-win64.zip	`d80dbf078c448b2f...`	`39d49939396262c9...`
*-x86_64-linux-gnu-debug.tar.gz	`aaf23ef58ab12417...`	`6ff5119ec082ac61...`
*-x86_64-linux-gnu.tar.gz	`60ffa8b26ec97535...`	`77d61eae861a289f...`
*.tar.gz	`807305e2b5af4d43...`	`2910c6ae2ef39128...`
guix_build.log	`6d0e747c7c923549...`	`8d6a7ffeda747f2f...`
guix_build.log.diff		`19ab4fd999c4c15e...`

DrahtBot removed the label Needs Guix build on Nov 9, 2020

DrahtBot commented at 8:11 am on November 11, 2020: member

Gitian builds

File	commit 7e373294a5ae819099c39d9d03d1f5a311d63cfc(master)	commit be7cd174e98a82f0d62c6382760e895a040a931a(master and this pull)
*-aarch64-linux-gnu-debug.tar.gz	`83c77088c64ce35f...`	`8fe8a34390a5e151...`
*-aarch64-linux-gnu.tar.gz	`36ed972b65709cc9...`	`ca83ce0a1cfaa07f...`
*-arm-linux-gnueabihf-debug.tar.gz	`116e04dba520d05a...`	`fd60fba116de4f8c...`
*-arm-linux-gnueabihf.tar.gz	`ac7ef6850d92f55c...`	`3b1c84e55d1cdcb5...`
*-osx-unsigned.dmg	`8881d819b5a03e1d...`	`e4b96f56cf27d9b1...`
*-osx64.tar.gz	`bcf7429bd9891ff3...`	`a40095438c8b2d71...`
*-riscv64-linux-gnu-debug.tar.gz	`9ead8bb9823b686d...`	`193f2ce6abd4e365...`
*-riscv64-linux-gnu.tar.gz	`98bcd32e7c3bd042...`	`a88805fc5985fd95...`
*-win64-debug.zip	`7f60cd29a7864522...`	`64b399dc41a67f08...`
*-win64-setup-unsigned.exe	`c081d7eedbe5b1a7...`	`54d74ef7ac4bf51b...`
*-win64.zip	`ac8172f935e15a54...`	`a58506ae7f5ff59e...`
*-x86_64-linux-gnu-debug.tar.gz	`7523c4786d05da01...`	`ad4024b5c67dae0a...`
*-x86_64-linux-gnu.tar.gz	`1c8142e21d050a46...`	`aaad1f6bbf6fb746...`
*.tar.gz	`807305e2b5af4d43...`	`2910c6ae2ef39128...`
bitcoin-core-linux-0.21-res.yml	`cdcebbbe4862da12...`	`da602a49c39086c6...`
bitcoin-core-osx-0.21-res.yml	`9391742ea84e255a...`	`6bf1674317b65818...`
bitcoin-core-win-0.21-res.yml	`a323be37316096c5...`	`ebc92564c29491cd...`
linux-build.log	`a02c01eaf94d10bd...`	`61e302176c3457ce...`
osx-build.log	`906bb37b69cf3ef3...`	`ec15b69402232249...`
win-build.log	`a643a56926f8f128...`	`7218f254e77a62bc...`
bitcoin-core-linux-0.21-res.yml.diff		`81c2211c2f8fc8e9...`
bitcoin-core-osx-0.21-res.yml.diff		`8cc2bc1f4cdf95f1...`
bitcoin-core-win-0.21-res.yml.diff		`aa8421d171bbaf97...`
linux-build.log.diff		`8f69192d0cef2372...`
osx-build.log.diff		`5032e8f5e703ff42...`
win-build.log.diff		`d3cc10154a2ebf5b...`

DrahtBot removed the label Needs gitian build on Nov 11, 2020

fanquake commented at 5:15 am on November 12, 2020: member

Rebased and removed the patch! Should be good to go!

Thanks. We’ll merge this post branch-off.

MarcoFalke added this to the milestone 22.0 on Nov 12, 2020

depends: boost: Refer to version in URL 800655ff31

depends: boost: Bump to 1.71.0 a57b498560

depends: boost: Split into non-/native packages 9cf2ee54d3

depends: boost: Disable all compression d7048fa73f

depends: boost: Cleanup architecture/address-model 86002e7e90

depends: boost: Cleanup toolset selection ab9e047cc2

depends: boost: Remove unnecessary _archiver_

We already have $(package)_ar, so just use that instead

b2328b7989

depends: boost: Specify cflags+compileflags f190343c96

dongcarl force-pushed on Nov 23, 2020

dongcarl commented at 8:52 pm on November 23, 2020: member

Pushed a pure-rebase. Still ready for merge!

MarcoFalke added the label Needs gitian build on Nov 24, 2020

MarcoFalke added the label Needs Guix build on Nov 24, 2020

fanquake added this to the "PRs" column in a project

laanwj commented at 9:31 am on November 24, 2020: member

Concept and code review ACK f190343c96520db254d6689f8f24c9eb36bead6b

laanwj merged this on Nov 24, 2020

laanwj closed this on Nov 24, 2020

laanwj moved this from the "PRs" to the "Done" column in a project

laanwj referenced this in commit 0918eb49d5 on Nov 24, 2020

MarcoFalke referenced this in commit 6f9dac5ede on Nov 24, 2020

sidhujag referenced this in commit bd97aa3694 on Nov 24, 2020

sidhujag referenced this in commit 3bc5a35fe1 on Nov 24, 2020

MarcoFalke removed the label Needs Guix build on Nov 25, 2020

MarcoFalke removed the label Needs gitian build on Nov 25, 2020

zkbot referenced this in commit a0158ce837 on Jan 15, 2021

furszy referenced this in commit 8d2207cb8b on May 9, 2021

kittywhiskers referenced this in commit fee7bfa822 on Aug 22, 2021

kittywhiskers referenced this in commit ff42f3689b on Aug 22, 2021

kittywhiskers referenced this in commit 6a367fc198 on Aug 27, 2021

kittywhiskers referenced this in commit a003178978 on Aug 27, 2021

PastaPastaPasta referenced this in commit bbc8623245 on Aug 31, 2021

PastaPastaPasta referenced this in commit 721112247d on Sep 17, 2021

PastaPastaPasta referenced this in commit 08c66a27b4 on Sep 24, 2021

kittywhiskers referenced this in commit 3d8a358734 on Oct 12, 2021

DrahtBot locked this on Feb 15, 2022

depends: Split boost into build/host packages + bump + cleanup #19764

Conflicts

Guix builds

Gitian builds