Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Boost CMake module #3731

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

shrijitsingh99
Copy link
Contributor

Referenced in #2559.
Removed the replicated supported versions & cleaned up the modules section.
@SergioRAgostinho Is this what you had in mind?

Copy link
Member

@SergioRAgostinho SergioRAgostinho left a comment

Choose a reason for hiding this comment

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

You got the general idea but your solution still needs work. With the current approach you're breaking pcl's the config file. Downstream projects, like the tutorials job, are not configuring properly.

Have a look at what was done in #1421.

pcl_find_boost.cmake, needs to be migrated into it's own FindBoost.cmake module inside pcl/cmake/Modules. That way it can be invoked in both stages: when compiling pcl and when configuring a downstream project.

cmake/pcl_find_boost.cmake Outdated Show resolved Hide resolved
cmake/pcl_find_boost.cmake Outdated Show resolved Hide resolved
PCLConfig.cmake.in Outdated Show resolved Hide resolved
cmake/pcl_find_boost.cmake Outdated Show resolved Hide resolved
@SergioRAgostinho SergioRAgostinho added changelog: enhancement Meta-information for changelog generation module: cmake needs: more work Specify why not closed/merged yet labels Mar 12, 2020
@shrijitsingh99
Copy link
Contributor Author

You got the general idea but your solution still needs work. With the current approach you're breaking pcl's the config file. Downstream projects, like the tutorials job, are not configuring properly.

Have a look at what was done in #1421.

pcl_find_boost.cmake, needs to be migrated into it's own FindBoost.cmake module inside pcl/cmake/Modules. That way it can be invoked in both stages: when compiling pcl and when configuring a downstream project.

Got what you are trying to say, was initially thinking of doing something like suggested but wasn't sure then. Will incorporate the changes suggested in the next commit.

cmake/pcl_find_boost.cmake Outdated Show resolved Hide resolved
cmake/pcl_find_boost.cmake Outdated Show resolved Hide resolved
@shrijitsingh99
Copy link
Contributor Author

What's the idea behind setting separate variables like BOOST_INCLUDE_DIRS when Boost_INCLUDE_DIRS is already set?

@taketwo
Copy link
Member

taketwo commented Mar 13, 2020

Consider these lines:

pcl/PCLConfig.cmake.in

Lines 321 to 325 in 2133857

string(TOUPPER "${_component}" COMPONENT)
string(TOUPPER "${_lib}" LIB)
string(REGEX REPLACE "[.-]" "_" LIB ${LIB})
if(${LIB}_FOUND)
list(APPEND PCL_${COMPONENT}_INCLUDE_DIRS ${${LIB}_INCLUDE_DIRS})

We try to treat all external dependencies uniformly, thus the convention is that finder scripts set variables prefixed with capitalized name of the dependency. If it wasn't the case, we'd need to keep track of which library is called Library, which library, and which LIBRARY.

@shrijitsingh99 shrijitsingh99 force-pushed the boost-cmake branch 2 times, most recently from d18b9eb to e78a39d Compare April 18, 2020 11:12
@shrijitsingh99
Copy link
Contributor Author

shrijitsingh99 commented Apr 18, 2020

pcl_find_boost.cmake, needs to be migrated into it's own FindBoost.cmake module inside pcl/cmake/Modules. That way it can be invoked in both stages: when compiling pcl and when configuring a downstream project.

Made the appropriate changes.

I thought of using pkg-config initially, but it seems Boost doesn't have support for it currently.

@shrijitsingh99 shrijitsingh99 marked this pull request as ready for review April 22, 2020 10:26
@shrijitsingh99 shrijitsingh99 changed the title Combine pcl_find_boost.cmake and find_boost macro in PCLConfig.cmake Add Boost CMake module Apr 22, 2020
cmake/Modules/FindBoost.cmake Show resolved Hide resolved
find_package(Boost 1.55.0 ${QUIET_} COMPONENTS @PCLCONFIG_AVAILABLE_BOOST_MODULES@)

set(BOOST_FOUND ${Boost_FOUND})
Copy link
Member

Choose a reason for hiding this comment

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

Would this stop overriding the Boost_LIBRARIES variable for anyone consuming PCL? There's an issue open (and verified) which says that find_package(Boost) before find_package(PCL) causes PCL to override the Boost libraries

Copy link
Member

Choose a reason for hiding this comment

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

Also, what's the relationship of pcl_find_boost.cmake with Modules/FindBoost.cmake (because I notice it's untouched in this PR)

Copy link
Contributor Author

@shrijitsingh99 shrijitsingh99 May 22, 2020

Choose a reason for hiding this comment

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

Also, what's the relationship of pcl_find_boost.cmake with Modules/FindBoost.cmake (because I notice it's untouched in this PR)

That is to be removed.

Would this stop overriding the Boost_LIBRARIES variable for anyone consuming PCL? There's an issue open (and verified) which says that find_package(Boost) before find_package(PCL) causes PCL to override the Boost libraries

Currently, no. It should be possible though to append the already existing found components to the list of found components.

@shrijitsingh99 shrijitsingh99 marked this pull request as draft May 22, 2020 11:59
@stale
Copy link

stale bot commented Jun 21, 2020

Marking this as stale due to 30 days of inactivity. Commenting or adding a new commit to the pull request will revert this.

@stale stale bot added the status: stale label Jun 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: enhancement Meta-information for changelog generation module: cmake needs: more work Specify why not closed/merged yet status: stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants