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

stdenv: fix backward multiple outputs conditional #91277

Merged
merged 5 commits into from Jun 29, 2020
Merged

Conversation

@alyssais
Copy link
Member

alyssais commented Jun 22, 2020

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.

@alyssais alyssais requested a review from vcunat Jun 22, 2020
@alyssais alyssais requested a review from Ericson2314 as a code owner Jun 22, 2020
@vcunat
Copy link
Member

vcunat commented Jun 22, 2020

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.

@jtojnar
Copy link
Contributor

jtojnar commented Jun 22, 2020

Should close: #90486, and maybe also #82304, #72160 and #83248.

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: #90486
@Ericson2314
Copy link
Member

Ericson2314 commented Jun 23, 2020

Wow! Yeah let's give this a shot.

alyssais added 4 commits Jun 24, 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
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: #72160
Fixes: #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.
@alyssais alyssais force-pushed the alyssais:qemu branch from 34ddaa3 to f61a26f Jun 24, 2020
@alyssais
Copy link
Member Author

alyssais commented Jun 24, 2020

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.

@vcunat
Copy link
Member

vcunat commented Jun 24, 2020

I believe some of the passed flags were unrecognized and failing the build. Perhaps newer cmake fixed that in the meantime. At that time it was even happening in some rarer cases where configure scripts were generated upstream by too old autotools, but the question here is about cmake anyway.

@alyssais
Copy link
Member Author

alyssais commented Jun 24, 2020

@vcunat
Copy link
Member

vcunat commented Jun 24, 2020

Eh, my bad, the setOutputFlags part disables putting flags like --bindir=/foo/bar into configureFlags... that wouldn't work in cmake anyway, would it?

@jtojnar
Copy link
Contributor

jtojnar commented Jun 24, 2020

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?

Yeah, I do not understand that either. The setOutputFlags only disables setting configureFlags and installFlags (even then). CMake does not use configureFlags so the only reason they could be disabling it is to avoid setting installFlags. But CMake generates regular Makefiles and make should not give a darn if pkgconfigdir, m4datadir or aclocaldir is set.

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)

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.

👍

@alyssais
Copy link
Member Author

alyssais commented Jun 24, 2020

@jtojnar
Copy link
Contributor

jtojnar commented Jun 24, 2020

Cool, so you think the way I've done it is fine?

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.

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.

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 lib as a relative path when not building multiple outputs. But personally, I would not bother.

@alyssais
Copy link
Member Author

alyssais commented Jun 29, 2020

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.

@alyssais alyssais merged commit 3abe43e into NixOS:staging Jun 29, 2020
17 of 18 checks passed
17 of 18 checks passed
cmake, cmake.passthru.tests, ghostscript, ghostscript.passthru.tests, stdenv, stdenv.passthru.tests, transmission, transmission.passthru.tests on x86_64-darwin
Details
cmake.passthru.tests, ghostscript.passthru.tests, stdenv.passthru.tests, transmission.passthru.tests on x86_64-darwin No attempt
Details
Evaluation Performance Report Evaluator Performance Report
Details
cmake, cmake.passthru.tests, ghostscript, ghostscript.passthru.tests, stdenv, stdenv.passthru.tests, transmission, transmission.passthru.tests on aarch64-linux Success
Details
cmake, cmake.passthru.tests, ghostscript, ghostscript.passthru.tests, stdenv, stdenv.passthru.tests, transmission, transmission.passthru.tests on x86_64-linux Success
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="f61a26f"; rev="f61a26f6ae2024fdd21006223e2ebb93bdeea6fe"; } ./pkgs/t
Details
grahamcofborg-eval-lib-tests nix-build --arg pkgs import ./. {} ./lib/tests/release.nix
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="f61a26f"; rev="f61a26f6ae2024fdd21006223e2ebb93bdeea6fe"; } ./nixos/
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="f61a26f"; rev="f61a26f6ae2024fdd21006223e2ebb93bdeea6fe"; } ./nixos/
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="f61a26f"; rev="f61a26f6ae2024fdd21006223e2ebb93bdeea6fe"; } ./nixos/
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="f61a26f"; rev="f61a26f6ae2024fdd21006223e2ebb93bdeea6fe"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="f61a26f"; rev="f61a26f6ae2024fdd21006223e2ebb93bdeea6fe"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="f61a26f"; rev="f61a26f6ae2024fdd21006223e2ebb93bdeea6fe"; } ./pkgs/t
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@alyssais alyssais deleted the alyssais:qemu branch Jun 29, 2020
vcunat 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 referenced this pull request Jul 14, 2020
This is a very common path that often collides with other packages.
jtojnar referenced this pull request Jul 14, 2020
This is a very common path that often collides with other packages.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.