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

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

Merged
merged 1 commit into from
May 11, 2021

Conversation

vcunat
Copy link
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
10 tasks
@vcunat
Copy link
Member Author

vcunat commented May 11, 2021

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.

@vcunat
Copy link
Member Author

vcunat commented May 11, 2021

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
Member Author

vcunat commented May 11, 2021

@GrahamcOfBorg build ruby

@r-rmcgibbo
Copy link

r-rmcgibbo commented May 11, 2021

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
10 tasks
@vcunat
Copy link
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
Member

@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
Member Author

vcunat commented May 11, 2021

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
Member

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

@Ericson2314
Copy link
Member

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

@vcunat
Copy link
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
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
12 tasks
roberth added a commit to hercules-ci/nixpkgs that referenced this pull request Feb 27, 2023
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.
alyssais pushed a commit that referenced this pull request Feb 28, 2023
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants