Skip to content
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

Merged
merged 2 commits into from Jul 5, 2020

Conversation

jtojnar
Copy link
Contributor

@jtojnar jtojnar commented Jul 4, 2020

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
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@jtojnar jtojnar requested a review from ttuegel as a code owner July 4, 2020 22:19
jtojnar referenced this pull request Jul 4, 2020
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
@jtojnar jtojnar requested review from alyssais and vcunat July 4, 2020 22:30
@ofborg ofborg bot requested review from periklis and abbradar July 4, 2020 22:36
@vcunat
Copy link
Member

vcunat commented Jul 5, 2020

Another case that got broken:

CMake Warning at /nix/store/5ml1nh1zd6c2z5zqwnj0h43cyxfqffrg-opencv-4.3.0/lib/cmake/opencv4/OpenCVConfig.cmake:116 (message):
  OpenCV: Include directory doesn't exist: '//include/opencv4'.  OpenCV

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.

@vcunat
Copy link
Member

vcunat commented Jul 5, 2020

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 + sqlite).

@jtojnar jtojnar requested a review from FRidh July 5, 2020 16:53
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.
@jtojnar
Copy link
Contributor Author

jtojnar commented Jul 5, 2020

Rebased against git merge-base staging-next staging so that the branch can be switched if we decide to go that way.

Copy link
Member

@vcunat vcunat left a 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.

@jtojnar jtojnar merged commit b01f391 into NixOS:staging Jul 5, 2020
@jtojnar jtojnar deleted the always-cmake-multiout branch July 5, 2020 21:49
@vcunat
Copy link
Member

vcunat commented Jul 5, 2020

Oh, GitHub still haven't fixed their merge attributions 🙄

@jtojnar
Copy link
Contributor Author

jtojnar commented Jul 5, 2020

Well, you merged it to staging-next, I merged staging-next to staging.

@vcunat
Copy link
Member

vcunat commented Jul 5, 2020

Oh, I forgot about target branch here. Then this attribution was correct.

@jtojnar
Copy link
Contributor Author

jtojnar commented Jul 15, 2020

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 setOutputFlags disables the function that actually sets shareDocName. And even if we remove setOutputFlags=, it will still not be set for single-output derivations.

@jtojnar
Copy link
Contributor Author

jtojnar commented Jul 15, 2020

Actually, it turns out the shareDocName was always a local variable so #91277 could not have fixed it.

@jtojnar
Copy link
Contributor Author

jtojnar commented Jul 15, 2020

#93166 will attempt to fix this for good.

jtojnar added a commit to jtojnar/nixpkgs that referenced this pull request Jul 16, 2020
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
rvem pushed a commit to serokell/nixpkgs that referenced this pull request Nov 8, 2022
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants