Skip to content

Rework #14

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Rework #14

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3d759d3
Switch to CMAKE default variables
kwizart Jul 18, 2018
96683a2
Set the appropriate SONAME for the library
kwizart Jul 18, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 12 additions & 25 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,35 +48,20 @@ cmake_minimum_required (VERSION 2.6)
project (AcesContainer)

include (GenerateExportHeader)
include (GNUInstallDirs)

set( AcesContainer_MAJOR_VERSION 1 )
set( AcesContainer_MINOR_VERSION 0 )
set( AcesContainer_PATCH_VERSION 2 )
set( AcesContainer_VERSION ${AcesContainer_MAJOR_VERSION}.${AcesContainer_MINOR_VERSION}.${AcesContainer_PATCH_VERSION} )

set( INSTALL_LIB_DIR lib CACHE PATH "Install directory for libraries" )
set( INSTALL_INCLUDE_DIR include CACHE PATH "Install directory for public header files" )


if(APPLE)
set( CMAKE_MACOSX_RPATH 1 )
endif()

if( WIN32 AND NOT CYGWIN )
set(DEF_INSTALL_CMAKE_DIR CMake)
set ( CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /W0" )
else()
set(DEF_INSTALL_CMAKE_DIR lib/CMake/AcesContainer)
endif()
set(INSTALL_CMAKE_DIR ${DEF_INSTALL_CMAKE_DIR} CACHE PATH "Install directory for project CMake files" )

## convert install paths to absolute
foreach( p LIB INCLUDE CMAKE )
set( var INSTALL_${p}_DIR )
if( NOT IS_ABSOLUTE "${${var}}" )
set( ${var} "${CMAKE_INSTALL_PREFIX}/${${var}}" )
endif()
endforeach()

OPTION (BUILD_SHARED_LIBS "Build Shared Libraries" ON)
IF ( BUILD_SHARED_LIBS )
Expand All @@ -103,7 +88,13 @@ GENERATE_EXPORT_HEADER( AcesContainer
STATIC_DEFINE AcesContainer_BUILT_AS_STATIC
)

install (TARGETS AcesContainer EXPORT AcesContainerTargets DESTINATION ${INSTALL_LIB_DIR})
# Set the build version (VERSION) and the API version (SOVERSION)
set_target_properties(AcesContainer
PROPERTIES
VERSION ${AcesContainer_VERSION}
SOVERSION ${AcesContainer_MAJOR_VERSION})

install (TARGETS AcesContainer EXPORT AcesContainerTargets DESTINATION ${CMAKE_INSTALL_LIBDIR})
Copy link

@kmilos kmilos Apr 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On Windows, need to make sure binary artifacts (DLL) is installed into bin while import library in lib. This is done by defining RUNTIME, ARCHIVE, and LIBRARY destinations: https://cmake.org/cmake/help/latest/command/install.html#installing-targets

Copy link
Author

@kwizart kwizart Apr 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I Can change that, but I won't be able to test.

As I undertstood, I need to have:

install (TARGETS AcesContainer EXPORT AcesContainerTargets
   RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR}
   LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR}
   ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR}
)

Copy link

@kmilos kmilos Apr 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes! I plan to test your changes for a MINGW build and will let you know how it goes.

Copy link
Author

@kwizart kwizart Apr 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I have a flight tonight so after Easter I will try to add commit to fix theses issue. Hopefully next week.

install (FILES
aces_errors.h
aces_genericWriter.h
Expand All @@ -118,14 +109,14 @@ install (FILES
aces_types.h
aces_writeattributes.h
DESTINATION
${INSTALL_INCLUDE_DIR}/aces
${CMAKE_INSTALL_INCLUDEDIR}/aces
)


find_package( PkgConfig )
if ( PKG_CONFIG_FOUND )
configure_file(config/AcesContainer.pc.in "${PROJECT_BINARY_DIR}/AcesContainer.pc" @ONLY)
install( FILES "${PROJECT_BINARY_DIR}/AcesContainer.pc" DESTINATION lib/pkgconfig COMPONENT dev )
install( FILES "${PROJECT_BINARY_DIR}/AcesContainer.pc" DESTINATION ${CMAKE_INSTALL_LIBDIR}/pkgconfig COMPONENT dev )
endif()
Comment on lines 116 to 120
Copy link

@kmilos kmilos Apr 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, no need to depend on PkgConfig here - just doing string substitution w/ CMake built-in configure_file() command, no pkgconf commands are actually used...

Copy link
Author

@kwizart kwizart Apr 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If PkgConfig isn't found, then the $_libdir/pkgconfig "system directory" will likely not exist (when installed as system lib).
I don't think cmake will error on this.

This effectively makes pkgconf optional, which I think it is.

Copy link

@kmilos kmilos Apr 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think anything prevents you from installing the $_libdir/pkgconfig/.pc files even if the directory doesn't exist... (The only exception might be if you're building the library w/ and for MSVC; all other platforms can use it.)

One is providing this file as convenience for clients of this library at all times - pkgconf is indeed optional for them (they choose what build system and library dependency handling they use), but nothing stops you providing .pc files (and equivalent .cmake config files for that matter) at all times. Most distro packages do, without checking for pkgconf (or cmake).

Copy link

@kmilos kmilos Apr 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, it makes sense to just install always, even MSVC (i.e. vcpkg) supports it.


include_directories(
Expand All @@ -143,10 +134,6 @@ export(TARGETS AcesContainer
export(PACKAGE AcesContainer)
# export(PACKAGE AcesContainer_lib)

# Create the FooBarConfig.cmake and FooBarConfigVersion files
file(RELATIVE_PATH REL_INCLUDE_DIR "${INSTALL_CMAKE_DIR}"
"${INSTALL_INCLUDE_DIR}")

# ... for the build tree
set(CONF_INCLUDE_DIRS "${PROJECT_SOURCE_DIR}" "${PROJECT_BINARY_DIR}")
set(CONF_LIB_DIRS "${PROJECT_BINARY_DIR}")
Expand All @@ -166,10 +153,10 @@ configure_file(config/AcesContainerConfigVersion.cmake.in
install(FILES
"${PROJECT_BINARY_DIR}${CMAKE_FILES_DIRECTORY}/AcesContainerConfig.cmake"
"${PROJECT_BINARY_DIR}/AcesContainerConfigVersion.cmake"
DESTINATION "${INSTALL_CMAKE_DIR}" COMPONENT dev)
DESTINATION "${CMAKE_INSTALL_LIBDIR}/cmake" COMPONENT dev)

# Install the export set for use with the install-tree
install(EXPORT AcesContainerTargets DESTINATION
"${INSTALL_CMAKE_DIR}" COMPONENT dev)
"${CMAKE_INSTALL_LIBDIR}/cmake" COMPONENT dev)


6 changes: 3 additions & 3 deletions config/AcesContainer.pc.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,9 +45,9 @@
# A.M.P.A.S., WHETHER DISCLOSED OR UNDISCLOSED.

prefix=@CMAKE_INSTALL_PREFIX@
libdir=@INSTALL_LIB_DIR@
includedir=@INSTALL_INCLUDE_DIR@
AcesContainer_includedir=@INSTALL_INCLUDE_DIR@/aces
libdir=@CMAKE_INSTALL_LIBDIR@
includedir=@CMAKE_INSTALL_INCLUDEDIR@
AcesContainer_includedir=@CMAKE_INSTALL_INCLUDEDIR@/aces
Comment on lines +48 to +50
Copy link

@kmilos kmilos Apr 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should be ${prefix}/@CMAKE_INSTALL_<dir>@ otherwise you get only "lib" or "include": https://people.freedesktop.org/~dbn/pkg-config-guide.html

Copy link
Author

@kwizart kwizart Apr 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for review.
Sure, I can add a prefix.

Copy link
Author

@kwizart kwizart Apr 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I might have miss-understood. (there is already a prefix ).

Point is cmake doesn't pass variable relative to other variable. So if one ever one choose to use a custom layout, it should still work.

Copy link

@kmilos kmilos Apr 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, not sure there is...

The current template libdir=@CMAKE_INSTALL_LIBDIR@ will evaluate to just libdir=lib, no?

It needs to be libdir=${exec_prefix}/lib (and same for the others), as shown in the pkgconf docs linked.

So the template should be

prefix=@CMAKE_INSTALL_PREFIX@
exec_prefix=${prefix}
includedir=${prefix}/@CMAKE_INSTALL_INCLUDEDIR@
libdir=${exec_prefix}/@CMAKE_INSTALL_LIBDIR@

I also suggest getting rid of AcesContainer_includedir and just use ${includedir}/aces directly in Cflags just like the example, it's more "traditional"...


Name: AcesContainer
Description: A library containing an implementation of ACES Image Container File
Expand Down
4 changes: 2 additions & 2 deletions config/AcesContainerConfig.cmake.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,10 +53,10 @@
# find paths
get_filename_component( AcesContainer_CMAKE_DIR "${CMAKE_CURRENT_LIST_FILE}" PATH )

set(AcesContainer_INCLUDE_DIRS "@CONF_INCLUDE_DIRS@")
set(AcesContainer_INCLUDE_DIRS "@CMAKE_INSTALL_INCLUDEDIR@")

set(AcesContainer_LIBRARIES AcesContainer )
set(AcesContainer_LIBRARY_DIRS "@CONF_LIB_DIRS@" )
set(AcesContainer_LIBRARY_DIRS "@CMAKE_INSTALL_LIBDIR@" )
set(AcesContainer_VERSION "@AcesContainer_VERSION@" )

set(AcesContainer_FOUND 1 )