Stlink: [install] CMakeLists.txt hardcodes /etc/... paths

[bjornfor]

Please use GNUInstallDirs[1] so that sysconfdir can be passed to cmake in a standard way.

(In Nixpkgs we now have to patch CMakeLists.txt, and that's pointless extra work for downstreams.)

[1] https://cmake.org/cmake/help/v3.0/module/GNUInstallDirs.html

[xor-gate]

This is only available in CMake 3.0 or higher, currently we support 2.6 and up. This will break compatability.

[xor-gate]

You may propose a PR/patch and try to not break compat with 2.6-3.0. Thanks!

[Nightwalker-87]

See also #899.

[Nightwalker-87]

We should be able to deal with this now, as the minimum version requirement for cmake has been raised to 3.4.2, so one does no longer need to worry about any broken compatibilities.

[Nightwalker-87]

@bjornfor: I have added the GNUInstallDirs.cmake module locally, but are not quite sure yet how to properly integrate it into the build configuration we currently have. I'll open a new branch for this, so you or anyone else can have a look at it. I am sure there are people who know more about cmake than I currently do...

[bjornfor]

@Nightwalker-87 : GNUInstallDirs.cmake is distributed with cmake, you don't need to add it yourself to the codebase.

To use it just

include(GNUInstallDirs)

# Now you have access to all the variables mentioned in the docs
message("/etc files will be installed to ${CMAKE_INSTALL_FULL_SYSCONFDIR}")

[Nightwalker-87]

I understand, but is this really all that has do be done unrelated to any custom configuration?

[bjornfor]

I think so. Heres an (untested) diff of how I'd replace two hardcoded /etc instances in stlink:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 90378f9..e81f621 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -2,8 +2,9 @@ cmake_minimum_required(VERSION 2.8.7)
 set(CMAKE_USER_MAKE_RULES_OVERRIDE cmake/c_flag_overrides.cmake)
 project(stlink C)
 set(PROJECT_DESCRIPTION "Open source version of the STMicroelectronics Stlink Tools")
-set(STLINK_UDEV_RULES_DIR "/etc/udev/rules.d" CACHE PATH "Udev rules directory")
-set(STLINK_MODPROBED_DIR "/etc/modprobe.d" CACHE PATH "modprobe.d directory")
+include(GNUInstallDirs)
+set(STLINK_UDEV_RULES_DIR "${CMAKE_INSTALL_FULL_SYSCONFDIR}/udev/rules.d" CACHE PATH "Udev rules directory")
+set(STLINK_MODPROBED_DIR "${CMAKE_INSTALL_FULL_SYSCONFDIR}/modprobe.d" CACHE PATH "modprobe.d directory")
 set(STLINK_STATIC_LIB ON CACHE BOOL "Install static lib")

 if( IS_DIRECTORY ${LIB_INSTALL_DIR})

[bjornfor]

Ah, the remaining "/etc/" instances are in comments or documentation, so I think that patch above is all that's needed. (The patching in nixpkgs was done with sed and no GNUInstallDirs so it's not directly applicable.)

[Nightwalker-87]

@bjornfor: Can you test it?

[bjornfor]

It does work, I played around with this script:

#!/usr/bin/env bash                                                                                                                                                                                                                                                                                                          

set -x                                                                                                                                                                                                                                                                                                                       

cmakeFlags=                                                                                                                                                                                                                                                                                                                  
#cmakeFlags="$cmakeFlags -DCMAKE_INSTALL_SYSCONFDIR=/etc"                                                                                                                                                                                                                                                                    

cmakeFlags="$cmakeFlags -DCMAKE_INSTALL_PREFIX=/local"                                                                                                                                                                                                                                                                       
#cmakeFlags="$cmakeFlags -DCMAKE_INSTALL_PREFIX=/tmp/stlink-prefix"                                                                                                                                                                                                                                                          
#cmakeFlags="$cmakeFlags -DCMAKE_INSTALL_PREFIX=/usr"                                                                                                                                                                                                                                                                        
#cmakeFlags="$cmakeFlags -DCMAKE_INSTALL_PREFIX=/usr/local"                                                                                                                                                                                                                                                                  
#cmakeFlags="$cmakeFlags -DCMAKE_INSTALL_PREFIX=/opt"                                                                                                                                                                                                                                                                        
#cmakeFlags="$cmakeFlags -DCMAKE_INSTALL_PREFIX=/opt/stlink"                                                                                                                                                                                                                                                                 

nix-shell -p cmake pkgconfig libusb --command "mkdir -p build && cd build && rm -rf ../build/* && cmake $cmakeFlags .."                                                                                                                                                                                                      

I also edited CMakeLists.txt to print out interesting variables.

I guess the default CMAKE_INSTALL_FULL_SYSCONFDIR might not be what everyone expects (not that I know what that is though), so perhaps it's a good idea to update documentation to suggest building with -DCMAKE_INSTALL_SYSCONFDIR=/etc to ensure no surprises in the common case LSB distro case. Background: testing shows that -DCMAKE_INSTALL_PREFIX=/usr results in CMAKE_INSTALL_FULL_SYSCONFDIR=/usr/etc.

[Nightwalker-87]

Let me take a closer look at this and related documentation on the web.
We should have an idea about how this works in detail.
I'll update our project documentation if necessary.

[Nightwalker-87]

I think -DCMAKE_INSTALL_SYSCONFDIR=/etc should rather go into our cmake config as a default by referring to: https://cmake.org/cmake/help/latest/module/GNUInstallDirs.html

However I'm not sure yet, whether we

• should preset -DCMAKE_INSTALL_PREFIX= or
• leave that flag to the user or
• do a preset (first option) with allowing him to overwrite this preset flag.

[Nightwalker-87]

@bjornfor: What's your opinion on this?

[bjornfor]

To have the least support issues, I think I would preset -DCMAKE_INSTALL_SYSCONFDIR=/etc and leave -DCMAKE_INSTALL_PREFIX= unset.

But I'm really unsure of presetting /etc (vs just documenting the option). I mean the option does have a default value already, so with /etc as overridden default it would no longer behave as documented in GNUInstallDirs.

[Nightwalker-87]

Ok, thanks for your feedback. I'll add this to our documentation then and we can ensure the defaults as documented in GNUInstallDirs, which also appears to be in the sense of the opening post.

[Nightwalker-87]

I think we should take a closer look at the previous configuration now, after including GNUInstallDirs to ensure there are no conflicts regarding directory paths. Otherwise we may find that old settings reconfigure standard settings of the former which clearly would not help...

[Nightwalker-87]

@bjornfor: As proposed, I've added a note on cmake build options related to GNUInstallDirs to our compiling manual. Thus this issue is now fully addressed and resolved.

[bjornfor]

Thanks!