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

root: fix build #218537

Closed
wants to merge 1 commit into from
Closed

root: fix build #218537

wants to merge 1 commit into from

Conversation

alyssais
Copy link
Member

Description of changes

Broken by 5b2f597 ("lib.extendDerivation: Fix interaction between output selection and overrideAttrs"). cc @roberth in case this is an unintended consequence or there's some other fix we should be making.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.05 Release Notes (or backporting 22.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Needs llvm-config.

Fixes: 5b2f597 ("lib.extendDerivation: Fix interaction between output selection and overrideAttrs")
@roberth
Copy link
Member

roberth commented Feb 27, 2023

unintended consequence

With our surface area, every bug is potentially a feature.


Each version of llvm uses a hack to first select the out output, and then pretend that no output was selected. It was introduced for some compatibility reason.

# `llvm` historically had the binaries. When choosing an output explicitly,
# we need to reintroduce `outputSpecified` to get the expected behavior e.g. of lib.get*
llvm = tools.libllvm.out // { outputSpecified = false; };

The comment had to be reinterpreted once. A better comment would be:

llvm historically had the binaries. When choosing an output explicitly, we need to disable outputSpecified to get the expected behavior e.g. of lib.get*

Previously, overrideAttrs also had the effect of removing and therefore disabling the outputSpecified attribute, regardless of previous output selection operations. This implicit resetting behavior has been removed in 5b2f597 as you point out.

The workaround in the llvm expressions was made in

This PR introduces many other fixes, but the specific workaround may have been made by mistake, as it does not seem to have caused a significant change in evaluation behavior, when simplifying to just tools.libllvm; and just tools.libclang; a couple of lines down, neither ruby nor gnucash change (attributes taken from the discussion and ofborg diff resp.).

Removing the workaround does fix the behavior for root. It seems like a better fix, as it removes two workarounds instead of adding one. I'll make a PR so we can assess the impact with another ofborg diff.

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.
@roberth
Copy link
Member

roberth commented Feb 27, 2023

@alyssais
Copy link
Member Author

Closing in favour of #218551.

@alyssais alyssais closed this Feb 27, 2023
@alyssais alyssais deleted the root-llvm-dev branch February 27, 2023 10:07
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.
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.

2 participants