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

Fix installation path of CMake package files #460

Closed
wants to merge 27 commits into from

Conversation

shaxbee
Copy link
Contributor

@shaxbee shaxbee commented Mar 8, 2015

Installation path prevented cmake from finding GLFW. It seems that INSTALL_DIRECTORY was correct but path provided to INSTALL command was not updated.
Added GLFW_LIBRARIES export to glfwConfig.cmake.in as well.

Now it is possible to use glfw with externalproject_add and find_package as follows:

find_package(glfw3 REQUIRED)
add_executable(foo foo.cpp)
target_link_libraries(foo ${GLFW3_LIBRARY})

@elmindreda elmindreda self-assigned this Mar 8, 2015
@elmindreda elmindreda added the bug Bug reports and bugfix pull requests label Mar 8, 2015
@@ -567,7 +567,7 @@ if (GLFW_INSTALL)

install(FILES "${GLFW_BINARY_DIR}/src/glfw3Config.cmake"
"${GLFW_BINARY_DIR}/src/glfw3ConfigVersion.cmake"
DESTINATION lib${LIB_SUFFIX}/cmake/glfw)
DESTINATION ${GLFW_CONFIG_PATH})

install(EXPORT glfwTargets DESTINATION lib${LIB_SUFFIX}/cmake/glfw)
Copy link
Contributor

Choose a reason for hiding this comment

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

It may be appropriate to change glfwTargets DESTINATION to ${GLFW_CONFIG_PATH} as well.

@shaxbee
Copy link
Contributor Author

shaxbee commented Mar 9, 2015

Fixed the glfwTargets
Where should I get the INCLUDE_DIRS / LIBRARY_DIRS from?

@nussetorten
Copy link
Contributor

I would set them to ${GLFW3_INCLUDE_DIR} and ${GLFW3_LIBRARY_DIR}, respectively, to maintain SSOT.

But before implementing this change, remind me of the rational of defining two sets of _LIBRARY, _INCLUDE_DIR and _LIBRARY_DIR with GLFW and GLFW3 prefixes? Is this to maintain some backwards compatibility?

@shaxbee
Copy link
Contributor Author

shaxbee commented Mar 9, 2015

No, i just wanted to keep it uniform with package name. What is the reason config prefix is glfw3 not simply glfw?

@nussetorten
Copy link
Contributor

I believe this was done intentionally, as not to cause confusion with installations of the now-legacy glfw2.x.

@nussetorten
Copy link
Contributor

A quick build of your master branch yields the following:

C:\Users\nvitovitch\Projects\glfw\build\glfw-shaxbee>cmake -G"Visual Studio 10 Win64" -DCMAKE_PREFIX_PATH=C:\Users\nvitovitch\Projects\glfw\pkg-shaxbee -DCMAKE_INSTALL_PREFIX=C:\Users\nvitovitch\Projects\glfw\pkg-shaxbee ..\..\code\glfw-shaxbee
-- The C compiler identification is MSVC 16.0.40219.1
-- Check for working C compiler using: Visual Studio 10 Win64
-- Check for working C compiler using: Visual Studio 10 Win64 -- works
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Found OpenGL: opengl32
-- Looking for include file pthread.h
-- Looking for include file pthread.h - not found
-- Found Threads: TRUE
-- Could NOT find Doxygen (missing:  DOXYGEN_EXECUTABLE)
-- Using Win32 for window creation
-- Using WGL for context creation
-- Configuring done
CMake Error: install(EXPORT "glfwTargets") given absolute DESTINATION "C:/Users/nvitovitch/Projects/glfw/pkg-shaxbee" but the export references an installation of target "glfw" which has relative DESTINATION "lib".
CMake Error: install(EXPORT "glfwTargets") given absolute DESTINATION "C:/Users/nvitovitch/Projects/glfw/pkg-shaxbee" but the export references an installation of target "glfw" which has relative DESTINATION "lib".
CMake Error: install(EXPORT "glfwTargets") given absolute DESTINATION "C:/Users/nvitovitch/Projects/glfw/pkg-shaxbee" but the export references an installation of target "glfw" which has relative DESTINATION "lib".
CMake Error: install(EXPORT "glfwTargets") given absolute DESTINATION "C:/Users/nvitovitch/Projects/glfw/pkg-shaxbee" but the export references an installation of target "glfw" which has relative DESTINATION "lib".
-- Generating done
-- Build files have been written to: C:/Users/nvitovitch/Projects/glfw/build/glfw-shaxbee

I think the correct fix is to change GLFW_CONFIG_PATH to a relative path, namely set(GLFW_CONFIG_PATH "lib${LIB_SUFFIX}/cmake/glfw3").

To address the original intent of your pull request, linking external projects with ease, target_link_libraries(glfw LINK_PUBLIC ${glfw_LIBRARIES}) might exhibit the desired behaviour. I'm going to run some tests and get back to you :)

