Skip to content

Commit

Permalink
cmake: only set output paths with multiple outputs
Browse files Browse the repository at this point in the history 
This brings cmake inline with the behaviour used for configure
scripts, defined in multiple-outputs.sh.  It's important because
that setup hook will only set shareDocName if multiple outputs are in
use (and setOutputFlags hasn't been disabled).  So previously,
CMAKE_INSTALL_DOCDIR would be set to $out/share/doc for single-output
derivations, instead of $out/share/doc/$shareDocName, which would
result in collisions.

Since this hook now uses the setOutputFlags variable, I had to remove
the empty assignment of it added in
a714284.

Fixes: #82304
  • Loading branch information
@alyssais
alyssais committed Jun 29, 2020
1 parent 1421404 commit be1b225
Showing 1 changed file with 17 additions and 13 deletions.
30 changes: 17 additions & 13 deletions pkgs/development/tools/build-managers/cmake/setup-hook.sh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,19 +68,24 @@ cmakeConfigurePhase() {
# nix/store directory.
cmakeFlags="-DCMAKE_INSTALL_NAME_DIR=${!outputLib}/lib $cmakeFlags"

# This ensures correct paths with multiple output derivations
# It requires the project to use variables from GNUInstallDirs module
# https://cmake.org/cmake/help/latest/module/GNUInstallDirs.html
cmakeFlags="-DCMAKE_INSTALL_BINDIR=${!outputBin}/bin $cmakeFlags"
cmakeFlags="-DCMAKE_INSTALL_SBINDIR=${!outputBin}/sbin $cmakeFlags"
cmakeFlags="-DCMAKE_INSTALL_INCLUDEDIR=${!outputInclude}/include $cmakeFlags"
cmakeFlags="-DCMAKE_INSTALL_OLDINCLUDEDIR=${!outputInclude}/include $cmakeFlags"
cmakeFlags="-DCMAKE_INSTALL_MANDIR=${!outputMan}/share/man $cmakeFlags"
cmakeFlags="-DCMAKE_INSTALL_INFODIR=${!outputInfo}/share/info $cmakeFlags"
cmakeFlags="-DCMAKE_INSTALL_DOCDIR=${!outputDoc}/share/doc/${shareDocName} $cmakeFlags"
if [ "$outputs" != "out" -a -n "${setOutputFlags-1}" ]; then
# This ensures correct paths with multiple output derivations
# It requires the project to use variables from GNUInstallDirs module
# https://cmake.org/cmake/help/latest/module/GNUInstallDirs.html
cmakeFlags="-DCMAKE_INSTALL_BINDIR=${!outputBin}/bin $cmakeFlags"
cmakeFlags="-DCMAKE_INSTALL_SBINDIR=${!outputBin}/sbin $cmakeFlags"
cmakeFlags="-DCMAKE_INSTALL_INCLUDEDIR=${!outputInclude}/include $cmakeFlags"
cmakeFlags="-DCMAKE_INSTALL_OLDINCLUDEDIR=${!outputInclude}/include $cmakeFlags"
cmakeFlags="-DCMAKE_INSTALL_MANDIR=${!outputMan}/share/man $cmakeFlags"
cmakeFlags="-DCMAKE_INSTALL_INFODIR=${!outputInfo}/share/info $cmakeFlags"
cmakeFlags="-DCMAKE_INSTALL_DOCDIR=${!outputDoc}/share/doc/${shareDocName} $cmakeFlags"
cmakeFlags="-DCMAKE_INSTALL_LIBEXECDIR=${!outputLib}/libexec $cmakeFlags"
cmakeFlags="-DCMAKE_INSTALL_LOCALEDIR=${!outputLib}/share/locale $cmakeFlags"
fi

# This output flag must always be set, unlike the others, because
# otherwise we end up with lib64.
cmakeFlags="-DCMAKE_INSTALL_LIBDIR=${!outputLib}/lib $cmakeFlags"
cmakeFlags="-DCMAKE_INSTALL_LIBEXECDIR=${!outputLib}/libexec $cmakeFlags"
cmakeFlags="-DCMAKE_INSTALL_LOCALEDIR=${!outputLib}/share/locale $cmakeFlags"

# Don’t build tests when doCheck = false
if [ -z "${doCheck-}" ]; then
Expand Down Expand Up @@ -115,7 +120,6 @@ cmakeConfigurePhase() {
}

if [ -z "${dontUseCmakeConfigure-}" -a -z "${configurePhase-}" ]; then
setOutputFlags=
configurePhase=cmakeConfigurePhase
fi

Expand Down

7 comments on commit be1b225

@vcunat
Copy link
Member

@vcunat vcunat commented on be1b225 Jul 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This broke qt5.qtwebkit. I wonder if other packages might also suffer. EDIT: perhaps not this exact commit, but this PR #91277 is probably the trigger anyway.

The build succeeds, but some references from *.pc and cmake got bad (compared to nixpkgs master):

CMake Error in src/Mod/Web/Gui/CMakeLists.txt:
  Imported target "Qt5::WebKitWidgets" includes non-existent path

    "/nix/store/xnvnjxkq4pmjvsqkkqk2a6xh8ksn1fi2-qtwebkit-5.212-alpha-01-26-2018/include"

  in its INTERFACE_INCLUDE_DIRECTORIES.

and Qt5WebKit.pc:

Libs: -L/nix/store/xnvnjxkq4pmjvsqkkqk2a6xh8ksn1fi2-qtwebkit-5.212-alpha-01-26-2018//nix/store/xnvnjxkq4pmjvsqkkqk2a6xh8ksn1fi2-qtwebkit-5.212-alpha-01-26-2018/lib -lQt5WebKit
Cflags: -I/nix/store/xnvnjxkq4pmjvsqkkqk2a6xh8ksn1fi2-qtwebkit-5.212-alpha-01-26-2018/include/Qt5WebKit

@jtojnar
Copy link
Member

@jtojnar jtojnar commented on be1b225 Jul 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Right, looks like qtModule sets setOutputFlags to false by default:

nixpkgs/pkgs/development/libraries/qt-5/qtModule.nix

Line 24 in ffbbdf2

setOutputFlags = args.setOutputFlags or false;

which we now respect. I guess we will need to revert this commit, or change it to use a separate setCmakeOutputFlags variable. I think the former makes more sense since CMake ignores unknown variables.

@vcunat
Copy link
Member

@vcunat vcunat commented on be1b225 Jul 4, 2020

Choose a reason for hiding this comment

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

Ah, I confused it the other way. So setOutputFlags = true; in pkgs/development/libraries/qt-5/modules/qtwebkit.nix would fix this one, but right now I can't estimate all other effects. I also wouldn't expect qmake and cmake being combined frequently.

@jtojnar
Copy link
Member

@jtojnar jtojnar commented on be1b225 Jul 4, 2020

Choose a reason for hiding this comment

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

I think this will be problem with every package using qtModule (basically everything in https://github.com/NixOS/nixpkgs/tree/ffbbdf247a14808307ef558d71afca4ac364c795/pkgs/development/libraries/qt-5), since it also enables multiple outputs:

nixpkgs/pkgs/development/libraries/qt-5/qtModule.nix

Line 23 in ffbbdf2

outputs = args.outputs or [ "out" "dev" ];

I doubt anything uses both CMake and qmake simultaneously (if they did, they would not use our setup hooks either way).

We would either need to manually add setOutputFlags = true; to every qt module built using cmake. Actually, it looks like qtwebkit is the only one.

@vcunat
Copy link
Member

@vcunat vcunat commented on be1b225 Jul 4, 2020

Choose a reason for hiding this comment

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

I doubt anything uses both CMake and qmake simultaneously (if they did, they would not use our setup hooks either way).

qt5.qtwebkit has both in build inputs.

@jtojnar
Copy link
Member

@jtojnar jtojnar commented on be1b225 Jul 4, 2020

Choose a reason for hiding this comment

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

That is just inherited from qtModule I think.

@jtojnar
Copy link
Member

@jtojnar jtojnar commented on be1b225 Jul 4, 2020

Choose a reason for hiding this comment

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

Fixed in dee7377, will be reverted in #92298.

Please sign in to comment.