Added packaging with CPack and generation of pkg-config. by KOLANICH · Pull Request #52 · bfgroup/Lyra

KOLANICH · 2021-07-22T14:43:39Z

No description provided.

jbadwaik

In general, it looks like a lot of different changes in a single comment. You are at least:

Adding project description information.
Changing include path for installed version of library.
Trying to make the installation architecture independent.
Adding CPack Scripts
Adding PkgConfig Information

I would personally like to see all those changes made in separate commits. In particular, I am not convinced of wisdom is complicating scripts to make installs architecture independent (part 3 above). While this looks nice here, in my experience, this complicates the life of package managers, who might want to specify their own changes. However, I can be convinced of this, if someone presents a good logic.

In short, I like 1 and 5. I am not well versed enough to evaluate 4, and I do not like 2 and 3 as of now.

jbadwaik · 2021-07-22T15:27:16Z

CMakeLists.txt

+set(PROJECT_HOMEPAGE_URL "https://github.com/bfgroup/Lyra")
+
+include(GNUInstallDirs)
+string(REPLACE "/${CMAKE_LIBRARY_ARCHITECTURE}" "" CMAKE_INSTALL_LIBDIR_ARCHIND "${CMAKE_INSTALL_LIBDIR}")


Why are we using CMAKE_LIBRARY_ARCHITECTURE here?

It is the impl detail of GNUInstallDirs. CMAKE_INSTALL_LIBDIR is composed of it and other stuff, usually lib. I have tried to set it to "" before including GNUInstallDirs without any success. IMHO instead GNUInstallDirs should also create arch-independent vars.

As noted, two big points:

Not a fan of doing things Arch Independent. Just complicates build scripts for no apparent benefit.

Do not define variables starting with CMAKE_. I want to reject the second line of change. I accept the first part of change which moves GNUInstallDirs at the top.

CMakeLists.txt

grafikrobot

Please provide tests to ensure the changes are valid.

KOLANICH · 2021-07-22T18:25:16Z

Please provide tests to ensure the changes are valid.

I don't understand what you mean. I went to tests/lib_use_test, created build dir, entered it, configured, generated and ninjaed. Works for me. Ubuntu 21.04, clang 13.

jbadwaik

Mistakenly approved the changes.

jbadwaik

Mistakenly approved everything. Will try to approve at a more granular level.

jbadwaik

The original path is not incorrect. If there is a policy of distributions to do something specific, then we can look at it. Otherwise, no need to change the path.

CMakeLists.txt

jbadwaik

I don't mind this change. But the name can be more descriptive. CMAKE_CONFIG_DEST is not a good name in my opinion. First of all, you should not define any name that starts with CMAKE_. That space is reserved for cmake. And secondly. the name should use the same naming pattern as CMAKE_INSTALL_<X>DIR. So, instead, that name should be LYRA_INSTALL_CONFIGDIR.

jbadwaik

Thank you for dividing up the commits.

I've marked some minor changes in some commits which I am ready to accept. I've marked commits I don't like as such as well.

CMakeLists.txt

jbadwaik · 2021-07-28T03:48:14Z

CMakeLists.txt

 add_library( bfg::Lyra ALIAS lyra )

-set(CMAKE_CONFIG_DEST "${CMAKE_INSTALL_DATADIR}/lyra/cmake")
+set(CMAKE_CONFIG_DEST "${CMAKE_INSTALL_LIBDIR}/cmake/lyra")


Is there a policy by distributions to do so? If yes, we might consider it. I do not dislike this change, but I don't want to change just for change's sake.

It is the convention used in ubuntu and I don't even remember if the previous way have been working

jbadwaik · 2021-07-28T03:49:15Z

CMakeLists.txt

 # Add an alias to public name.
 add_library( bfg::Lyra ALIAS lyra )

+set(CMAKE_CONFIG_DEST "${CMAKE_INSTALL_DATADIR}/lyra/cmake")


Do not define variables starting with CMAKE_. Those names are reserved for cmake. I like this change, but the name has to be better and fir the already existing naming scheme.

I suggest LYRA_INSTALL_CONFIGDIR as the alternate which I'll be happy with.

CMakeLists.txt

jbadwaik · 2021-07-28T03:50:34Z

CMakeLists.txt

  FILES
  ${PROJECT_BINARY_DIR}/lyraConfig.cmake
-  DESTINATION ${CMAKE_INSTALL_DATADIR}/lyra/cmake/
+  DESTINATION ${CMAKE_INSTALL_DATADIR}/lyra/cmake


Why do you remove the /? Any specific reason? I do not want to change lines for change's sake.

jbadwaik · 2021-07-28T03:54:51Z

packaging/pkgconfig.pc.in

@@ -0,0 +1,7 @@
+prefix=@CMAKE_INSTALL_PREFIX@
+includedir=${prefix}/include


If you replace include with CMAKE_INSTALL_INCLUDEDIR in original place, you should also replace similar here to be consistent.

CMakeLists.txt

jbadwaik · 2021-07-28T04:00:31Z

CMakeLists.txt

+set(CPACK_PACKAGE_VENDOR "${PROJECT_NAME} developers")
+set(CPACK_PACKAGE_DESCRIPTION "${PROJECT_DESCRIPTION}")
+set(CPACK_DEBIAN_PACKAGE_NAME "lib${CPACK_PACKAGE_NAME}-dev")
+set(CPACK_RPM_PACKAGE_NAME "lib${CPACK_PACKAGE_NAME}-devel")


Are you sure that the CPack command won't fail because it will try to generate packages for every single distribution listed here? May be make sure that the cpack won't fail on different platforms.

This is something you can write a test for on each platform in the CI. The CI should generate cpack packages and succeed in doing so.

…riables defined there earlier in the script. Added a variable for path to architecture-independent libs.

…o the same dir.

…ource of the previous mistake) by introducing a variable `LYRA_INSTALL_CONFIGDIR`

…placed in /usr/<lib dir>[/<arch>]/cmake/<package name> in distros. At least, in Ubuntu and Fedora.

…onfig files should be in an arch-independent location.

…uld be taken from CMake variables.

jbadwaik suggested changes Jul 22, 2021

View reviewed changes

grafikrobot requested changes Jul 22, 2021

View reviewed changes

KOLANICH force-pushed the cpack branch from 3fe70b3 to e44c61e Compare July 22, 2021 18:10

jbadwaik approved these changes Jul 28, 2021

View reviewed changes

jbadwaik suggested changes Jul 28, 2021

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

jbadwaik suggested changes Jul 28, 2021

View reviewed changes

KOLANICH force-pushed the cpack branch 3 times, most recently from 60d09df to e766efb Compare October 21, 2021 16:25

KOLANICH force-pushed the cpack branch from e766efb to 30863f3 Compare December 9, 2021 10:29

KOLANICH force-pushed the cpack branch from 30863f3 to d3ea65f Compare February 2, 2022 12:15

KOLANICH force-pushed the cpack branch from d3ea65f to 2ec8e27 Compare November 10, 2022 14:51

KOLANICH added 9 commits January 17, 2023 18:50

Added metadata for the project in the format available for CPack.

f9845cd

Added CPack packaging.

a795de2

Added generation of a metadata file for pkg-config

803f2e8

Moved inclusion of GNUInstallDirs closer to top. We would need the va…

a923d46

…riables defined there earlier in the script. Added a variable for path to architecture-independent libs.

Fixed mistakes in CMake config file destinations. They are placed int…

a49cfa8

…o the same dir.

Eliminated repipitions of CMake config file for Lyra (that were the s…

856df8b

…ource of the previous mistake) by introducing a variable `LYRA_INSTALL_CONFIGDIR`

Changed the incorrect path to CMake config for Lyra. These files are …

4d72c62

…placed in /usr/<lib dir>[/<arch>]/cmake/<package name> in distros. At least, in Ubuntu and Fedora.

The library is header-only, it is architecture-insdependent. So pkg-c…

8c3098b

…onfig files should be in an arch-independent location.

Fixed the path to includes. It should not be hardcoded, rather it sho…

3d69d37

…uld be taken from CMake variables.

KOLANICH force-pushed the cpack branch from 2ec8e27 to 3d69d37 Compare January 17, 2023 15:51

grafikrobot added the enhancement New feature or request label Apr 3, 2023

		@@ -0,0 +1,7 @@
		prefix=@CMAKE_INSTALL_PREFIX@
		includedir=${prefix}/include

Uh oh!

Conversation

KOLANICH commented Jul 22, 2021

Uh oh!

jbadwaik left a comment • 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!

jbadwaik Jul 22, 2021

Choose a reason for hiding this comment

Uh oh!

KOLANICH Jul 22, 2021

Choose a reason for hiding this comment

Uh oh!

jbadwaik Jul 28, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

grafikrobot left a comment

Choose a reason for hiding this comment

Uh oh!

KOLANICH commented Jul 22, 2021

Uh oh!

jbadwaik left a comment • 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!

jbadwaik left a comment • 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!

jbadwaik left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jbadwaik left a comment

Choose a reason for hiding this comment

Uh oh!

jbadwaik left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jbadwaik Jul 28, 2021

Choose a reason for hiding this comment

Uh oh!

KOLANICH Oct 21, 2021 • 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!

jbadwaik Jul 28, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jbadwaik Jul 28, 2021 • 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!

jbadwaik Jul 28, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jbadwaik Jul 28, 2021 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jbadwaik left a comment •

edited

Loading

jbadwaik left a comment •

edited

Loading

jbadwaik left a comment •

edited

Loading

KOLANICH Oct 21, 2021 •

edited

Loading

jbadwaik Jul 28, 2021 •

edited

Loading

jbadwaik Jul 28, 2021 •

edited

Loading