@nussetorten
Copy link
Contributor

I spent some significant time reviewing cmake, cmake targets, etc. and I have a solution. I will try an submit a pull request to your master branch by 5EST! Afterwards we should do some prelim builds and tests, just to be sure everything is working.

@shaxbee
Copy link
Contributor Author

shaxbee commented Mar 11, 2015

Merged @ngvitovitch changes into pull request - they add link libraries as transitive dependency, I've added include directory as well. This change removes need to set include_directories and target_link_libraries (for transitive dependencies).

Example:
find_package(glfw3 REQUIRED)
add_executable(foo foo.cpp)
target_link_libraries(foo glfw)

@shaxbee
Copy link
Contributor Author

shaxbee commented Mar 11, 2015

Tested on Mac OS X.

@nussetorten
Copy link
Contributor

Will test on Windows, Linux at my earliest convenience. Could you confirm that the generated glfw3Targets* files contain no absolute paths? I am mildly suspicious of the latest target_include_dirs().

@shaxbee
Copy link
Contributor Author

shaxbee commented Mar 11, 2015

It does contain absolute path because relative path nor source/binary
prefixed path is not allowed in there:

  Target "glfw" INTERFACE_INCLUDE_DIRECTORIES property contains path:

    "/Users/shaxbee/hasiok/glfw-cmake-package/src/../include"

  which is prefixed in the source directory.

On Wed, 11 Mar 2015 at 22:22 Nicholas Gregory Vitovitch <
notifications@github.com> wrote:

Will test on Windows, Linux at my earliest convenience. Could you confirm
that the generated glfw3Targets* files contain no absolute paths? I am
mildly suspicious of the latest target_include_dirs().


Reply to this email directly or view it on GitHub
#460 (comment).

@nussetorten
Copy link
Contributor

Not true, the absolute path is not necessary and moreover it breaks portability of the package. Example:

  1. Install glfw to <pkg_dir>,
  2. Relocate <pkg_dir>
  3. Configure and build <my_app> using CMake - building fails due to missing headers b/c the path baked into glfw3Targets.cmake is now invalid.

Check out this example using CMake generator expressions: http://www.cmake.org/cmake/help/v3.0/command/target_include_directories.html

Also, target_include_directories was introduced in CMake 2.8.11 and will cause an error when building with glfw's minimum required version of CMake (2.8.9). If this command is included, it needs to be if-guarded for older versions of CMake.

@shaxbee
Copy link
Contributor Author

shaxbee commented Mar 12, 2015

@elmindreda @ngvitovitch: BTW. Since C++11 is already enforced, and Ubuntu 14.04 LTS comes with CMake 2.8.12, maybe it would worth to bump cmake version to provide uniform build.

@nussetorten
Copy link
Contributor

@elmindreda requesting feedback on PR

@elmindreda elmindreda added this to the 3.2 milestone Mar 15, 2015
@elmindreda
Copy link
Member

Will do after 3.1.1. Only one issue remaining.

@nussetorten
Copy link
Contributor

@elmindreda huzzah for the 3.1.1 release!

@elmindreda
Copy link
Member

Please make your commit messages well formed. Subject line max 50 characters, remaining lines max 72 characters, empty lines between paragraphs.

@shaxbee
Copy link
Contributor Author

shaxbee commented Mar 29, 2015

@elmindreda Reformatted commit messages

@shaxbee
Copy link
Contributor Author

shaxbee commented Apr 6, 2015

@elmindreda PR ready for merge

@elmindreda
Copy link
Member

@shaxbee I know. I'm just catching up.

@shaxbee
Copy link
Contributor Author

shaxbee commented Oct 4, 2015

Rebased, added Travis CI to track CMake <3.0 compatibility

@shaxbee
Copy link
Contributor Author

shaxbee commented Oct 4, 2015

@elmindreda Any ETA on merging this?

@shaxbee shaxbee force-pushed the master branch 3 times, most recently from 9fdee6e to 66d3aab Compare October 4, 2015 22:04
@elmindreda
Copy link
Member

I've picked out bits I wanted from this patch set. There's a lot of good stuff here, but also some bugs, things I won't be merging and things I'd already fixed while learning modern CMake so I could tackle this issue.

I've tried to retain proper credits for all the pieces, but not at the expense of clear patches without backtracking. Thank you so much, both of you, and sorry for taking so long to get to this.

@elmindreda elmindreda closed this Feb 2, 2016
elmindreda pushed a commit that referenced this pull request Feb 2, 2016
@elmindreda
Copy link
Member

Also merged bits of the Travis and Appveyor CI files. Thanks!

@elmindreda elmindreda added the enhancement Feature suggestions and PRs label Feb 4, 2016
@elmindreda elmindreda added the build Build file bugs and PRs (not compilation errors) label Oct 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug reports and bugfix pull requests build Build file bugs and PRs (not compilation errors) enhancement Feature suggestions and PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants