Use GNUInstallDirs for cmake install locations #3150

ErixenCruz · 2022-03-29T18:41:18Z

Closes CMake: convoluted use of paths #3100
Added clear title that can be used to generate release notes

Changes to the CMAKE build system to use GNUInstallDirs for installation locations. I only tested this on Ubuntu. Would someone verify that this is fine on Windows and Mac, too?

mwtoews · 2022-03-29T20:39:41Z

There are some issues generating a proj.pc file for pkg-config, but otherwise this looks good so far!

ErixenCruz · 2022-03-31T18:44:39Z

Thanks @mwtoews proj.pc should be good now. I think this is ready for review.

ErixenCruz · 2022-03-31T19:20:04Z

Ah apparently not. The problem for the Linux build seems to be moving the include directory somewhere else. I will explore /~https://github.com/nektos/act to be able to run the Github actions locally instead of pushing and waiting for it to fail.

mwtoews · 2022-03-31T20:35:17Z

I suppose the issue is around here:

PROJ/travis/install.sh

Lines 156 to 162 in b1d2fcb

    
           sed -i'.bak' -e '1c\ 
        
           prefix='"$INST"'/proj_shared_install_from_dist_renamed/subdir' $INST/proj_shared_install_from_dist_renamed/subdir/lib/pkgconfig/proj.pc 
        
           # cat $INST/proj_shared_install_from_dist_renamed/subdir/lib/pkgconfig/proj.pc 
        
           sed -i'.bak' -e '1c\ 
        
           prefix=/tmp/proj_static_install_from_dist_renamed/subdir' /tmp/proj_static_install_from_dist_renamed/subdir/lib/pkgconfig/proj.pc 
        
           # cat /tmp/proj_static_install_from_dist_renamed/subdir/lib/pkgconfig/proj.pc

this assumes there is only one prefix at the top of proj.pc to change on the first line (1c). You could use sed differently to replace each line that matches the contents of the original path with the moved path. Be aware that there are different versions of sed on the runners, so that macOS have different options than on (e.g.) Ubuntu. Also, I noticed that exec_prefix could be removed from the template, since it's no longer needed in this revision.

…refix.

mwtoews · 2022-04-01T01:53:56Z

The macOS failure because the executables are installed to:

-- Installing: /tmp/proj_static_install_from_dist/Applications/OSGEO/projinfo

due to set(CMAKE_INSTALL_BINDIR ${BUNDLEDIR}) in cmake/ProjMac.cmake. The logic doesn't seem right before, as this variable is set no matter the configuration of BUILD_FRAMEWORKS_AND_BUNDLE. Perhaps add a seperate if(BUILD_FRAMEWORKS_AND_BUNDLE) block to set this variable. I'm not even sure if anyone actually uses this CMake option, and am tempted to delete cmake/ProjMac.cmake altogether someday.

ErixenCruz · 2022-04-01T15:57:51Z

Ok I have wrapped the variable setting inside that condition. No Mac to test on, but hopefully the Mac build Github action is good now.

mwtoews

This looks excellent, and I can't see any obvious issues. Anyone else have a chance to review? Otherwise I'll merge this in a few days. Note that this should not be backported.

rouault · 2022-04-04T08:54:57Z

cmake/project-config.cmake.in

+set (@PROJECT_VARIANT_NAME@_INCLUDE_DIRS ${CMAKE_INSTALL_INCLUDEDIR})
+set (@PROJECT_VARIANT_NAME@_LIBRARY_DIRS ${CMAKE_INSTALL_LIBDIR})
+set (@PROJECT_VARIANT_NAME@_BINARY_DIRS ${CMAKE_INSTALL_BINDIR})


I believe this should rather be:

Suggested change

set (@PROJECT_VARIANT_NAME@_INCLUDE_DIRS ${CMAKE_INSTALL_INCLUDEDIR})

set (@PROJECT_VARIANT_NAME@_LIBRARY_DIRS ${CMAKE_INSTALL_LIBDIR})

set (@PROJECT_VARIANT_NAME@_BINARY_DIRS ${CMAKE_INSTALL_BINDIR})

set (@PROJECT_VARIANT_NAME@_INCLUDE_DIRS "${_ROOT}/@CMAKE_INSTALL_INCLUDEDIR@")

set (@PROJECT_VARIANT_NAME@_LIBRARY_DIRS "${_ROOT}/@CMAKE_INSTALL_LIBDIR@")

set (@PROJECT_VARIANT_NAME@_BINARY_DIRS "${_ROOT}/@CMAKE_INSTALL_BINDIR@")

as _ROOT must remain referenced

CMAKE_INSTALL_<dir> are relative paths, meant to be used with install( ... DESTINATION ...).

For absolute paths, there is CMAKE_INSTALL_FULL_<dir>. This takes into account CMAKE_INSTALL_PREFIX (but breaks CMAKE_STAGING_PREFIX).

Now what is _ROOT?

Which do we think it should be? I think it is wrong the way it is, too.

If these variables should be the include, library, and binary directories of install, then the FULL variables from GNUInstallDirs will do fine as @dg0yt says I think

Now what is _ROOT?

As far as I can see, this is a way to figure out the (actual) installation prefix from where the CMake config file is located.

If these variables should be the include, library, and binary directories of install, then the FULL variables from GNUInstallDirs will do fine

yeah that was my first thought too. But it seems the existing logic in the file was to make it possibly relocatable. And I hope that what I proposed would make it still possible

