Skip to content
This repository has been archived by the owner on Feb 21, 2019. It is now read-only.

cmake: Conditionally add log and log_setup to exported configuration #51

Merged
merged 2 commits into from
Oct 10, 2017
Merged

cmake: Conditionally add log and log_setup to exported configuration #51

merged 2 commits into from
Oct 10, 2017

Conversation

rleigh-codelibre
Copy link
Contributor

These were missing from the exported configuration. Partly an oversight, and partly because they were conditionally used. This adds them conditionally so should work in all cases.

Testing: Check all builds remain green. Check OMECommonConfig.cmake and ensure that it contains:

find_dependency(Boost 1.46 REQUIRED
                COMPONENTS date_time filesystem system iostreams
                program_options regex log_setup log)

(in any build; CentOS7 platform builds will have it missing because Boost.Log is not available, but we don't have this in the CI builds yet.)

This is added implicitly by find_dependency.
@sbesson
Copy link
Member

sbesson commented Oct 3, 2017

@rgozim did you have a chance to review this PR and its companion ones? Do these need to be relisted for review?

@rgozim
Copy link
Member

rgozim commented Oct 5, 2017

Everything looks okay. CI box build looks to working too, except for a small error due to a missing header file in OME-FILES-CPP-DEV-merge-sourcebuild.

COMPONENTS date_time filesystem system iostreams
program_options regex)
find_dependency(XercesC 3.0.0 REQUIRED)
Copy link
Member

Choose a reason for hiding this comment

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

Is the drop of the REQUIRED qualifier required for the conditional import to work or is this related to the find_dependency limitations discussed in ome/ome-files-cpp#85 (comment)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's unrelated to both. It's always been incorrect to use REQUIRED with find_dependency, but I didn't understand this at the time these files were written. find_dependency adds REQUIRED for you if the top-level find_package call used REQUIRED otherwise it's not added and left optional. It will work without the change, but the behaviour will be wrong.

@sbesson sbesson merged commit 7c133c4 into ome:master Oct 10, 2017
@rleigh-codelibre rleigh-codelibre deleted the exported-config-fixes branch October 10, 2017 07:39
@sbesson sbesson added this to the 5.5.0 milestone Nov 18, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants