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.
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
Revert "cmake: only set output paths with multiple outputs" #92298
Conversation
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
Another case that got broken:
overall I expect it will be best if such larger multi-output changes get their own branch and Hydra jobset, as apparently it might be hard to stabilize that quickly. |
Perhaps we should include this in the current staging-next iteration already? (#91090) I suspect the current state there might still be too broken by this to merge into master. In that case I'd probably also add some security updates with low risk of regressions (say, nss + |
This reverts commit be1b225. The commit broke Qt modules using CMake because they disable setOutputFlags. There is no need to have these flags limited to multiple output derivations since it should just work. If it does not, it is a bug that should be fixed as per https://github.com/jtojnar/cmake-snips#assuming-cmake_install_dir-is-relative-path Likewise, having a variable to disable passing the flags is also unnecessary, since CMake, unlike some configure scripts, ignores unknown flags. And if a person does not like the values, they can just override them by passing the offending flag with a different value to cmakeFlags.
This reverts commit dee7377.
44dbf46
to
f38d3df
Compare
Rebased against |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do think so. And Hydra is almost idle ATM anyway.
Oh, GitHub still haven't fixed their merge attributions 🙄 |
Well, you merged it to staging-next, I merged staging-next to staging. |
Oh, I forgot about target branch here. Then this attribution was correct. |
Hmm, turns out the revert of "cmake: only set output paths with multiple outputs" also undid the fix for which #91277 was created in the first place. This is because the re-introduced emptying of |
Actually, it turns out the |
#93166 will attempt to fix this for good. |
The docdir flag needs to include `PROJECT_NAME` according to [GNU guidelines]. We are passing `-DCMAKE_INSTALL_DOCDIR=${!outputDoc}/share/doc/${shareDocName}` but `$shareDocName` was unset. The `multiple-outputs.sh` setup hook actually only defines `shareDocName` as a local variable so it was not available for cmake setup hook. Making it global would be of limited usability, since it primarily tries to extract the project name from configure script. Additionally, it would not be set because the setup hook defines `setOutputFlags=`, preventing the function defining `shareDocName` from running. And lastly, the function would not run for single-output derivations. Previously, we tried [not disabling `setOutputFlags`] and passing the directory flags only for multi-output derivations that do not disable `setOutputFlags` but that meant having two different branches of code, making it harder to check correctness. The multi-output one did in fact not work due to aforementioned undefined `shareDocName`. It also broke derivations that set `setOutputFlags=` like [`qtModule` function does] (probably because some Qt modules have configure scripts incompatible with `configureFlags` defined by `multiple-outputs.sh` setup hook). For that reason, it was [reverted], putting us back to start. Let’s try to extract the project name from CMake in the cmake setup hook. CMake has a `-L` flag for dumping variables but `PROJECT_NAME` did not seem to be among them when I tested, so I had to resort to parsing the `CMakeLists.txt` file. The extraction function is limited, it does not deal with * project name on different line from the `project(` command opening - that will just not get matched so we will fall back to using the derivation name * variable interpolation - we will just fall back to using derivation name when the extracted `project_name` contains a dollar character * multiple [`project`] commands - The command sets `PROJECT_NAME` variable anew with each call, so the last `project` call before `include(GNUInstallDirs)` command will be used when the included module would [cache the `CMAKE_INSTALL_DOCDIR` variable]. We will just take the first discovered `project` command for simplicity. Hopefully, there are not many projects that use multiple `project` calls before including `GNUInstallDirs`. In either case, we will have some subdirectory so the conflicts will be minimized. [GNU guidelines]: https://www.gnu.org/prep/standards/html_node/Directory-Variables.html#index-docdir [not disabling `setOutputFlags`]: NixOS@be1b225 [`qtModule` function does]: NixOS#12740 [reverted]: NixOS#92298 [`PROJECT_NAME`]: https://cmake.org/cmake/help/v3.18/variable/PROJECT_NAME.html [`project`]: https://cmake.org/cmake/help/v3.18/command/project.html [cache the `CMAKE_INSTALL_DOCDIR` variable]: https://github.com/Kitware/CMake/blob/92e30d576d66ac05254bba0f0ff7d655947beb0f/Modules/GNUInstallDirs.cmake#L298-L299
The docdir flag needs to include `PROJECT_NAME` according to [GNU guidelines]. We are passing `-DCMAKE_INSTALL_DOCDIR=${!outputDoc}/share/doc/${shareDocName}` but `$shareDocName` was unset. The `multiple-outputs.sh` setup hook actually only defines `shareDocName` as a local variable so it was not available for cmake setup hook. Making it global would be of limited usability, since it primarily tries to extract the project name from configure script. Additionally, it would not be set because the setup hook defines `setOutputFlags=`, preventing the function defining `shareDocName` from running. And lastly, the function would not run for single-output derivations. Previously, we tried [not disabling `setOutputFlags`] and passing the directory flags only for multi-output derivations that do not disable `setOutputFlags` but that meant having two different branches of code, making it harder to check correctness. The multi-output one did in fact not work due to aforementioned undefined `shareDocName`. It also broke derivations that set `setOutputFlags=` like [`qtModule` function does] (probably because some Qt modules have configure scripts incompatible with `configureFlags` defined by `multiple-outputs.sh` setup hook). For that reason, it was [reverted], putting us back to start. Let’s try to extract the project name from CMake in the cmake setup hook. CMake has a `-L` flag for dumping variables but `PROJECT_NAME` did not seem to be among them when I tested, so I had to resort to parsing the `CMakeLists.txt` file. The extraction function is limited, it does not deal with * project name on different line from the `project(` command opening - that will just not get matched so we will fall back to using the derivation name * variable interpolation - we will just fall back to using derivation name when the extracted `project_name` contains a dollar character * multiple [`project`] commands - The command sets `PROJECT_NAME` variable anew with each call, so the last `project` call before `include(GNUInstallDirs)` command will be used when the included module would [cache the `CMAKE_INSTALL_DOCDIR` variable]. We will just take the first discovered `project` command for simplicity. Hopefully, there are not many projects that use multiple `project` calls before including `GNUInstallDirs`. In either case, we will have some subdirectory so the conflicts will be minimized. [GNU guidelines]: https://www.gnu.org/prep/standards/html_node/Directory-Variables.html#index-docdir [not disabling `setOutputFlags`]: NixOS@be1b225 [`qtModule` function does]: NixOS#12740 [reverted]: NixOS#92298 [`PROJECT_NAME`]: https://cmake.org/cmake/help/v3.18/variable/PROJECT_NAME.html [`project`]: https://cmake.org/cmake/help/v3.18/command/project.html [cache the `CMAKE_INSTALL_DOCDIR` variable]: https://github.com/Kitware/CMake/blob/92e30d576d66ac05254bba0f0ff7d655947beb0f/Modules/GNUInstallDirs.cmake#L298-L299
Motivation for this change
This reverts commit be1b225.
The commit broke Qt modules using CMake because they disable setOutputFlags.
There is no need to have these flags limited to multiple output derivations since it should just work. If it does not, it is a bug that should be fixed as per https://github.com/jtojnar/cmake-snips#assuming-cmake_install_dir-is-relative-path Likewise, having a variable to disable passing the flags is also unnecessary, since CMake, unlike some configure scripts, ignores unknown flags. And if a person does not like the values, they can just override them by passing the offending flag with a different value to cmakeFlags.
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)