Skip to content

Conversation

theuni
Copy link
Member

@theuni theuni commented Jul 11, 2024

Marked as draft because this is much more relevant after we've switched to CMake.

This has a few advantages over the old method of simply copying headers:

  • Installs proper cmake files which can be picked up by our buildsystem
  • Only installs necessary headers, not all of boost

The only drawback is that it builds a few libs that we end up throwing away. date_time and test can both be optionally used header-only (which we do), but boost's CMake buildsystem doesn't expose an option to skip building them.

@DrahtBot
Copy link
Contributor

DrahtBot commented Jul 11, 2024

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/30434.

Reviews

See the guideline for information on the review process.
A summary of reviews will appear here.

@theuni
Copy link
Member Author

theuni commented Jul 11, 2024

Ping @hebasto. Tested on cmake-staging with Boost_NO_BOOST_CMAKE removed. Seems to work as expected. Though ofc we wouldn't be able to remove that until we require boost 1.82 for non-depends builds.

@maflcko
Copy link
Member

maflcko commented Jul 12, 2024

The CI failure https://cirrus-ci.com/task/4950868422819840:

Extracting boost...
/ci_container_base/depends/sources/boost-1.85.0-cmake.tar.gz: OK
Preprocessing boost...
Configuring boost...
-- The CXX compiler identification is Clang 18.1.3
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /usr/bin/env - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Boost: using system layout: include, bin, lib, lib/cmake
-- Boost: Release build, static libraries, MPI OFF, Python OFF, testing OFF
-- Boost: libraries included: date_time;multi_index;signals2;test
-- The C compiler identification is Clang 18.1.3
CMake Error at /usr/share/cmake-3.28/Modules/CMakeFindBinUtils.cmake:251 (message):
  Could not find install_name_tool, please check your installation.
Call Stack (most recent call first):
  /usr/share/cmake-3.28/Modules/CMakeDetermineCCompiler.cmake:196 (include)
  libs/container/CMakeLists.txt:7 (project)


-- Configuring incomplete, errors occurred!
make: *** [funcs.mk:302: /ci_container_base/depends/x86_64-apple-darwin/.boost_stamp_configured] Error 1
make: Leaving directory '/ci_container_base/depends'

Exit status: 2

@hebasto
Copy link
Member

hebasto commented Jul 12, 2024

The CI failure https://cirrus-ci.com/task/4950868422819840:

Maybe revert 3bee514?

FWIW, in the CMake staging branch, set(CMAKE_PLATFORM_HAS_INSTALLNAME FALSE) is used in between the project() and enable_language(CXX) calls to workaround it:

#=============================
# Language setup
#=============================
if(CMAKE_SYSTEM_NAME STREQUAL "Darwin" AND NOT CMAKE_HOST_APPLE)
  # We do not use the install_name_tool when cross-compiling for macOS.
  # So disable this tool check in further enable_language() commands.
  set(CMAKE_PLATFORM_HAS_INSTALLNAME FALSE)
endif()
enable_language(CXX)

@fanquake
Copy link
Member

Maybe revert 3bee514?

A better solution would be to fix the CMake build systems, so we don't have to pointlessly compile stub libraries, that we ultimately don't even use.

@theuni
Copy link
Member Author

theuni commented Jul 12, 2024

Pushed a workaround hack in the meantime.

@fanquake
Copy link
Member

@theuni want to rebase this, (and even kick it along to 1.86.0?), to see what works, what breaks etc.

@theuni theuni force-pushed the cmake-boost-depends branch 2 times, most recently from bdbe350 to 43bd397 Compare August 29, 2024 19:28
@theuni theuni changed the title depends: bump boost to 1.85.0 and use new CMake buildsystem depends: bump boost to 1.86.0 and use new CMake buildsystem Aug 29, 2024

define $(package)_set_vars
$(package)_config_opts=-DBOOST_INCLUDE_LIBRARIES="date_time;multi_index;signals2;test" -DBOOST_INSTALL_LAYOUT=system
$(package)_config_opts_darwin=-DCMAKE_PLATFORM_HAS_INSTALLNAME="FALSE"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that the previous variant is correct:

