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

lib.systems.extensions.sharedLibrary: do not throw #244118

Merged
1 commit merged into from Jul 20, 2023
Merged

lib.systems.extensions.sharedLibrary: do not throw #244118

1 commit merged into from Jul 20, 2023

Conversation

ghost
Copy link

@ghost ghost commented Jul 18, 2023

Description of changes

Because downstream code expects to use == on platform attrsets, we are unfortunately not able to throw a useful error message when the sharedLibrary attribute is accessed.

When users do a comparison like:

stdenv.hostPlatform == pkgsStatic.stdenv.hostPlatform

... in a situation where stdenv.hostPlatform.hasSharedLibraries, they expect this to return false. Unfortunately Nix does a deep equality comparison here, and ends up forcing the pkgsStatic.stdenv.hostPlatform.extensions.sharedLibrary attribute, which throws the error.

Rather than returning null, this commit instead simply omits the extensions.sharedLibrary attribute. This provides the user with a more-useful error message: instead of waiting until the null is used (and hoping that produces an error), the user will get an error about the extensions.sharedLibrary attribute being missing, at the position where it was referenced.

Big thanks to @trofi for his PR to add NIX_VALIDATE_EVAL_NONDETERMINISM to Nix, which I am now using. It made tracking this down really easy!

Fixes #244045

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.11 Release Notes (or backporting 23.05 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.

Because downstream code expects to use `==` on platform attrsets, we
are unfortunately not able to throw a useful error message when the
`sharedLibrary` attribute is accessed.

When users do a comparison like:

  stdenv.hostPlatform == pkgsStatic.stdenv.hostPlatform

... in a situation where `stdenv.hostPlatform.hasSharedLibraries`,
they expect this to return `false`.  Unfortunately Nix does a deep
equality comparison here, and ends up forcing the
`pkgsStatic.stdenv.hostPlatform.extensions.sharedLibrary` attribute,
which throws the error.

Rather than returning `null`, this commit instead simply omits the
`extensions.sharedLibrary` attribute.  This provides the user with a
more-useful error message: instead of waiting until the `null` is
used (and hoping that produces an error), the user will get an error
about the `extensions.sharedLibrary` attribute being missing, at the
position where it was referenced.

Big thanks to @trofi for his PR to add
`NIX_VALIDATE_EVAL_NONDETERMINISM` to Nix, which I am now using.  It
made tracking this down really easy!

Fixes #244045
@trofi
Copy link
Contributor

trofi commented Jul 18, 2023

Another option is not to provide the attribute at all with a fancy computed assignment:

nix-repl> { ${if false then "attr" else null} = 42; }
{ }

nix-repl> { ${if true then "attr" else null} = 42; }
{ attr = 42; }

That way error messages attempting to use irrelevant attribute might be slightly clearer. But I did not test it. Might break more than it fixes.

@ghost ghost marked this pull request as draft July 18, 2023 06:29
@ghost
Copy link
Author

ghost commented Jul 18, 2023

Updated to implement @trofi's suggestion, but using lib.optionalAttrs instead of dynamic attributes.

No drv-hash change (or eval failure) with NIX_VALIDATE_EVAL_NONDETERMINISM=1 for all my usual builds (mips, powerpc, aarch64, x86_64) as well as tests.cross.sanity.

@ghost ghost marked this pull request as ready for review July 18, 2023 06:41
@vkryachko
Copy link

Any chance this could be reviewed? thanks

@ghost ghost merged commit 7363eed into NixOS:master Jul 20, 2023
9 checks passed
@ghost ghost deleted the pr/fix/244045 branch July 20, 2023 07:43
This pull request was closed.
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.

stdenv: assertion '(final).hasSharedLibraries' failed
2 participants