Improve CMake module file by myd7349 · Pull Request #312 · KjellKod/g3log

myd7349 · 2019-04-06T04:32:24Z

I am trying to add g3log to vcpkg: microsoft/vcpkg#5961 these days, and encountered some issues:

Lack of install target on Win32
The g3loggerConfig.cmake doesn't work out on Win32
The g3loggerConfig.cmake only takes care of SHARED library

CMakeLists.txt

myd7349 · 2019-04-08T09:51:15Z

Hi! @KjellKod Please don't merge yet. I just want to make sure that CPack works well after this PR.

KjellKod · 2019-04-09T16:05:38Z

To further improve this change and to make it more robust please consider:

Add a build section on the readme for this change
Please consider extending the content of the AppVeyor script so the installation is tested also in the Cloud CI - ref: https://github.com/KjellKod/g3log/blob/master/appveyor.yml

KjellKod · 2019-04-17T21:21:16Z

@myd7349 : any progress on this one?

myd7349 · 2019-04-18T00:06:29Z

Hi! @KjellKod Sorry for the delay! I will finish it as soon as possible.

myd7349 · 2019-04-18T09:51:49Z

I have done some tests about CMAKE_INSTALL_PREFIX and CPACK_PACKAGING_INSTALL_PREFIX. And the result is here: https://github.com/myd7349/Ongoing-Study/blob/master/cpp/CMake/libfoo_v2/README.md

According to the tests:

   IF(NOT CMAKE_INSTALL_PREFIX_INITIALIZED_TO_DEFAULT)
      SET(CPACK_PACKAGING_INSTALL_PREFIX "${CMAKE_INSTALL_PREFIX}")
   ELSEIF(NOT CPACK_PACKAGING_INSTALL_PREFIX)
      SET(CPACK_PACKAGING_INSTALL_PREFIX /usr/local)
   ENDIF()

will cause CPACK_PACKAGING_INSTALL_PREFIX always having the same value as CMAKE_INSTALL_PREFIX on Linux.
(If we don't specify CMAKE_INSTALL_PREFIX, it will has a default value /usr/local according to https://cmake.org/cmake/help/latest/variable/CMAKE_INSTALL_PREFIX.html.)
So, I think:

IF(NOT MINGW)
   INSTALL( TARGETS g3logger
               ARCHIVE DESTINATION ${CPACK_PACKAGING_INSTALL_PREFIX}/${CMAKE_INSTALL_LIBDIR}
               LIBRARY DESTINATION ${CPACK_PACKAGING_INSTALL_PREFIX}/${CMAKE_INSTALL_LIBDIR}
               COMPONENT libraries)

   INSTALL( FILES ${HEADER_FILES}
               DESTINATION ${CPACK_PACKAGING_INSTALL_PREFIX}/${CMAKE_INSTALL_HEADERDIR}
               COMPONENT headers)

   INSTALL( FILES ${PROJECT_SOURCE_DIR}/cmake/g3loggerConfig.cmake
               DESTINATION ${CPACK_PACKAGING_INSTALL_PREFIX}/${CMAKE_INSTALL_LIBDIR}/cmake/g3logger)
ELSE()
   INSTALL( TARGETS g3logger
               ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR}
               LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR}
               COMPONENT libraries)

   INSTALL( FILES ${HEADER_FILES}
               DESTINATION ${CMAKE_INSTALL_HEADERDIR}
               COMPONENT headers)

   INSTALL( FILES ${PROJECT_SOURCE_DIR}/cmake/g3loggerConfig.cmake
               DESTINATION ${CMAKE_INSTALL_LIBDIR}/cmake/g3logger)
ENDIF()

can be simplified as:

   INSTALL( TARGETS g3logger
               ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR}
               LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR}
               COMPONENT libraries)

   INSTALL( FILES ${HEADER_FILES}
               DESTINATION ${CMAKE_INSTALL_HEADERDIR}
               COMPONENT headers)

   INSTALL( FILES ${PROJECT_SOURCE_DIR}/cmake/g3loggerConfig.cmake
               DESTINATION ${CMAKE_INSTALL_LIBDIR}/cmake/g3logger)

The test result also shows that:

cmake .. -DCMAKE_INSTALL_PREFIX=<one value> -DCPACK_PACKAGING_INSTALL_PREFIX=<another value>

will not properly.

To fix this problem, we may change these lines to:

IF(NOT CPACK_PACKAGING_INSTALL_PREFIX)
   IF(NOT CMAKE_INSTALL_PREFIX_INITIALIZED_TO_DEFAULT)
      SET(CPACK_PACKAGING_INSTALL_PREFIX ${CMAKE_INSTALL_PREFIX})
   ELSE()
      SET(CPACK_PACKAGING_INSTALL_PREFIX /usr/local)
   ENDIF()
ENDIF()

KjellKod · 2019-04-18T14:15:16Z

Cool. That sounds like an amazing improvement. Please comment when you are ready for a re-review

KjellKod · 2019-04-18T14:16:55Z

@myd7349 : Please consider adding a section to the readme for the build/install options

KjellKod · 2019-04-18T14:31:26Z

@myd7349 your example [link] seem to be using CPACK_PACKAGING_PREFIX not CPACK_PACKAGING_INSTALL_PREFIX like your comment above suggests.

CPACK_PACKAGING_PREFIX seems to be lacking useable documentation AFAIK. It also doesn't seem to have an effect if it was supposed to work like: CPACK_PACKAGING_INSTALL_PREFIX

myd7349 · 2019-04-19T10:43:43Z

Oh! Sorry. That's a shame. I have tested it again: https://github.com/myd7349/Ongoing-Study/blob/master/cpp/CMake/libfoo_v2/README.md
I also gave the lastest commit a test: https://github.com/myd7349/Ongoing-Study/blob/master/cpp/CMake/libfoo_v2/README.md#test-b30cd9360990fd439682e3543705aab7ea2d49fc

@myd7349 : Please consider adding a section to the readme for the build/install options

Sure.

myd7349 · 2019-04-20T10:16:37Z

Hi! @KjellKod As of the last commit, I think I have finished this PR. Could you give it a review again?

KjellKod

Amazing work @myd7349
Great attention to details and a huge community boon with the improved build system!
I haven’t redone the testing yet but overall it looks great.

I left some minor comments to address

KjellKod · 2019-04-20T16:14:18Z

README.markdown

+You may also specify one or more of those options listed above from the command line.
+For example, on Windows:
+```
+cmake .. -G "Visual Studio 15 2017" -DG3_SHARED_LIB=OFF -DCMAKE_INSTALL_PREFIX=C:/g3log -DADD_G3LOG_UNIT_TEST=ON -DADD_FATAL_EXAMPLE=OFF


nit: please line break

KjellKod · 2019-04-20T16:25:17Z

README.markdown

+```
+This will install g3log to `/usr` instead of `/usr/local`.

+Package maintainers may be interested in the `CPACK_PACKAGING_INSTALL_PREFIX`. For example:


nit: Make it clear that this does not work on Windows.
Ref: your testing that shows that specifying CPACK_PACKAGING_INSTALL_PREFIX fails with cpack.

Suggestion:
Package maintainers
-> *nix* package maintainers or
-> Linux/OSX package maintainers

KjellKod · 2019-04-20T16:29:03Z

README.markdown

+## Configuring
+g3log provides following CMake options (and default values):
+```
+$ cmake -LAH


Super! Didn’t know you could trigger a listing of all the variables.

nit: explain above the command in a one liner what cmake -LAH is

It doesn’t hurt to mention Options.cmake

Suggestion: something like

List all of g3log’s cmake options to see configuration choices. For additional option context and comments please also see [Options.cmake](https://github.com/KjellKod/g3log/blob/master/Options.cmake) *Generate listing of all of g3log’s CMake variables:* `cmake-LAH`

One more commit is added. And the second sentence is added after those options.

KjellKod

Detailed. To the point and overall a Great improvement.
Approved, pending testing before merging

KjellKod · 2019-05-02T02:53:20Z

@myd7349
Can I re-review, test and hopefully merge this one or do you still have work in progress that I should wait on?

Thanks
Kjell

myd7349 · 2019-05-02T05:27:00Z

Yes. I think this PR is more better after the last two commits. Please review it again. :)

KjellKod · 2019-05-13T19:18:35Z

Thanks. I have not forgotten about this but work-life balance has been off lately.
I will attend to this tonight!

KjellKod

Can you verify if there is an : OSX issue on the generated target file.

scripts/buildAndRunTests.sh

KjellKod · 2019-05-14T04:42:25Z

Nice work @myd7349! thanks

myd7349 added 4 commits April 6, 2019 12:25

Improve CMake module file

f2e9413

Fix include directories of tests

0c90f6f

Append dependencies to G3LOG_LIBRARY

967d996

Fix include directories

b9febab

myd7349 marked this pull request as ready for review April 6, 2019 11:11

KjellKod requested changes Apr 7, 2019

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

KjellKod mentioned this pull request Apr 7, 2019

Updated CMakeLists.txt to enable installing on Windows #313

Closed

myd7349 added 2 commits April 8, 2019 08:06

Remove unnecessary system check

1fd01f1

Update comments

6a9b443

myd7349 added 2 commits April 18, 2019 18:18

Fix packaging on Win32

a72961d

Use another way to specify interface include directories

b30cd93

myd7349 added 3 commits April 20, 2019 13:49

appveyor: Test install and package target

8a5462a

Travis-CI: Test install and package target

9ac37ec

Update documentation for working with CMake

56832ff

KjellKod requested changes Apr 20, 2019

View reviewed changes

Improve documentation

89ae2f8

myd7349 force-pushed the win32-cmake-install branch from 1f96aac to 89ae2f8 Compare April 21, 2019 06:41

Improve documentation

f80d6de

KjellKod previously approved these changes Apr 21, 2019

View reviewed changes

Fix indention and minor simplification

4f6101b

myd7349 dismissed KjellKod’s stale review via 4f6101b April 30, 2019 10:02

Simplify g3loggerConfig.cmake

fd041d5

myd7349 added 2 commits May 12, 2019 12:58

Merge branch 'master' into win32-cmake-install

273550d

Add Prerequisites section

19a55ed

KjellKod reviewed May 14, 2019

View reviewed changes

scripts/buildAndRunTests.sh Show resolved Hide resolved

KjellKod approved these changes May 14, 2019

View reviewed changes

KjellKod merged commit 376c417 into KjellKod:master May 14, 2019

myd7349 deleted the win32-cmake-install branch May 14, 2019 10:11

This was referenced Jul 28, 2019

Fix CMake interface include directories #321

Merged

Cmake include paths adds './COMPONENT' and './libraries' #319

Closed

Uh oh!

Conversation

myd7349 commented Apr 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

myd7349 commented Apr 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KjellKod commented Apr 9, 2019

Uh oh!

KjellKod commented Apr 17, 2019

Uh oh!

myd7349 commented Apr 18, 2019

Uh oh!

myd7349 commented Apr 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KjellKod commented Apr 18, 2019

Uh oh!

KjellKod commented Apr 18, 2019

Uh oh!

KjellKod commented Apr 18, 2019

Uh oh!

myd7349 commented Apr 19, 2019

Uh oh!

myd7349 commented Apr 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KjellKod left a comment

Choose a reason for hiding this comment

Uh oh!

KjellKod Apr 20, 2019

Choose a reason for hiding this comment

Uh oh!

myd7349 Apr 21, 2019

Choose a reason for hiding this comment

Uh oh!

KjellKod Apr 20, 2019

Choose a reason for hiding this comment

Uh oh!

myd7349 Apr 21, 2019

Choose a reason for hiding this comment

Uh oh!

KjellKod Apr 20, 2019

Choose a reason for hiding this comment

Uh oh!

KjellKod Apr 20, 2019

Choose a reason for hiding this comment

Uh oh!

myd7349 Apr 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KjellKod left a comment

Choose a reason for hiding this comment

Uh oh!

KjellKod commented May 2, 2019

Uh oh!

myd7349 commented May 2, 2019

Uh oh!

KjellKod commented May 13, 2019

Uh oh!

KjellKod left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

KjellKod commented May 14, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

myd7349 commented Apr 6, 2019 •

edited

Loading

myd7349 commented Apr 8, 2019 •

edited

Loading

myd7349 commented Apr 18, 2019 •

edited

Loading

myd7349 commented Apr 20, 2019 •

edited

Loading

myd7349 Apr 21, 2019 •

edited

Loading