Skip to content

llvm*Packages: fix output selection (lib.get*)#122554

Merged
vcunat merged 1 commit intoNixOS:staging-nextfrom
vcunat:p/llvm-outputUnspecified
May 11, 2021
Merged

llvm*Packages: fix output selection (lib.get*)#122554
vcunat merged 1 commit intoNixOS:staging-nextfrom
vcunat:p/llvm-outputUnspecified

Conversation

@vcunat
Copy link
Copy Markdown
Member

@vcunat vcunat commented May 11, 2021

Motivation for this change

#120058 (comment)

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.

@vcunat vcunat requested a review from matthewbauer as a code owner May 11, 2021 08:56
@vcunat vcunat mentioned this pull request May 11, 2021
Merged
10 tasks
@vcunat
Copy link
Copy Markdown
Member Author

vcunat commented May 11, 2021 •
edited
Loading

It's fallout from PR #111487.

Well, maybe the old generic logic around lib.get* could be improved, but I would certainly avoid trying to do that for 21.05. If I remember right, this arose once upon a time, when multiple outputs were being introduced. There was a desire for nix-env (and similar others like buildEnv) to choose some outputs for each package unless the user explicitly specified they want a particular output. And that's how we ended up with the output selections everywhere working like this: if you select output once, further calls to lib.get* won't change that choice.

@ofborg ofborg bot added the 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild on Darwin and must target a staging branch. label May 11, 2021
@ofborg ofborg bot requested review from 7c6f434c, dtzWill, lovek323 and primeos May 11, 2021 09:06
@ofborg ofborg bot added 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 2501-5000 This PR causes many rebuilds on Linux and should target the staging branches. labels May 11, 2021
@vcunat vcunat requested a review from Ericson2314 May 11, 2021 09:07
@vcunat
Copy link
Copy Markdown
Member Author

vcunat commented May 11, 2021 •
edited
Loading

Perhaps my mind is flakey. It might also have purpose in buildInputs (.dev by default unless you select something explicitly) and elsewhere. All the more reason not to change it hastily.

@vcunat
Copy link
Copy Markdown
Member Author

vcunat commented May 11, 2021

@GrahamcOfBorg build ruby

@r-rmcgibbo
Copy link
Copy Markdown

r-rmcgibbo commented May 11, 2021 •
edited
Loading

Result of nixpkgs-review pr 122554 at 6b3b794 run on aarch64-linux 1

465 packages marked as broken and skipped:
  • agdaPackages.iowa-stdlib
  • aldor
  • bareos
  • belle-sip
  • betaflight-configurator
  • bless
  • cassandra_2_1
  • cassandra_2_2
  • clang-sierraHack
  • clang-sierraHack-stdenv
  • ...
8494 packages skipped due to time constraints:
  • AusweisApp2
  • DisnixWebService
  • EBTKS
  • R
  • abcl
  • accountsservice
  • acsccid
  • adapta-gtk-theme
  • adoptopenjdk-icedtea-web
  • aerc
  • ...
2 packages built successfully:
  • clang (clang_7 ,llvmPackages.clang ,llvmPackages.libstdcxxClang ,llvmPackages_7.clang ,llvmPackages_7.libstdcxxClang)
  • haskell.compiler.ghc8102BinaryMinimal

Result of nixpkgs-review pr 122554 at 6b3b794 run on x86_64-linux 1

409 packages marked as broken and skipped:
  • aldor
  • atom-beta
  • cassandra_2_1
  • cassandra_2_2
  • clang-sierraHack
  • clang-sierraHack-stdenv
  • darwin.binutils
  • darwin.binutils-unwrapped
  • darwin.binutilsNoLibc
  • darwin.cctools
  • ...
2055 packages skipped due to time constraints:
  • AusweisApp2
  • DisnixWebService
  • R
  • Sylk
  • _1password-gui
  • abcl
  • acsccid
  • adapta-gtk-theme
  • adoptopenjdk-icedtea-web
  • aeon
  • ...
31 packages built successfully:
  • accountsservice
  • clightd
  • colord
  • colord-gtk
  • gcr (gnome.gcr)
  • gnome.gdm
  • gnome2.GConf
  • gnome2.gnome_vfs
  • gvfs (gnome2.gvfs)
  • gnupg (gnupg22)
  • gpgme
  • jsawk
  • libblockdev
  • libjcat
  • libnma
  • libsForQt5.polkit-qt (libsForQt515.polkit-qt ,plasma5Packages.polkit-qt)
  • libsForQt514.polkit-qt
  • lxqt.liblxqt
  • modemmanager
  • networkmanager
  • pantheon.elementary-default-settings
  • pantheon.switchboard-plug-security-privacy
  • pcsclite
  • plowshare
  • polkit
  • python38Packages.xapp
  • python39Packages.xapp
  • spidermonkey_78
  • sysprof
  • udisks (udisks2)
  • volume_key

