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

Use more GNUInstallDirs #31

Merged
merged 1 commit into from
Aug 21, 2017
Merged

Use more GNUInstallDirs #31

merged 1 commit into from
Aug 21, 2017

Conversation

a17r
Copy link
Contributor

@a17r a17r commented Aug 20, 2017

Conveniently fixes multiarch install dir issues.

@clanmills clanmills requested a review from piponazo August 20, 2017 20:39
@clanmills
Copy link
Collaborator

Andreas. Thanks for this contribution. Luis (piponazo) is our CMake expert and will review your changes.

@piponazo
Copy link
Collaborator

piponazo commented Aug 20, 2017

Hi @a17r , thanks a lot for the contribution.

I did not know about the GNUInstallDirs module. I actually think that I might have removed something about it during the CMake refactoring it is taking place during the last weeks.

Could you please explain briefly what is your user case and why this is needed ?

I have checked this and it works perfectly. However I left a comment about the usage of CMAKE_SOURCE_INCLUDEDIR. On Windows, the value of that variable is empty, and I could not find any reference to it on the web. Might it be a typo ?

Conveniently fixes multiarch install dir issues.
@a17r
Copy link
Contributor Author

a17r commented Aug 20, 2017

Thanks for catching that one, fixed the bogus var.

Basically, everyone should use GNUInstallDirs with cmake - it is giving us sane defaults to work with, less re-invented wheels, and easy to override vars for different distro needs (Gentoo has libx32, lib32, lib64, other distributions may have more complicated multiarch names). With exiv2 as-is, a 32/64 bit multiarch build would simply install twice into /usr/lib/libexiv2.so, raising an error with strict checks. And instead of chasing libdir paths we now even get automatically correct /usr/lib{32,64}/pkgconfig files.

Copy link
Collaborator

@piponazo piponazo left a comment

Choose a reason for hiding this comment

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

The comment I made has been addressed. Now the PR only make changes related to the GNUInstallDirs


# TODO This should not be needed here! we need to fix the previous TODO
target_include_directories(exiv2 PRIVATE ${CMAKE_SOURCE_DIR}/include/)
target_include_directories(exiv2 PRIVATE ${CMAKE_SOURCE_INCLUDEDIR})
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this might be a typo. I have been searching for this CMAKE_SOURCE_INCLUDEDIR and I did not found anything about it. Could you give any reference to its usage or double check if it is a typo ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I actually checked again and the lines 292 and 293 can be removed. The exiv2 target is propagating already the needed include directories.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants