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

Already on GitHub? Sign in to your account

Update cmake configuration to allow for use of system libraries. #253

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants
Contributor

hobbes1069 commented Apr 5, 2012

Here's my patch to allow use of system libraries. I think this should be in pretty good shape but as usual let me know if you're concerned about any of it.

Is there any particular reason to hide the options on other platforms? I'm pretty sure you could still -D USE_EXTERNAL_YAML=yes on other operating systems, just things like the CMake GUI's wont show them

Utterly pedantic nit-pick: I find repeating the condition in else(THING_FOUND) unnecessary/confusing (reads a bit like an else if)

Even more pedantic, even more nittier-pick - the "System yaml-cpp" and the likes should probably be worded "External yaml-cpp"

Contributor

dbr commented Apr 7, 2012

Diff looks good - will test tomorrow!

Contributor

hobbes1069 commented Apr 7, 2012

The only reason for hiding the options on other systems is I wasn't sure if my package finding logic was safe for other platforms. If someone can test on Apple and Windows that would be great since I only run linux. Feel free to tweak things. I got into the habit of keeping the conditionals in the else and endif because I often ran into multiple nested conditionals and it can get confusing which one you're in sometimes.

I'm not picky on the wording either... I just added these to make packaging on Fedora easier since we don't allow bundled libraries.

Contributor

dbr commented Apr 8, 2012

USE_EXTERNAL_LCMS isn't used anywhere. I think the logic in src/apps/ociobakelut/CMakeLists.txt needs reworked a bit, so it just does:

if(USE_EXTERNAL_LCMS)
    pkg_check_modules(LCMS lcms2 REQUIRED) #along with the version checks and such
else()
    ExternalProject_Add(LCMS ...) # and so on
endif()

...much the same as your changes for tinyxml/yaml.

wasn't sure if my package finding logic was safe for other platforms

I think it should be fine - it depends on the pkgconfig files being correctly written at install-time, which is a reasonable thing to rely on. Plus people will only have problems if they've intentionally added the USE_EXTERNAL flag

Contributor

hobbes1069 commented Apr 8, 2012

I've been working on this on and off for so long I'm starting to forget where I left things! We can kill the external LCMS option since ociobakelut will use it if found. I was thinking I still needed something to error out if it wasn't found because it will fall back to using bundled lcms but as I'm removing the lcms archive in %prep it will error out anyway.

Collaborator

jeremyselan commented Apr 9, 2012

This looks good, but I'm going to defer committing it until we've bumped the master up to yaml-cpp 0.3.0. (This bumps to 0.3, and we're not quite ready to go on that front).

I'll do my best to get the 0.3.0 issues sorted out and committed this week.

Collaborator

jeremyselan commented Apr 10, 2012

Committed in bdc4327, after bumping to 0.3.0

Could you both test and confirm building with USE_EXTERNAL_TINYXML and USE_EXTERNAL_YAML still works?

Collaborator

jeremyselan commented Apr 10, 2012

Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment