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

Prevent search for disabled optional dependencies in targets. #2229

Merged
merged 1 commit into from
Feb 26, 2018

Conversation

SergioRAgostinho
Copy link
Member

This covers the first issue mentioned in #1547.

@SergioRAgostinho SergioRAgostinho added the needs: code review Specify why not closed/merged yet label Feb 25, 2018
@SergioRAgostinho SergioRAgostinho added this to the pcl-1.9.0 milestone Feb 25, 2018
Copy link
Member

@taketwo taketwo left a comment

Choose a reason for hiding this comment

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

LGTM

string(REGEX REPLACE "-(.*)" "" _condition ${_tmp}) #libusb-1.0 case
if(${${_condition}})
set(PCLCONFIG_OPTIONAL_DEPENDENCIES "${PCLCONFIG_OPTIONAL_DEPENDENCIES}${_opt_dep} ")
endif(${${_condition}})
Copy link
Member

Choose a reason for hiding this comment

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

It is not necessary to repeat condition expression in the endif since CMake 2.8.

set(PCLCONFIG_OPTIONAL_DEPENDENCIES "${PCLCONFIG_OPTIONAL_DEPENDENCIES}${_opt_dep} ")
string(TOUPPER "WITH_${_opt_dep}" _tmp)
string(REGEX REPLACE "-(.*)" "" _condition ${_tmp}) #libusb-1.0 case
if(${${_condition}})
Copy link
Member

Choose a reason for hiding this comment

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

It is enough to supply variable name to if. Thus in this case

if(${_condition})

@taketwo
Copy link
Member

taketwo commented Feb 26, 2018

I assume you tried to build/install/find_package after this change?

@SergioRAgostinho
Copy link
Member Author

SergioRAgostinho commented Feb 26, 2018

Yep. I hit the bug on a fresh machine installation. I didn't have Qhull installed and cmake was throwing some problematic warnings/errors regarding Qhull not being found.

@taketwo
Copy link
Member

taketwo commented Feb 26, 2018

Great. Small patch, but extremely useful 💯

@taketwo taketwo removed the needs: code review Specify why not closed/merged yet label Feb 26, 2018
@taketwo taketwo merged commit e157f3f into PointCloudLibrary:master Feb 26, 2018
@SergioRAgostinho SergioRAgostinho deleted the targets-opt-deps branch August 26, 2018 11:54
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.

2 participants