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 compatibility with find package #181

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

andre-nguyen
Copy link

@andre-nguyen andre-nguyen commented Jun 15, 2019

This preserves behaviour with the add_subdirectory stuff.


This change is Reviewable

@jarro2783
Copy link
Owner

Can you explain the problem that you're fixing here?

@andre-nguyen
Copy link
Author

andre-nguyen commented Jun 17, 2019

Oh sorry about the lack of details. This PR fixes 2 problems and adds 1 feature.

  1. Problem: the added export(PACKAGE ...) line now actually exports cxxopts to the user package registry (~/.cmake/packages/ on linux) that way it is find_package-able by other projects.
  2. Problem: With a project structure such as:
|-- Project
     -- CmakeLists.txt
            add_subdirectory(cxxopts)
            add_subdirectory(liba)
            add_subdirectory(libb)
    |-- liba
        -- CmakeLists.txt (find_package(cxxopts REQUIRED)
    |-- libb
        -- CmakeLists.txt (find_package(cxxopts REQUIRED)
    |-- cxxopts
        -- CmakeLists.txt

Where two parts of a project can depend on cxxopts and cxxopts is embedded in the library. With the current master commit I couldn't get my libraries to find_package cxxopts. Now it works and it preserves compatibility with the "regular" approach where cxxopts is a subdirectory of the main project rather than a sister directory.

  1. New feature: With the project prefix based on the project version, multiple versions of the library can be installed.

@jarro2783
Copy link
Owner

I don't know how all of this works because I'm not very familiar with cmake, but something doesn't look right here:

jarryd@guepardo ~/current/soft/build/cxxopts $ cmake . -DCMAKE_INSTALL_PREFIX=$HOME/local
-- cxxopts version 2.2.0
-- Configuring done
-- Generating done
-- Build files have been written to: /home/jarryd/current/soft/build/cxxopts
jarryd@guepardo ~/current/soft/build/cxxopts $ ninja install
[0/1] Install the project...
-- Install configuration: "Debug"
-- Installing: /cxxopts-2.2.0/cmake/cxxopts-targets.cmake
-- Installing: /home/jarryd/local/lib/cmake/cxxopts/cxxopts-config.cmake
-- Installing: /home/jarryd/local/lib/cmake/cxxopts/cxxopts-config-version.cmake
-- Installing: /home/jarryd/local/lib/cmake/cxxopts/cxxopts-targets.cmake
-- Installing: /cxxopts-2.2.0/cxxopts/cxxopts.hpp

@andre-nguyen
Copy link
Author

Just to confirm, do you mean these two lines?
-- Installing: /cxxopts-2.2.0/cmake/cxxopts-targets.cmake
-- Installing: /cxxopts-2.2.0/cxxopts/cxxopts.hpp
Indeed that path looks incomplete.

@jarro2783
Copy link
Owner

Yes. I don't expect something to be installed in the root directory when giving a prefix in my home directory. In general an arbitrary user probably can't do that anyway.

@andre-nguyen andre-nguyen force-pushed the feature/find_package_compatibility branch from e624de7 to 80bdf93 Compare June 22, 2019 04:42
@andre-nguyen
Copy link
Author

I had forgotten the line

include(GNUInstallDirs)

Which defines the CMAKE_INSTALL_LIBDIR variable. It works on my side now, can you confirm?

andre@neptune:~/Documents/personal/cxxopts/build$ cmake .. -DCMAKE_INSTALL_PREFIX=${PWD}/install
-- cxxopts version 2.2.0
-- Configuring done
-- Generating done
-- Build files have been written to: /home/andre/Documents/personal/cxxopts/build
andre@neptune:~/Documents/personal/cxxopts/build$ make install
[ 25%] Built target example
[ 62%] Built target link_test
[100%] Built target options_test
Install the project...
-- Install configuration: ""
-- Installing: /home/andre/Documents/personal/cxxopts/build/install/lib/cxxopts-2.2.0/cmake/cxxopts-targets.cmake
-- Installing: /home/andre/Documents/personal/cxxopts/build/install/lib/cmake/cxxopts/cxxopts-config.cmake
-- Installing: /home/andre/Documents/personal/cxxopts/build/install/lib/cmake/cxxopts/cxxopts-config-version.cmake
-- Installing: /home/andre/Documents/personal/cxxopts/build/install/lib/cmake/cxxopts/cxxopts-targets.cmake
-- Installing: /home/andre/Documents/personal/cxxopts/build/install/include/cxxopts-2.2.0/cxxopts/cxxopts.hpp

dm3 added a commit to dm3/cxxopts that referenced this pull request Oct 9, 2019
dm3 added a commit to dm3/cxxopts that referenced this pull request Oct 9, 2019
@andre-nguyen
Copy link
Author

@jarro2783 Anything I can do to help this get merged in?

@jarro2783
Copy link
Owner

Sorry I forgot about this one. Can you rebase first?

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.

None yet

2 participants