@vcunat vcunat mentioned this pull request May 11, 2021
Merged
10 tasks
@vcunat
Copy link
Copy Markdown
Member Author

vcunat commented May 11, 2021

Hmm, no luck with verifying ruby on OfBorg – it timed out. I expect the change also caused rebuild of some of its deeper dependencies.

@Ericson2314
Copy link
Copy Markdown
Member

Ericson2314 commented May 11, 2021

@vcunat I think buildInputs's lib.getDev is going to be causing the mass rebuild. But clever idea.

It might also help to do out and bin instead of out and lib: i.e. just make it a library by default.

@vcunat
Copy link
Copy Markdown
Member Author

vcunat commented May 11, 2021 •
edited
Loading

You could, but so far our general convention is to choose the output with binaries as the default one; IIRC only glibc was an exception 😄

@Ericson2314
Copy link
Copy Markdown
Member

Ericson2314 commented May 11, 2021

@vcunat well, I defer to you on that as original author of multiple outputs in Nixpkgs :).

@Ericson2314
Copy link
Copy Markdown
Member

Ericson2314 commented May 11, 2021

@vcunat anyways I think more dev will be harmless because it will propagate the other inputs.

@vcunat
Copy link
Copy Markdown
Member Author

vcunat commented May 11, 2021

I clearly managed to forget a lot of the context since then, so I'm probably not really special in this respect (anymore). I suppose we'll try it then; perhaps I'll wait a bit in case someone else wants to have a look, too.

@vcunat vcunat merged commit ec97081 into NixOS:staging-next May 11, 2021
@vcunat vcunat deleted the p/llvm-outputUnspecified branch May 11, 2021 20:09
@vcunat
Copy link
Copy Markdown
Member Author

vcunat commented May 11, 2021

... or not 😄 I'm feeling optimistic and Hydra would otherwise be quite idle now. It's just staging-next, so in the worst case we can change it again.

@roberth roberth mentioned this pull request Feb 27, 2023
Closed
12 tasks
roberth added a commit to hercules-ci/nixpkgs that referenced this pull request Feb 27, 2023
@roberth
llvm*: Remove outputSpecified workaround where possible
c88f634 
The effect of `.out // { outputSpecified = false; }` in these cases
is to select the default output explicitly, but then make the
selection implicit until `overrideAttrs` is called. Previously
`overrideAttrs` would not preserve output selection, masking the
apparently unnecessary behavior of this workaround.

For `libllvm-polly`, this logic does not apply, as it does not
select the default output.

The `outputSpecified` workaround was introduced in
NixOS#122554

and was perhaps rushed because of a release deadline, and expected
delays from mass rebuilds.

The change in `overrideAttrs` behavior was added in
5b2f597 / NixOS#211685

and the problem was discovered in NixOS#218537,
which may contain further information.
@roberth roberth mentioned this pull request Feb 27, 2023
Merged
12 tasks
alyssais pushed a commit that referenced this pull request Feb 28, 2023
@roberth @alyssais
llvm*: Remove outputSpecified workaround where possible
359a46e 
The effect of `.out // { outputSpecified = false; }` in these cases
is to select the default output explicitly, but then make the
selection implicit until `overrideAttrs` is called. Previously
`overrideAttrs` would not preserve output selection, masking the
apparently unnecessary behavior of this workaround.

For `libllvm-polly`, this logic does not apply, as it does not
select the default output.

The `outputSpecified` workaround was introduced in
#122554

and was perhaps rushed because of a release deadline, and expected
delays from mass rebuilds.

The change in `overrideAttrs` behavior was added in
5b2f597 / #211685

and the problem was discovered in #218537,
which may contain further information.
@rrbutani rrbutani added the 6.topic: llvm/clang Issues related to llvmPackages, clangStdenv and related label May 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Ericson2314 Ericson2314 Ericson2314 approved these changes

@matthewbauer matthewbauer Awaiting requested review from matthewbauer

@7c6f434c 7c6f434c Awaiting requested review from 7c6f434c

@primeos primeos Awaiting requested review from primeos

@dtzWill dtzWill Awaiting requested review from dtzWill

@lovek323 lovek323 Awaiting requested review from lovek323

Assignees

No one assigned

Labels

6.topic: llvm/clang Issues related to llvmPackages, clangStdenv and related 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild on Darwin and must target a staging branch. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 2501-5000 This PR causes many rebuilds on Linux and should target the staging branches.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@vcunat @r-rmcgibbo @Ericson2314 @rrbutani