Minor cmake fixes #385

lgbaldoni · 2024-01-19T08:54:56Z

No description provided.

kmilos · 2024-01-19T09:39:59Z

CMakeLists.txt

@@ -173,7 +173,7 @@ if (USE_CRYPTO)
 endif ()

 if(KVZ_BUILD_SHARED_LIBS)
-  list( APPEND CMAKE_INSTALL_RPATH "${CMAKE_INSTALL_PREFIX}/lib" "./" "../lib" )  
+  list( APPEND CMAKE_INSTALL_RPATH "${CMAKE_INSTALL_PREFIX}/lib${LIB_SUFFIX}" "./" "../lib" )  


Are there any cases where GNUInstallDirs is broken?

See https://gitlab.kitware.com/cmake/cmake/-/issues/18640 which suggests $LIB_SUFFIX is maybe not the best way to go...

@kmilos you're completely right and I don't know how I could forget about it.

kmilos · 2024-01-19T13:21:58Z

CMakeLists.txt

@@ -173,7 +173,7 @@ if (USE_CRYPTO)
 endif ()

 if(KVZ_BUILD_SHARED_LIBS)
-  list( APPEND CMAKE_INSTALL_RPATH "${CMAKE_INSTALL_PREFIX}/lib" "./" "../lib" )  
+  list( APPEND CMAKE_INSTALL_RPATH "${CMAKE_INSTALL_PREFIX}/${CMAKE_INSTALL_LIBDIR}" "./" "../lib" )  


Great, but CMAKE_INSTALL_LIBDIR can be an absolute path on some platforms like NixOS... Perhaps just use ${CMAKE_INSTALL_FULL_LIBDIR} instead of ${CMAKE_INSTALL_PREFIX}/${CMAKE_INSTALL_LIBDIR}?

I remember that problem on openSUSE, years ago. I thought it had been solved. Not everywhere, it comes out.

kmilos · 2024-01-19T15:05:53Z

src/kvazaarCMake.pc.in

-libdir=${prefix}/@CMAKE_INSTALL_LIBDIR@
+libdir=@CMAKE_INSTALL_FULL_LIBDIR@


Ah, here you actually don't want an absolute path always in order to enable a relocatable package...

Instead, one has to check if CMAKE_INSTALL_LIBDIR was absolute and act accordingly. See for example lensfun/lensfun@542b989

And while you're doing that, you'll realize there's no need for two separate .pc.in templates, you're setting a couple of placeholder variables anyway.

Considering that the kvazaarCMake.pc.in is based on the one that is used by uvg266, we are going to be maintaining two templates regardless, unless automake can also use the CMake version. Additionally, the template has been updated literally once, nine years ago, it is not really an issue to have two templates, even if it is not an optimal solution.

If you are adamant on us not having two templates on this repository I will accept a PR, but please submit a similar PR for uvg266 also.

If you are adamant on us not having two templates on this repository

I'm not. Just sharing what has become sort of "best practice" I've experienced across several imaging related libraries, especially ones that are (or were) supporting both autotools and cmake, like libtiff and libheif for example. I see no reason not to reuse it...

It's of course your prerogative how you want to maintain your repositories, and apologies to have annoyed you with my comments.

Yes, I understand why having two templates cannot be considered a best practice. However, considering the low maintenance needed for these files, and the fact that we aim not to support both autotools and cmake, but replace autotools with cmake, I'm fine with not following best practices here.

For your other suggestions I'm grateful, as I have very little experience with CMake.

apologies to have annoyed you with my comments.

I interpreted your usage of ellipsis as a criticism towards the fact that I was not aware of the best practices, which I assume was not the intention? I would like to apologize for my maybe overly harsh comments towards you.

Otherwise, in your opinion, does the current state of the PR adhere to the best practices?

No problem. There's just two more minor details, I'll leave comments on the latest commits.

kmilos · 2024-01-22T09:56:00Z

CMakeLists.txt

+if(IS_ABSOLUTE "${CMAKE_INSTALL_LIBDIR}")
+  set(KVAZAAR_PC_LIBDIR "${CMAKE_INSTALL_LIBDIR}")
+else()
+  set(KVAZAAR_PC_LIBDIR "\${prefix}/${CMAKE_INSTALL_LIBDIR}")


Suggested change

set(KVAZAAR_PC_LIBDIR "\${prefix}/${CMAKE_INSTALL_LIBDIR}")

set(KVAZAAR_PC_LIBDIR "\${exec_prefix}/${CMAKE_INSTALL_LIBDIR}")

Will also need to add and handle KVAZAAR_PC_INCDIR, but that one indeed w/ \${prefix}

kmilos · 2024-01-22T09:58:22Z

src/kvazaarCMake.pc.in

@@ -1,6 +1,6 @@
 prefix=@CMAKE_INSTALL_PREFIX@
 exec_prefix=${prefix}
-libdir=${prefix}/lib
+libdir=@KVAZAAR_PC_LIBDIR@
 incdir=${prefix}/include


Add @KVAZAAR_PC_INCDIR@

kmilos · 2024-01-22T10:00:03Z

CMakeLists.txt

 if(KVZ_BUILD_SHARED_LIBS) # Just add the lib to the bin directory for now
  if(MSVC)
    install(TARGETS kvazaar DESTINATION ${CMAKE_INSTALL_PREFIX}/bin)
  endif()
 endif()
 install(FILES ${PROJECT_SOURCE_DIR}/src/kvazaar.h DESTINATION ${CMAKE_INSTALL_PREFIX}/include)
-install(FILES ${PROJECT_SOURCE_DIR}/doc/kvazaar.1 DESTINATION ${CMAKE_INSTALL_PREFIX}/share/man)
+install(FILES ${PROJECT_SOURCE_DIR}/doc/kvazaar.1 DESTINATION ${CMAKE_INSTALL_PREFIX}/share/man/man1)


Suggested change

install(FILES ${PROJECT_SOURCE_DIR}/doc/kvazaar.1 DESTINATION ${CMAKE_INSTALL_PREFIX}/share/man/man1)

install(FILES ${PROJECT_SOURCE_DIR}/doc/kvazaar.1 DESTINATION ${CMAKE_INSTALL_MANDIR})

kmilos · 2024-01-22T10:00:47Z

CMakeLists.txt

 install(TARGETS kvazaar-bin DESTINATION ${CMAKE_INSTALL_PREFIX}/bin)
-install(TARGETS kvazaar DESTINATION ${CMAKE_INSTALL_PREFIX}/lib)
+install(TARGETS kvazaar DESTINATION ${CMAKE_INSTALL_LIBDIR})
 if(KVZ_BUILD_SHARED_LIBS) # Just add the lib to the bin directory for now
  if(MSVC)
    install(TARGETS kvazaar DESTINATION ${CMAKE_INSTALL_PREFIX}/bin)
  endif()
 endif()
 install(FILES ${PROJECT_SOURCE_DIR}/src/kvazaar.h DESTINATION ${CMAKE_INSTALL_PREFIX}/include)


Use CMAKE_INSTALL_INCLUDEDIR

kmilos · 2024-01-22T10:02:38Z

CMakeLists.txt

@@ -281,16 +289,16 @@ source_group( "" FILES ${SOURCE_GROUP_TOPLEVEL})

 # ToDo: make configurable

-install(FILES ${PROJECT_SOURCE_DIR}/src/kvazaar.pc DESTINATION ${CMAKE_INSTALL_PREFIX}/share/pkgconfig)
+install(FILES ${PROJECT_SOURCE_DIR}/src/kvazaar.pc DESTINATION ${CMAKE_INSTALL_LIBDIR}/pkgconfig)
 install(TARGETS kvazaar-bin DESTINATION ${CMAKE_INSTALL_PREFIX}/bin)


Use CMAKE_INSTALL_BINDIR

kmilos · 2024-01-22T10:07:53Z

CMakeLists.txt

 install(TARGETS kvazaar-bin DESTINATION ${CMAKE_INSTALL_PREFIX}/bin)
-install(TARGETS kvazaar DESTINATION ${CMAKE_INSTALL_PREFIX}/lib)
+install(TARGETS kvazaar DESTINATION ${CMAKE_INSTALL_LIBDIR})
 if(KVZ_BUILD_SHARED_LIBS) # Just add the lib to the bin directory for now
  if(MSVC)


Why is this MSVC only? One can build shared libs on MINGW as well, so this should potentially be WIN32?

Also, maybe there is no need to special case this anyway? If you do

install(TARGETS kvazaar RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR} ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR} LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR}

this will work out for shared and static libraries for all platforms.

Also, maybe there is no need to special case this anyway? If you do

RUNTIME is not being installed that way. Do I need to define it somewhere first?

kmilos · 2024-01-22T12:07:35Z

CMakeLists.txt

 else()
-  set(KVAZAAR_PC_LIBDIR "\${prefix}/${CMAKE_INSTALL_LIBDIR}")
+  set(KVAZAAR_PC_LIBDIR "\${exec_prefix}/${CMAKE_INSTALL_LIBDIR}")
+  set(KVAZAAR_PC_INCDIR "\${prefix}/${CMAKE_INSTALL_INCLUDEDIR}")


Suggested change

set(KVAZAAR_PC_INCDIR "\${prefix}/${CMAKE_INSTALL_INCLUDEDIR}")

endif()

if(IS_ABSOLUTE "${CMAKE_INSTALL_INCLUDEDIR}")

set(KVAZAAR_PC_INCDIR "${CMAKE_INSTALL_INCLUDEDIR}")

else()

set(KVAZAAR_PC_INCDIR "\${prefix}/${CMAKE_INSTALL_INCLUDEDIR}")

Silly me. Although it would have worked most of the time:)

Probably, but there is no guarantee I guess!

kmilos · 2024-01-22T12:07:49Z

CMakeLists.txt

@@ -101,8 +101,10 @@ add_definitions(-DCMAKE_BUILD)
 # Set correct pkgconfig libdir variable
 if(IS_ABSOLUTE "${CMAKE_INSTALL_LIBDIR}")
  set(KVAZAAR_PC_LIBDIR "${CMAKE_INSTALL_LIBDIR}")
+  set(KVAZAAR_PC_INCDIR "${CMAKE_INSTALL_INCLUDEDIR}")


Suggested change

set(KVAZAAR_PC_INCDIR "${CMAKE_INSTALL_INCLUDEDIR}")

kmilos · 2024-01-22T12:14:26Z

CMakeLists.txt

 install(TARGETS kvazaar
-  RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR}


Why leave this out? RUNTIME DESTINATION covers the .dll on Windows.

Ouch, misunderstood.

Yep, it's not just for the binary/.exe

kmilos · 2024-01-22T12:22:56Z

I think it looks ok now 👍

Should still get tested on as many different platforms (and configs) as possible before releasing though...

lgbaldoni · 2024-01-22T13:36:28Z

Should still get tested on as many different platforms (and configs) as possible before releasing though...

That would require setting up docker/appveyor accounts...

kmilos · 2024-01-22T13:49:47Z

That would require setting up docker/appveyor accounts...

You can cover at least Linux, macOS and Windows (both MSVC and MinGW) w/ readily available GitHub Actions.

Jovasa · 2024-01-25T11:45:23Z

I tested on Manjaro, Ubuntu, MSVC, and MinGW, and as far I can tell, everything seems to be working as it should be.

Big thanks to you both!

lgbaldoni added 3 commits January 19, 2024 09:49

Link libkvazaar to libm

12a6915

Fix lib installation paths

0802a0d

Fix man installation path

ecbc1a4

kmilos reviewed Jan 19, 2024

View reviewed changes

Use newer cmake macro for libdir

ebbdaac

kmilos reviewed Jan 19, 2024

View reviewed changes

Use CMAKE_INSTALL_FULL_LIBDIR

1492496

kmilos reviewed Jan 19, 2024

View reviewed changes

lgbaldoni added 2 commits January 21, 2024 10:20

Revert CMAKE_INSTALL_FULL_LIBDIR

3697d38

Account for non-absolute libdir in pkgconfig file

0c3b5d0

kmilos reviewed Jan 22, 2024

View reviewed changes

lgbaldoni added 3 commits January 22, 2024 12:47

Further pkgconfig refinement

fe021f4

Use a couple macros more

d1bde02

Use more elegant general installer

426a7d1

kmilos reviewed Jan 22, 2024

View reviewed changes

Revert executable install command

6c7722e

kmilos reviewed Jan 22, 2024

View reviewed changes

lgbaldoni added 2 commits January 22, 2024 13:14

Specific absolute path detection for incdir

f282d16

Re-add .dll install command

ce2799d

Jovasa merged commit bb89833 into ultravideo:master Jan 25, 2024

chenrui333 mentioned this pull request Apr 11, 2024

kvazaar 2.3.1 Homebrew/homebrew-core#168597

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Minor cmake fixes #385

Minor cmake fixes #385

lgbaldoni commented Jan 19, 2024

kmilos Jan 19, 2024 •

edited

Loading

lgbaldoni Jan 19, 2024

kmilos Jan 19, 2024

lgbaldoni Jan 19, 2024

kmilos Jan 19, 2024 •

edited

Loading

kmilos Jan 19, 2024

Jovasa Jan 19, 2024

kmilos Jan 19, 2024

Jovasa Jan 22, 2024

kmilos Jan 22, 2024

kmilos Jan 22, 2024

kmilos Jan 22, 2024

kmilos Jan 22, 2024

kmilos Jan 22, 2024

kmilos Jan 22, 2024

kmilos Jan 22, 2024

kmilos Jan 22, 2024

lgbaldoni Jan 22, 2024

kmilos Jan 22, 2024

lgbaldoni Jan 22, 2024

kmilos Jan 22, 2024

kmilos Jan 22, 2024

kmilos Jan 22, 2024

lgbaldoni Jan 22, 2024

kmilos Jan 22, 2024

kmilos commented Jan 22, 2024 •

edited

Loading

lgbaldoni commented Jan 22, 2024

kmilos commented Jan 22, 2024

Jovasa commented Jan 25, 2024

		libdir=${prefix}/@CMAKE_INSTALL_LIBDIR@
		libdir=@CMAKE_INSTALL_FULL_LIBDIR@

	set(KVAZAAR_PC_LIBDIR "\${prefix}/${CMAKE_INSTALL_LIBDIR}")
	set(KVAZAAR_PC_LIBDIR "\${exec_prefix}/${CMAKE_INSTALL_LIBDIR}")

	install(FILES ${PROJECT_SOURCE_DIR}/doc/kvazaar.1 DESTINATION ${CMAKE_INSTALL_PREFIX}/share/man/man1)
	install(FILES ${PROJECT_SOURCE_DIR}/doc/kvazaar.1 DESTINATION ${CMAKE_INSTALL_MANDIR})

		install(TARGETS kvazaar
		RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR}

Minor cmake fixes #385

Minor cmake fixes #385

Conversation

lgbaldoni commented Jan 19, 2024

kmilos Jan 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmilos Jan 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmilos commented Jan 22, 2024 • edited Loading

lgbaldoni commented Jan 22, 2024

kmilos commented Jan 22, 2024

Jovasa commented Jan 25, 2024

kmilos Jan 19, 2024 •

edited

Loading

kmilos Jan 19, 2024 •

edited

Loading

kmilos commented Jan 22, 2024 •

edited

Loading