[cmake] adding a target install #182

matlo607 · 2018-02-13T13:27:17Z

I propose to add a target install to CMakeLists.txt

Types of changes

Bug fix (non-breaking change which fixes an issue).
New feature (non-breaking change which adds functionality).
Breaking change (fix or feature that would cause existing functionality to not work as expected).

Checklist:

It is my own work, its copyright is implicitly assigned to the project and no substantial part of it has been copied from other sources (including Stack Overflow). In rare occasions this is acceptable, like in CMake modules where the original copyright information should be kept.
I'm using the same code standards as the existing code (indentation, spacing, variable naming, ...).
I've added tests for my code.
I have verified whether my change requires changes to the documentation
My change either requires no documentation change or I've updated the documentation accordingly.
My branch has been rebased to master, keeping only relevant commits.

matlo607 · 2018-02-14T21:55:24Z

@paoloambrosio, @muggenhor or @konserw : could you please review the pull request ?
Please ignore the commit 3887d2c, konserw told me it's wrong. I will wait for #184.

coveralls · 2018-03-21T18:05:49Z

Coverage remained the same at 62.814% when pulling edea427 on matlo607:cmake-add-target-install into 870bfc2 on cucumber:master.

konserw · 2018-03-30T19:17:15Z

@matlo607 could you please rebase this branch onto master now that it is fixed? Also please drop commmit
8076700

matlo607 · 2018-04-03T07:55:56Z

Hello @konserw,
I rebased the branch and removed 8076700

muggenhor · 2018-05-17T11:41:21Z

CMakeLists.txt

@@ -188,9 +188,13 @@ endif()
 #
 # Cucumber-Cpp
 #
+set (CMAKE_INSTALL_INCLUDEDIR "${CMAKE_INSTALL_PREFIX}")
+set (CMAKE_INSTALL_LIBDIR "${CMAKE_INSTALL_PREFIX}/lib")
+set (CMAKE_INSTALL_BINDIR "${CMAKE_INSTALL_PREFIX}/bin")


I think include(GNUInstallDirs) should already do the right thing here.

muggenhor · 2018-05-18T08:47:44Z

src/CMakeLists.txt

+target_include_directories(cucumber-cpp-nomain PUBLIC
+    $<BUILD_INTERFACE:${CUKE_INCLUDE_DIR}>
+    $<INSTALL_INTERFACE:include>
+    PRIVATE $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR})


The current source directory isn't necessary as include path so don't add it. Same below

muggenhor · 2018-05-18T08:48:22Z

src/CMakeLists.txt

@@ -45,9 +45,27 @@ endif()
 add_library(cucumber-cpp-nomain STATIC ${CUKE_SOURCES})
 add_library(cucumber-cpp STATIC ${CUKE_SOURCES} main.cpp)

+target_include_directories(cucumber-cpp-nomain PUBLIC
+    $<BUILD_INTERFACE:${CUKE_INCLUDE_DIR}>
+    $<INSTALL_INTERFACE:include>


This should be $<INSTALL_INTERFACE:$<INSTALL_PREFIX>/include>. Same below

muggenhor · 2018-05-18T08:49:37Z

src/CMakeLists.txt

@@ -45,9 +45,27 @@ endif()
 add_library(cucumber-cpp-nomain STATIC ${CUKE_SOURCES})
 add_library(cucumber-cpp STATIC ${CUKE_SOURCES} main.cpp)

+target_include_directories(cucumber-cpp-nomain PUBLIC
+    $<BUILD_INTERFACE:${CUKE_INCLUDE_DIR}>


Is adding this enough to permit us from removing the directory-scoped include_directories(${CUKE_INCLUDE_DIR}) in the toplevel CMakeLists.txt?

muggenhor · 2018-05-18T08:50:39Z

src/CMakeLists.txt

+    ARCHIVE  DESTINATION ${CMAKE_INSTALL_LIBDIR}
+    LIBRARY  DESTINATION ${CMAKE_INSTALL_LIBDIR}
+    RUNTIME  DESTINATION ${CMAKE_INSTALL_BINDIR})
+install(TARGETS cucumber-cpp


Why the duplication? The install() command supports multiple targets after TARGETS.

muggenhor · 2018-05-18T08:53:08Z

src/CMakeLists.txt

+install(TARGETS cucumber-cpp
+    ARCHIVE  DESTINATION ${CMAKE_INSTALL_LIBDIR}
+    LIBRARY  DESTINATION ${CMAKE_INSTALL_LIBDIR}
+    RUNTIME  DESTINATION ${CMAKE_INSTALL_BINDIR})


For usability I'm expecting an export() of the library targets too.

To support find_package(CucumberCpp) install(EXPORT). And use GNUInstallDirs instead of hardcoded installation paths.

This makes installing of the built libraries along with their headers possible through the CMake-standard 'install' target. It responds to the usual DESTDIR and CMAKE_INSTALL_PREFIX variables to control installation location.

aslakhellesoy · 2018-05-31T16:47:17Z

Hi @matlo607,

Thanks for your making your first contribution to Cucumber, and welcome to the Cucumber committers team! You can now push directly to this repo and all other repos under the cucumber organization! 🍾

In return for this generous offer we hope you will:

✅ Continue to use branches and pull requests. When someone on the core team approves a pull request (yours or someone else's), you're welcome to merge it yourself.
💚 Commit to setting a good example by following and upholding our code of conduct in your interactions with other collaborators and users.
💬 Join the community Slack channel to meet the rest of the team and make yourself at home.
ℹ️ Don't feel obliged to help, just do what you can if you have the time and the energy.
🙋 Ask if you need anything. We're looking for feedback about how to make the project more welcoming, so please tell us!

On behalf of the Cucumber core team,
Aslak Hellesøy
Creator of Cucumber

matlo607 mentioned this pull request Feb 14, 2018

fix OSX build failing on warning #184

Closed

9 tasks

matlo607 force-pushed the cmake-add-target-install branch from 3887d2c to 8076700 Compare March 21, 2018 17:23

matlo607 force-pushed the cmake-add-target-install branch from 8076700 to 116615b Compare March 30, 2018 20:01

muggenhor requested changes May 18, 2018

View reviewed changes

matlo607 and others added 3 commits May 23, 2018 11:12

[cmake] adding a target install

ec2cdcf

Install exports and use GNUInstallDirs

2332cec

To support find_package(CucumberCpp) install(EXPORT). And use GNUInstallDirs instead of hardcoded installation paths.

Test installation on CI

edea427

muggenhor added this to the v0.5 milestone May 23, 2018

muggenhor force-pushed the cmake-add-target-install branch from 116615b to edea427 Compare May 31, 2018 08:42

muggenhor approved these changes May 31, 2018

View reviewed changes

muggenhor merged commit edea427 into cucumber:master May 31, 2018

matlo607 deleted the cmake-add-target-install branch July 16, 2018 10:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[cmake] adding a target install #182

[cmake] adding a target install #182

matlo607 commented Feb 13, 2018 •

edited by muggenhor

Loading

matlo607 commented Feb 14, 2018

coveralls commented Mar 21, 2018 •

edited

Loading

konserw commented Mar 30, 2018

matlo607 commented Apr 3, 2018

muggenhor May 17, 2018

muggenhor May 18, 2018

muggenhor May 18, 2018

muggenhor May 18, 2018

muggenhor May 18, 2018

muggenhor May 18, 2018

aslakhellesoy commented May 31, 2018

[cmake] adding a target install #182

[cmake] adding a target install #182

Conversation

matlo607 commented Feb 13, 2018 • edited by muggenhor Loading

Types of changes

Checklist:

matlo607 commented Feb 14, 2018

coveralls commented Mar 21, 2018 • edited Loading

konserw commented Mar 30, 2018

matlo607 commented Apr 3, 2018

muggenhor May 17, 2018

Choose a reason for hiding this comment

muggenhor May 18, 2018

Choose a reason for hiding this comment

muggenhor May 18, 2018

Choose a reason for hiding this comment

muggenhor May 18, 2018

Choose a reason for hiding this comment

muggenhor May 18, 2018

Choose a reason for hiding this comment

muggenhor May 18, 2018

Choose a reason for hiding this comment

aslakhellesoy commented May 31, 2018

matlo607 commented Feb 13, 2018 •

edited by muggenhor

Loading

coveralls commented Mar 21, 2018 •

edited

Loading