stdenv: fix backward multiple outputs conditional #91277
Conversation
|
Oh, fixing this might be affecting many packages and thus break some things due to the move... but I suppose we do want it anyway. |
This is supposed to shareDocName to a fallback value if it can't be determined from looking at the configure script. But the conditional checked whether shareDocName was set, rather than if it wasn't. This meant that if shareDocName had been detected from a configure script, it would be immediately overridden by the package name, and if it couldn't be detected, shareDocName would remain unset. This resulted in QEMU installing files like $out/share/doc/index.html, which should of course have been in $out/share/doc/qemu/index.html. An interesting side effect of this is that, since 9f87515 when this code was added, the detected package name has never actually been used for installing documentation, because it would always be overridden. So this patch will actually enable that for the first time, four years later. Fixes: NixOS#90486
|
Wow! Yeah let's give this a shot. |
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: NixOS#82304
This hack is no longer necessary, since multiple-outputs.sh has been fixed to install docs in the right location.
This reverts commit 730133e. multiple-outputs.sh has been fixed, so documentation is now corrrrectly installed under $out/share/doc/darktable. Fixes: NixOS#72160 Fixes: NixOS#83248
The risk of collisions is gone now because documentation is correctly installed into $out/share/doc/transmission instead of $out/share/doc as before.
|
I’ve done some additional work now to also fix this for CMake, which implemented its own version of this. One thing I’d like to understand before this is merged is the purpose of a714284, which I’ve essentially reverted here to allow the CMake setup hook to use the same variable as stdenv to disable the output options. Perhaps @ttuegel or @vcunat can shed some light on that? I’m also a bit unsure whether my choice not to set the CMake options for single-output derivations is the right one. This does match what we’ve always done in stdenv, but CMake has set them unconditionally for a long time. So feedback on that would be appreciated. Finally, I did a search through the codebase for packages that manually did something to share/doc, and fixed any packages I found that looked like they would be affected by this change, hence the last three commits. |
|
I believe some of the passed flags were unrecognized and failing the build. |
|
Do you remember any packages that were affected at the time that we
could test?
|
|
Eh, my bad, the |
Yeah, I do not understand that either. The
I do not see why that would be a problem other than masking broken packages that expect the paths are relative. But some people might consider it a benefit: #52859 (comment)
👍 |
ofborg
bot
requested review from
astsmtl,
wizeman,
mrVanDalo,
flosse,
viric and
cillianderoiste
|
Jan Tojnar <notifications@github.com> writes:
> I’ve done some additional work now to also fix this for CMake, which
> implemented its own version of this. One thing I’d like to understand
> before this is merged is the purpose of
> [a714284](a714284),
> which I’ve essentially reverted here to allow the CMake setup hook to
> use the same variable as stdenv to disable the output
> options. Perhaps @ttuegel or @vcunat can shed some light on that?
Yeah, I do not understand that either. The `setOutputFlags` only
disables setting `configureFlags` and `installFlags` (even
[then](https://github.com/NixOS/nixpkgs/blob/a714284d8b7d2dac3ed2c76670f290fe332da00c/pkgs/build-support/setup-hooks/multiple-outputs.sh#L47-L63)). CMake
does not use `configureFlags` so the only reason they could be
disabling it is to avoid setting `installFlags`. But CMake generates
regular `Makefile`s and `make` should not give a darn if
`pkgconfigdir`, `m4datadir` or `aclocaldir` is set.
Yeah, that's what I was thinking.
> I’m also a bit unsure whether my choice not to set the CMake options
> for single-output derivations is the right one. This does match what
> we’ve always done in stdenv, but CMake has set them unconditionally
> for a long time. So feedback on that would be appreciated.
I do not see why that would be a problem other than masking broken
packages that expect the paths are relative. But some people might
consider it a benefit:
#52859 (comment)
Cool, so you think the way I've done it is fine?
It is a shame about libdir still having to be set. I could not set that
too, and rely on the hook that moves lib64 to lib, but then there'd be
an extra symlink and packages that do things to lib in installPhase
would break, so I decided that was probably best always being set for
CMake.
|
I have a minor preference for keeping it always there because I like it when things break early and less variability is better for predictability but I understand Matthew’s sentiment. Especially when the breakage is not always loud and might be only discovered at runtime or during build of dependent packages. I will defer to you.
Yeah, I agree that it is always best to let the project know upfront rather than change paths behind its backs. If you wish, you can pass it just |
|
Okay, let’s try it out. I wouldn’t be surprised if there are unforeseen consequences that warrant a revert and a refinement to the approach, but I don’t think we’re going to be able to identify those up front. |
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
This is a very common path that often collides with other packages.
This is a very common path that often collides with other packages.
This is supposed to shareDocName to a fallback value if it can't be
determined from looking at the configure script. But the conditional
checked whether shareDocName was set, rather than if it wasn't. This
meant that if shareDocName had been detected from a configure script,
it would be immediately overridden by the package name, and if it
couldn't be detected, shareDocName would remain unset.
This resulted in QEMU installing files like $out/share/doc/index.html,
which should of course have been in $out/share/doc/qemu/index.html.
An interesting side effect of this is that, since
9f87515 when this code was added, the
detected package name has never actually been used for installing
documentation, because it would always be overridden. So this patch
will actually enable that for the first time, four years later.
We should probably consider, seeing as we’ve been getting by without it for four years, do we want the configure detection at all? I think we probably do, but it’s worth thinking about.