[build] Allow using all GNU directories even if libdir is set #2450

robUx4 · 2022-09-02T07:47:42Z

It's possible to use GNUInstallDirs and allow the user to force some specific
target folder. The values set by the user are not overriden in that case [1]:

If the includer does not define a value the above-shown default will be used
and the value will appear in the cache for editing by the user.

According to the comment, the main goal is not to use GNU directories when
compiling with MSVC, so the MICROSOFT check should be enough.

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

robUx4 · 2022-09-02T08:24:18Z

I don't think this build system change can provoke a sole timeout test to fail...

ethouris · 2022-09-02T08:45:11Z

What I have earlier found out about CMake's GNUInstallDirs was that it does have some sensible definition in case of Windows. Not sure if any installation definition is being generated by CMake in that case, but at least it has never been failing. Also in this description I can't see any problem found in the development or a build break due to this.

Also the condition that you removed should remain there. If GNUInstallDirs are not to be used in case of MICROSOFT (which means "compiling under Windows using MSVC"), then add this as a second condition.

robUx4 · 2022-09-02T09:18:09Z

Rebased on a stable tag. It seems master is failing on Linux.

robUx4 · 2022-09-02T09:31:55Z

What I have earlier found out about CMake's GNUInstallDirs was that it does have some sensible definition in case of Windows. Not sure if any installation definition is being generated by CMake in that case, but at least it has never been failing. Also in this description I can't see any problem found in the development or a build break due to this.

I just tried CMake+MSVC and it tries to install a library in C:/Program Files (x86)/<name>/lib instead of C:/Program Files (x86)/<name>. So at least in that case, GNUInstallDirs should not be used so that it installs in the expect Windows directory layout. Same thing between C:/Program Files (x86)/<name>/bin and C:/Program Files (x86)/<name>.

Also the condition that you removed should remain there.

I disagree. The only (known) case you don't want the GNU or UNIX layout is when building for a Windows environment. If you really want a way to enable/disable GNU/UNIX output then it should be a separate option, not rely on CMAKE_INSTALL_LIBDIR not being set. In our use case (VLC) we do set CMAKE_INSTALL_LIBDIR to a specific name. And because of that we have to also set CMAKE_INSTALL_LIBDIR and every other CMAKE_INSTALL_xx variable we can think of because GNUInstallDirs is not used. And I think we are missing one.

maxsharabayko · 2022-09-06T11:22:56Z

AppVeyor reports errors with CMake

...
-- DEVEL APPS (testing): DISABLED
cmake.exe : CMake Error at CMakeLists.txt:1389 (install):
At line:1 char:1
+ & cmake ../ -G"Visual Studio 16 2019" -DCMAKE_BUILD_TYPE=Debug -DENAB ...
+ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : NotSpecified: (CMake Error at ...1389 (install)::String) [], RemoteException
    + FullyQualifiedErrorId : NativeCommandError
 
  install PROGRAMS given no DESTINATION!
-- Configuring incomplete, errors occurred!

robUx4 · 2022-09-06T12:31:05Z

AppVeyor reports errors with CMake

...
-- DEVEL APPS (testing): DISABLED
cmake.exe : CMake Error at CMakeLists.txt:1389 (install):
At line:1 char:1
+ & cmake ../ -G"Visual Studio 16 2019" -DCMAKE_BUILD_TYPE=Debug -DENAB ...
+ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : NotSpecified: (CMake Error at ...1389 (install)::String) [], RemoteException
    + FullyQualifiedErrorId : NativeCommandError
 
  install PROGRAMS given no DESTINATION!
-- Configuring incomplete, errors occurred!

Odd, the CMakeLists.txt file only has 1354 lines.

But I guess the real error is install PROGRAMS given no DESTINATION!

robUx4 · 2022-09-06T12:53:59Z

Technically I think a script should go in sbin. But this should be backward compatible.

robUx4 · 2022-09-06T13:46:13Z

It seems that cmake doesn't automatically pick GNU dirs if they are available. So now I use a variable to tell whether GNU dirs are used and act accordingly.

maxsharabayko · 2022-09-06T16:20:52Z

@lewk2 you might want to review the PR as well.

ethouris · 2022-09-07T06:26:18Z

Ok, what is needed from this installation is:

On POSIX systems (including Mac) it should allow to install things in the "standard" location of /usr/local/[lib64,bin,share], unless there's a directory specified explicitly, in which case it should install in this very directory, as specified. The way how this directory is specified should remain unchanged.

On Windows, I don't know, although the least that should be supported there is a possibility to create a package so that it can be installed manually. Also the installation directory should be able to be customized, although this isn't currently defined how to do it (that is, usually the procedure is that you run the CMake GUI and then override whatever variables you need).

robUx4 · 2022-09-07T07:41:32Z

Added some more checks on the GNU dirs so it doesn't install things in / on Windows. The rest of the installation folders can be deduced for Windows targets to use the default directories (which can be changed with a CMake prefix).

robUx4 · 2022-09-07T09:11:40Z

For regular executables, static libraries and shared libraries, the DESTINATION argument is not required.

I guess we can't rely on the CMake documentation...

ethouris · 2022-09-07T11:47:23Z

Maybe this is true only on POSIX systems.

robUx4 · 2022-09-08T08:03:16Z

Maybe this is true only on POSIX systems.

It depends on the CMake version. So I added a variable to check that. I use the default directories when it's possible (and not overriden for cygwin). If not I use the old system. And if the variables are not found, I emit a message saying installation is not possible. One can still build the project in that case.

It seems DESTINATION was only allowed to be omitted in 3.14

https://cmake.org/cmake/help/v3.14/command/install.html#targets
vs
https://cmake.org/cmake/help/v3.13/command/install.html#targets

robUx4 · 2022-09-08T08:05:17Z

It looks like all targets are OK now. Only some timeouts during tests which I don't think is related to these changes.

CMakeLists.txt

maxsharabayko · 2022-09-08T12:57:11Z

Review comments

Default CMake config on Windows with MSVC defines the following CMake variables (valid both for master and this PR):

> cmake ./
(...)
-- CMAKE_INSTALL_BINDIR: bin
-- CMAKE_INSTALL_INCLUDEDIR: include
-- CMAKE_INSTALL_LIBDIR: lib
-- INSTALL_SHARED_DIR: lib

Trying to build the "install" target (wants to install to C:/Program Files (x86)/SRT/lib):

> cmake --build . --target install --config Debug
  (...)
  Building Custom Rule D:/Projects/srt/CMakeLists.txt
  -- Install configuration: "Debug"
  CMake Error at cmake_install.cmake:37 (file):
    file cannot create directory: C:/Program Files (x86)/SRT/lib.  Maybe need
    administrative privileges.

robUx4 · 2022-09-09T06:32:12Z

Default CMake config on Windows with MSVC defines the following CMake variables

That's only true when running with CMake 3.14 and above. The current minimum CMake version is 2.8.12.

I just tried a fresh project with MSVC and not using GNUInstallDirs. It also tries to install in C:/Program Files (x86)/<project>/lib. So using GNUInstallDirs or not with MSVC should have no impact. But again, that depends on the CMake version. So maybe we should just ditch the GNU_DIRS option and just check the CMake version.

It seems DESTINATION was only allowed to be omitted in 3.14 https://cmake.org/cmake/help/v3.14/command/install.html#targets vs https://cmake.org/cmake/help/v3.13/command/install.html#targets After that the install destination directories can be deduced.

It's possible to use GNUInstallDirs and allow the user to force some specific target folder. The values set by the user are not overriden in that case [1]: > If the includer does not define a value the above-shown default will be used > and the value will appear in the cache for editing by the user. With MSVC builds this doesn't change the default values used to install targets. [1] https://cmake.org/cmake/help/latest/module/GNUInstallDirs.html

We don't need to use CMAKE_INSTALL_BINDIR which is not defined when GNUInstallDirs is not used. Using "TYPE BIN" will have the same result and will automatically pick the same as CMAKE_INSTALL_BINDIR. https://cmake.org/cmake/help/latest/command/install.html#installing-files

It is implied by the target type: > For regular executables, static libraries and shared libraries, the > DESTINATION argument is not required. For these target types, when > DESTINATION is omitted, a default destination will be taken from the > appropriate variable from GNUInstallDirs, or set to a built-in default value > if that variable is not defined. https://cmake.org/cmake/help/v3.14/command/install.html#targets

It is implied by each target type (static and dynamic libraries here): > For regular executables, static libraries and shared libraries, the > DESTINATION argument is not required. For these target types, when > DESTINATION is omitted, a default destination will be taken from the > appropriate variable from GNUInstallDirs, or set to a built-in default value > if that variable is not defined. https://cmake.org/cmake/help/v3.14/command/install.html#targets

…lable It wouldn't make a lot of sense to expect pkg-config otherwise. We shouldn't install in CMAKE_INSTALL_LIBDIR if it's not set (it will install in /pkgconfig).

We shouldn't install in CMAKE_INSTALL_INCLUDEDIR if it's not set (it will install them in /srt).

ethouris · 2022-09-09T09:23:05Z

Note that on Windows we may potentially allow the CMake minimum version to be higher - should that help. Only on Linux we need this 2.8.11 compatibility.

We do force CMAKE_INSTALL_LIBDIR which makes srt not use any other of the GNU directories... Merged upstream: Haivision/srt#2450

We do force CMAKE_INSTALL_LIBDIR which makes srt not use any other of the GNU directories... Merged upstream: Haivision/srt#2450 (cherry picked from commit 055841e)

maxsharabayko added this to the Next Release milestone Sep 2, 2022

robUx4 force-pushed the libdir-set branch from 3ee27d2 to 3e1b26a Compare September 2, 2022 09:17

maxsharabayko added [build] Area: Changes in build files Type: Maintenance Work required to maintain or clean up the code labels Sep 2, 2022

robUx4 force-pushed the libdir-set branch 2 times, most recently from 63421b7 to 9d69ffb Compare September 6, 2022 12:52

robUx4 force-pushed the libdir-set branch from 9d69ffb to a1fb035 Compare September 6, 2022 13:45

robUx4 force-pushed the libdir-set branch from caf99f4 to 72b3e49 Compare September 7, 2022 10:53

robUx4 force-pushed the libdir-set branch from 72b3e49 to 4dba7df Compare September 7, 2022 12:01

maxsharabayko reviewed Sep 8, 2022

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

CMakeLists.txt Outdated Show resolved Hide resolved

robUx4 force-pushed the libdir-set branch from 4dba7df to 4f9bd06 Compare September 8, 2022 12:56

robUx4 added 2 commits September 9, 2022 08:39

robUx4 added 2 commits September 9, 2022 08:54

robUx4 force-pushed the libdir-set branch from 4f9bd06 to 16843c6 Compare September 9, 2022 06:59

robUx4 added 3 commits September 9, 2022 09:00

[build] only install pkg-config files if CMAKE_INSTALL_LIBDIR is avai…

150637c

…lable It wouldn't make a lot of sense to expect pkg-config otherwise. We shouldn't install in CMAKE_INSTALL_LIBDIR if it's not set (it will install in /pkgconfig).

[build] only install headers if CMAKE_INSTALL_INCLUDEDIR is available

3daf891

We shouldn't install in CMAKE_INSTALL_INCLUDEDIR if it's not set (it will install them in /srt).

robUx4 force-pushed the libdir-set branch from 16843c6 to 3daf891 Compare September 9, 2022 07:01

maxsharabayko approved these changes Sep 9, 2022

View reviewed changes

maxsharabayko merged commit 95d82c4 into Haivision:master Sep 9, 2022

robUx4 deleted the libdir-set branch September 9, 2022 11:42

vlc-mirrorer pushed a commit to videolan/vlc that referenced this pull request Sep 10, 2022

contrib: srt: don't set default GNU folders

055841e

We do force CMAKE_INSTALL_LIBDIR which makes srt not use any other of the GNU directories... Merged upstream: Haivision/srt#2450

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[build] Allow using all GNU directories even if libdir is set #2450

[build] Allow using all GNU directories even if libdir is set #2450

robUx4 commented Sep 2, 2022

robUx4 commented Sep 2, 2022

ethouris commented Sep 2, 2022

robUx4 commented Sep 2, 2022

robUx4 commented Sep 2, 2022 •

edited

Loading

maxsharabayko commented Sep 6, 2022

robUx4 commented Sep 6, 2022

robUx4 commented Sep 6, 2022

robUx4 commented Sep 6, 2022

maxsharabayko commented Sep 6, 2022

ethouris commented Sep 7, 2022

robUx4 commented Sep 7, 2022

robUx4 commented Sep 7, 2022

ethouris commented Sep 7, 2022

robUx4 commented Sep 8, 2022

robUx4 commented Sep 8, 2022

maxsharabayko commented Sep 8, 2022

robUx4 commented Sep 9, 2022

ethouris commented Sep 9, 2022

[build] Allow using all GNU directories even if libdir is set #2450

[build] Allow using all GNU directories even if libdir is set #2450

Conversation

robUx4 commented Sep 2, 2022

robUx4 commented Sep 2, 2022

ethouris commented Sep 2, 2022

robUx4 commented Sep 2, 2022

robUx4 commented Sep 2, 2022 • edited Loading

maxsharabayko commented Sep 6, 2022

robUx4 commented Sep 6, 2022

robUx4 commented Sep 6, 2022

robUx4 commented Sep 6, 2022

maxsharabayko commented Sep 6, 2022

ethouris commented Sep 7, 2022

robUx4 commented Sep 7, 2022

robUx4 commented Sep 7, 2022

ethouris commented Sep 7, 2022

robUx4 commented Sep 8, 2022

robUx4 commented Sep 8, 2022

maxsharabayko commented Sep 8, 2022

Review comments

robUx4 commented Sep 9, 2022

ethouris commented Sep 9, 2022

robUx4 commented Sep 2, 2022 •

edited

Loading