Adding option for downloading Boost using CMake FetchContent #6424

vrnimje · 2024-01-23T16:38:05Z

Fixes #6392

Proposed Changes

Adding HPX_WITH_FETCH_BOOST bool option for fetching Boost v1.84.0 using CMake FetchContent
Adding CI tests for the above on macOS, Linux and Windows (VS2022, Debug)

Checklist

I have added a new feature and have added tests to go along with it.
I have fixed a bug and have added a regression test.

Signed-off-by: Vedant <[email protected]>

jenkins-cscs · 2024-01-23T16:39:41Z

Can one of the admins verify this patch?

Signed-off-by: Vedant <[email protected]>

SAtacker · 2024-01-23T16:48:28Z

CMakeLists.txt

+hpx_option(
+  HPX_WITH_FETCH_BOOST
+  BOOL
+  "Use FetchContent to fetch Asio. By default an installed Asio will be used. (default: OFF)"


Please change description

SAtacker · 2024-01-23T16:48:51Z

cmake/HPX_SetupBoost.cmake

+  set(Boost_DIR "${CMAKE_BINARY_DIR}/_deps/boost-src")
+  set(Boost_INCLUDE_DIR "${CMAKE_BINARY_DIR}/_deps/boost-src")


Are we sure this should not be internal?

I guess it makes sense to keep it Internal, as it is set when the option is used, so should not be visible
Will make the changes

SAtacker · 2024-01-23T16:49:20Z

cmake/HPX_SetupBoost.cmake

+endif()
+endif()


There are two endifs, does it not cause errors?

Nevermind, seems like you have fixed it

codacy-production · 2024-01-23T19:12:55Z

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation	Diff coverage
✅ 0.00%	✅ ∅

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (`8365e76`)	201880	171929	85.16%
Head commit (`437f365`)	190686 (-11194)	162390 (-9539)	85.16% (0.00%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#6424)	0	0	∅ (not applicable)

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings Change summary preferences

_{You may notice some variations in coverage metrics with the latest Coverage engine update. For more details, visit the documentation}

Signed-off-by: Vedant <[email protected]>

SAtacker · 2024-01-24T15:18:58Z

@hkaiser PTAL, thanks!

hkaiser · 2024-01-25T15:18:40Z

cmake/HPX_SetupBoost.cmake

+      execute_process(
+        COMMAND
+          cmd /C
+          "cd ${CMAKE_BINARY_DIR}\\_deps\\boost-src && .\\bootstrap.bat && .\\b2 headers cxxflags=/std:c++20"


Should we compile with the C++ version that corresponds to what the user requested for HPX (HPX_CXX_STANDARD)?

Yes, we could do that. Will make the changes

hkaiser · 2024-01-25T15:19:15Z

cmake/HPX_SetupBoost.cmake

+      execute_process(
+        COMMAND
+          sh -c
+          "cd ${CMAKE_BINARY_DIR}/_deps/boost-src && ./bootstrap.sh --prefix=${CMAKE_BINARY_DIR}/_deps/boost-installed && ./b2 && ./b2 install --prefix=${CMAKE_BINARY_DIR}/_deps/boost-installed cxxflags=--std=c++20"


Same here (and in more places below). Should we use HPX_CXX_STANDARD?

hkaiser · 2024-01-25T15:19:33Z

cmake/HPX_SetupBoost.cmake

+          "cd ${CMAKE_BINARY_DIR}/_deps/boost-src && ./bootstrap.sh && ./b2 headers cxxflags=--std=c++20"
+      )
+    endif()
+    set(EX_PRC_INTERNAL


What is this used for?

It makes sure that the commands defined to build Boost are not executed again, if the project is reconfigured
But I noticed that I have mistakenly defined it as INTERNAL instead of STRING above. Will make the changes

If this is something that is used outside of a CMake configuration step (e.g. needed for configuring dependent applications), please prefix the variable name with HPX_WITH_ for consistency and to avoid name clashes.

Yes, it is used for configuring the building of fetched Boost, so will update with the variable naming you have mentioned.

hkaiser · 2024-01-25T15:22:44Z

cmake/HPX_SetupBoost.cmake

+      "${CMAKE_BINARY_DIR}/_deps/boost-src"
+      CACHE INTERNAL ""
+  )
+endif()


Should all of the above go into a separate file FindBoost.cmake or similar (just for consistency reasons)? OTOH, this may conflict with the file with the same name in the CMake distribution...

…c changes Signed-off-by: Vedant <[email protected]>

Signed-off-by: Vedant <[email protected]>

hkaiser · 2024-01-27T20:25:40Z

cmake/HPX_SetupBoost.cmake

+      execute_process(
+        COMMAND
+          sh -c
+          "cd ${CMAKE_BINARY_DIR}/_deps/boost-src && ./bootstrap.sh && ./b2 headers cxxflags=--std=c++${HPX_WITH_CXX_STANDARD}"


Suggested change

"cd ${CMAKE_BINARY_DIR}/_deps/boost-src && ./bootstrap.sh && ./b2 headers cxxflags=--std=c++${HPX_WITH_CXX_STANDARD}"

"cd ${CMAKE_BINARY_DIR}/_deps/boost-src && ./bootstrap.sh && ./b2 headers cxxflags=--std=c++${HPX_CXX_STANDARD}"

hkaiser · 2024-01-27T20:26:18Z

cmake/HPX_SetupBoost.cmake

+    DOWNLOAD_EXTRACT_TIMESTAMP true
+  )
+  fetchcontent_populate(Boost)
+  set(HPX_WITH_EX_PRC


Would you mind making this variable a bit more self-explanatory?

Signed-off-by: Vedant <[email protected]>

SAtacker · 2024-03-26T15:39:15Z

What is stopping us here @vrnimje @hkaiser ?

vrnimje · 2024-03-26T16:01:33Z

What is stopping us here @vrnimje @hkaiser ?

I have made all the requested changes from my side, as far as I know.
Just pending more reviews / confirmation

vrnimje added 5 commits January 23, 2024 22:01

Adding boost fetch option and corresponding workflow tests

dfe2bbe

Signed-off-by: Vedant <[email protected]>

Added copyright, and renamed file for linux workflow

2c7a068

Signed-off-by: Vedant <[email protected]>

Set activation for macos workflows to pull_request only

2942a74

Signed-off-by: Vedant <[email protected]>

Fixing order of steps in macOS workflow

23c1d49

Signed-off-by: Vedant <[email protected]>

Added variable for Boost variables, and made some misc changes

0b69e30

Signed-off-by: Vedant <[email protected]>

vrnimje requested a review from hkaiser as a code owner January 23, 2024 16:38

vrnimje added 2 commits January 23, 2024 22:11

Applying cmake-format to Boost setup file

0287f31

Signed-off-by: Vedant <[email protected]>

Removing left-out endif() statement

d0f3a32

Signed-off-by: Vedant <[email protected]>

SAtacker suggested changes Jan 23, 2024

View reviewed changes

Adding description for fetch option

8a8d6ac

Signed-off-by: Vedant <[email protected]>

hkaiser added category: CMake type: enhancement type: compatibility issue labels Jan 25, 2024

hkaiser requested changes Jan 25, 2024

View reviewed changes

vrnimje added 2 commits January 26, 2024 15:38

Modified commands to build boost using HPX_CXX_STANDARD, and made mis…

505853d

…c changes Signed-off-by: Vedant <[email protected]>

Changing variable names for consistency

ecc252e

Signed-off-by: Vedant <[email protected]>

hkaiser reviewed Jan 27, 2024

View reviewed changes

Renaming variables to be self-explanatory

437f365

Signed-off-by: Vedant <[email protected]>

hkaiser approved these changes Apr 3, 2024

View reviewed changes

hkaiser merged commit fe66c1b into STEllAR-GROUP:master Apr 3, 2024
44 of 50 checks passed

hkaiser added this to the 1.10.0 milestone May 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding option for downloading Boost using CMake FetchContent #6424

Adding option for downloading Boost using CMake FetchContent #6424

vrnimje commented Jan 23, 2024

jenkins-cscs commented Jan 23, 2024

SAtacker Jan 23, 2024

SAtacker Jan 23, 2024

vrnimje Jan 23, 2024 •

edited

Loading

SAtacker Jan 23, 2024

SAtacker Jan 23, 2024

codacy-production bot commented Jan 23, 2024 •

edited

Loading

SAtacker commented Jan 24, 2024

hkaiser Jan 25, 2024

vrnimje Jan 25, 2024

hkaiser Jan 25, 2024

hkaiser Jan 25, 2024

vrnimje Jan 25, 2024 •

edited

Loading

hkaiser Jan 26, 2024 •

edited

Loading

vrnimje Jan 26, 2024 •

edited

Loading

hkaiser Jan 25, 2024

hkaiser Jan 27, 2024

hkaiser Jan 27, 2024

SAtacker commented Mar 26, 2024

vrnimje commented Mar 26, 2024

		set(Boost_DIR "${CMAKE_BINARY_DIR}/_deps/boost-src")
		set(Boost_INCLUDE_DIR "${CMAKE_BINARY_DIR}/_deps/boost-src")

	"cd ${CMAKE_BINARY_DIR}/_deps/boost-src && ./bootstrap.sh && ./b2 headers cxxflags=--std=c++${HPX_WITH_CXX_STANDARD}"
	"cd ${CMAKE_BINARY_DIR}/_deps/boost-src && ./bootstrap.sh && ./b2 headers cxxflags=--std=c++${HPX_CXX_STANDARD}"

Adding option for downloading Boost using CMake FetchContent #6424

Adding option for downloading Boost using CMake FetchContent #6424

Conversation

vrnimje commented Jan 23, 2024

Proposed Changes

Checklist

jenkins-cscs commented Jan 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vrnimje Jan 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codacy-production bot commented Jan 23, 2024 • edited Loading

Coverage summary from Codacy

SAtacker commented Jan 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vrnimje Jan 25, 2024 • edited Loading

Choose a reason for hiding this comment

hkaiser Jan 26, 2024 • edited Loading

Choose a reason for hiding this comment

vrnimje Jan 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SAtacker commented Mar 26, 2024

vrnimje commented Mar 26, 2024

vrnimje Jan 23, 2024 •

edited

Loading

codacy-production bot commented Jan 23, 2024 •

edited

Loading

vrnimje Jan 25, 2024 •

edited

Loading

hkaiser Jan 26, 2024 •

edited

Loading

vrnimje Jan 26, 2024 •

edited

Loading