Sorry, I missed part of the context. Basically @rouault is right, and I see where _ROOT comes from now.

I will test the PR in the next days.

dg0yt · 2022-04-04T14:34:54Z

data/CMakeLists.txt

@@ -101,5 +101,5 @@ set(ALL_DATA_FILE
 )
 install(
  FILES ${ALL_DATA_FILE}
-  DESTINATION ${DATADIR}
+  DESTINATION ${PROJ_LIB_PATH}


... but this looks strange.

it's after PROJ_LIB

The naming isn't straightforward. In the context of cmake, this implies the library install path I think, though we know what PROJ_LIB means outside of cmake (the link above was what I was thinking of when I named it). What's the best name for this? DATADIR isn't too clear either

dg0yt · 2022-04-04T14:35:37Z

CMakeLists.txt

-set(CMAKECONFIGDIR "${DEFAULT_CMAKEDIR}"
-  CACHE PATH "Parent of the directory to install cmake config files into.")
+include(GNUInstallDirs)
+set(PROJ_LIB_PATH "${CMAKE_INSTALL_FULL_DATADIR}/proj")


Maybe that's not the best name for data.

cmake/project-config.cmake.in

thesamesam · 2022-04-06T04:25:23Z

Just wanted to say thanks for this!

thesamesam

I'm not a PROJ developer but I am a maintainer of it downstream (in Gentoo) and occasional contributor. Also used to doing GNUInstallDirs ports. Looks great, thanks!

include/proj/CMakeLists.txt

src/CMakeLists.txt

mwtoews · 2022-04-07T23:14:46Z

Thanks again @ErixenCruz for putting this together!

ErixenCruz added 2 commits March 29, 2022 14:33

Use GNUInstallDirs for cmake install locations

66f0a33

Fix remaining LIBDIR

9b1eb93

ErixenCruz marked this pull request as draft March 29, 2022 19:39

ErixenCruz added 2 commits March 30, 2022 11:06

Change ProjUtilites to use GNUInstallDirs

c1c23be

Use full paths for proj.pc configuration.

d416379

ErixenCruz marked this pull request as ready for review March 31, 2022 18:43

Configure proj.pc with relative path and prefix. Remove unused exec_p…

b705144

…refix.

Change install dir for Mac only when building framework and app bundle.

a3decb7

mwtoews approved these changes Apr 3, 2022

View reviewed changes

rouault reviewed Apr 4, 2022

View reviewed changes

dg0yt reviewed Apr 4, 2022

View reviewed changes

Use _ROOT as prefix to maintain moveability

94ee707

rouault reviewed Apr 4, 2022

View reviewed changes

cmake/project-config.cmake.in Outdated Show resolved Hide resolved

Surround cmake variables in @ in .in file

56c171b

thesamesam approved these changes Apr 6, 2022

View reviewed changes

mwtoews reviewed Apr 7, 2022

View reviewed changes

include/proj/CMakeLists.txt Outdated Show resolved Hide resolved

mwtoews reviewed Apr 7, 2022

View reviewed changes

src/CMakeLists.txt Outdated Show resolved Hide resolved

PR feedback

bdc6527

mwtoews merged commit b7b89e7 into OSGeo:master Apr 7, 2022

mwtoews mentioned this pull request Apr 7, 2022

CMake: convoluted use of paths #3100

Closed

ErixenCruz deleted the use-gnu-install-dirs branch April 11, 2022 13:11

alexshpilkin mentioned this pull request May 18, 2022

CMake and pkg-config files are broken when CMAKE_INSTALL_{BIN,DATAROOT,INCLUDE,LIB}DIR is absolute #3206

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use GNUInstallDirs for cmake install locations #3150

Use GNUInstallDirs for cmake install locations #3150

ErixenCruz commented Mar 29, 2022 •

edited by mwtoews

Loading

mwtoews commented Mar 29, 2022

ErixenCruz commented Mar 31, 2022

ErixenCruz commented Mar 31, 2022

mwtoews commented Mar 31, 2022

mwtoews commented Apr 1, 2022

ErixenCruz commented Apr 1, 2022

mwtoews left a comment

rouault Apr 4, 2022

dg0yt Apr 4, 2022

ErixenCruz Apr 4, 2022

rouault Apr 4, 2022

dg0yt Apr 4, 2022

dg0yt Apr 4, 2022

mwtoews Apr 4, 2022

ErixenCruz Apr 4, 2022

dg0yt Apr 4, 2022

thesamesam commented Apr 6, 2022

thesamesam left a comment •

edited

Loading

mwtoews commented Apr 7, 2022

Use GNUInstallDirs for cmake install locations #3150

Use GNUInstallDirs for cmake install locations #3150

Conversation

ErixenCruz commented Mar 29, 2022 • edited by mwtoews Loading

mwtoews commented Mar 29, 2022

ErixenCruz commented Mar 31, 2022

ErixenCruz commented Mar 31, 2022

mwtoews commented Mar 31, 2022

mwtoews commented Apr 1, 2022

ErixenCruz commented Apr 1, 2022

mwtoews left a 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

thesamesam commented Apr 6, 2022

thesamesam left a comment • edited Loading

Choose a reason for hiding this comment

mwtoews commented Apr 7, 2022

ErixenCruz commented Mar 29, 2022 •

edited by mwtoews

Loading

thesamesam left a comment •

edited

Loading