Suggested change
$(package)_config_opts_darwin=-DCMAKE_PLATFORM_HAS_INSTALLNAME="FALSE"
$(package)_config_opts_darwin=-DCMAKE_INSTALL_NAME_TOOL=true

@hebasto
Copy link
Member

hebasto commented Nov 1, 2024

@fanquake
Copy link
Member

Boost 1.87.0 is out (https://www.boost.org/users/history/version_1_87_0.html). Note that we no-longer care about date_time after #31391, but the build changes to test haven't made it in yet: boostorg/test#426.

This has a few advantages over the old method of simply copying headers:
- Installs proper cmake files which can be picked up by our buildsystem
- Only installs necessary headers, not all of boost

The only drawback is that it builds a few libs that we end up throwing away.
date_time and test can both be optionally used header-only (which we do), but
boost's CMake buildsystem doesn't expose an option to skip building them.
@theuni theuni force-pushed the cmake-boost-depends branch from 43bd397 to 5ecaad0 Compare January 24, 2025 16:05
@theuni
Copy link
Member Author

theuni commented Jan 24, 2025

Updated:

  • bumped to 1.87
  • remove date_time
  • reverted to the working install_name_tool hack

@theuni theuni changed the title depends: bump boost to 1.86.0 and use new CMake buildsystem depends: bump boost to 1.87.0 and use new CMake buildsystem Jan 24, 2025
@hebasto
Copy link
Member

hebasto commented Apr 7, 2025

Boost 1.87.0 is out (https://www.boost.org/users/history/version_1_87_0.html). Note that we no-longer care about date_time after #31391, but the build changes to test haven't made it in yet: boostorg/test#426.

boostorg/test#426 has just been merged.

@hebasto
Copy link
Member

hebasto commented Apr 15, 2025

The only drawback is that it builds a few libs that we end up throwing away. date_time and test can both be optionally used header-only (which we do), but boost's CMake buildsystem doesn't expose an option to skip building them.

Here is an updated branch with Boost bumped to 1.88.0, and it avoids any compilation altogether.

@theuni

Feel free to pick it up.

@theuni
Copy link
Member Author

theuni commented Apr 16, 2025

@hebasto Want to try putting that behind a variable and upstreaming it? It'd be nice if we didn't have to carry that patch.

@hebasto
Copy link
Member

hebasto commented Apr 16, 2025

@hebasto Want to try putting that behind a variable and upstreaming it? It'd be nice if we didn't have to carry that patch.

Sure thing!

Done in boostorg/test#445.

@hebasto
Copy link
Member

hebasto commented Apr 17, 2025

@hebasto Want to try putting that behind a variable and upstreaming it? It'd be nice if we didn't have to carry that patch.

Sure thing!

Done in boostorg/test#445.

Here is an updated branch based on the upstream changes.

@hebasto
Copy link
Member

hebasto commented Apr 17, 2025

@hebasto Want to try putting that behind a variable and upstreaming it? It'd be nice if we didn't have to carry that patch.

Sure thing!

Done in boostorg/test#445.

boostorg/test#445 has just been merged.

@fanquake
Copy link
Member

fanquake commented Jun 2, 2025

I've pulled @hebasto's newer branch into #32665.

@fanquake fanquake closed this Jun 2, 2025
fanquake added a commit that referenced this pull request Jun 26, 2025
…stem

6c2538d depends: Bump boost to 1.88.0 and use new CMake buildsystem (Cory Fields)

Pull request description:

  Originally #30434.

  This has a few advantages over the old method of simply copying headers:
  - Installs proper CMake files which can be picked up by our buildsystem
  - Only installs necessary headers, not all of Boost

  Pulls in upstreamed boostorg/test#445.

ACKs for top commit:
  willcl-ark:
    tACK 6c2538d
  hebasto:
    re-ACK 6c2538d, only rebased since my previous [review](#32665 (review)).

Tree-SHA512: fc3fce77b21c8ea500370841f44f1cc87e0bb09cdde55f75d2f90853cb06a6f8c73ac6ca9ca3e91a879e9f95dd59aa40254c1b04e7a168c52fa1b31cc5b7f537
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants