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

Eval fixes from release-attrpaths-superset.nix optimization (part 2) #271123

Merged
merged 5 commits into from
Dec 1, 2023

Conversation

Mic92
Copy link
Member

@Mic92 Mic92 commented Nov 30, 2023

Description of changes

context #269356
Purpose is to reduce the size of the PR.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • 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/)
  • 24.05 Release Notes (or backporting 23.05 and 23.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.

Priorities

Add a 👍 reaction to pull requests you find important.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But please apply the two-line suggestion below.

pkgs/test/cuda/default.nix Outdated Show resolved Hide resolved
@infinisil infinisil merged commit 10bd922 into NixOS:master Dec 1, 2023
21 checks passed
@Mic92 Mic92 deleted the pr/release-outpaths- branch December 3, 2023 09:20
in if missingArgs == {} then makeOverridable f allArgs else abort error;
in if missingArgs == {}
then makeOverridable f allArgs
else throw "lib.customisation.callPackageWith: ${error}";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What was the motivation for this specific change? It makes it so nix-env -qa no longer fails and just silently drops the attrpath when querying pacakges that have missing arguments (and therefore ofborg and other CI relies on the previous behavior to catch missing argument errors during outpath calculation)

This still smells like a bug in nix-env regardless to me, so this bit in Nixpkgs probably doesn't need to be changed back, but I'm still trying to trace down in that code where/why this throw would be ignored rather than propagated as an eval error

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, this is intended behavior in Nix:

Throw an error message s. This usually aborts Nix expression
evaluation, but in nix-env -qa and other commands that try to
evaluate a set of derivations to get information about those
derivations, a derivation that throws an error is silently skipped
(which is not the case for abort).

https://nixos.org/manual/nix/stable/language/builtins.html#builtins-throw

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh damn yeah. Probably the entire reason this is in nix-env at all is because we historically haven't had anything like this PR, which would've prevented packages from not being evaluatable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Revert and eval fixes in #278777

infinisil added a commit to tweag/nixpkgs that referenced this pull request Jan 5, 2024
infinisil added a commit to tweag/nixpkgs that referenced this pull request Jan 7, 2024
Was broken in 9fddd79
but not caught due to NixOS#271123 (comment)
infinisil added a commit to tweag/nixpkgs that referenced this pull request Jan 7, 2024
This was not caught when the package was introduced because of
NixOS#271123 (comment)
infinisil added a commit to tweag/nixpkgs that referenced this pull request Jan 7, 2024
infinisil added a commit to tweag/nixpkgs that referenced this pull request Jan 7, 2024
infinisil added a commit to tweag/nixpkgs that referenced this pull request Jan 7, 2024
infinisil added a commit to tweag/nixpkgs that referenced this pull request Jan 7, 2024
infinisil added a commit to tweag/nixpkgs that referenced this pull request Jan 7, 2024
… false

Was broken since initialised in 9b2d4d2
but not noticed due to NixOS#271123 (comment)
infinisil added a commit to tweag/nixpkgs that referenced this pull request Jan 7, 2024
… false

Was broken since initialised in 9b2d4d2
but not noticed due to NixOS#271123 (comment)
SuperSandro2000 pushed a commit to SuperSandro2000/nixpkgs that referenced this pull request Jan 8, 2024
SuperSandro2000 pushed a commit to SuperSandro2000/nixpkgs that referenced this pull request Jan 8, 2024
SuperSandro2000 pushed a commit to SuperSandro2000/nixpkgs that referenced this pull request Jan 8, 2024
SuperSandro2000 pushed a commit to SuperSandro2000/nixpkgs that referenced this pull request Jan 8, 2024
SuperSandro2000 pushed a commit to SuperSandro2000/nixpkgs that referenced this pull request Jan 8, 2024
Was broken in 9fddd79
but not caught due to NixOS#271123 (comment)
SuperSandro2000 pushed a commit to SuperSandro2000/nixpkgs that referenced this pull request Jan 8, 2024
This was not caught when the package was introduced because of
NixOS#271123 (comment)
SuperSandro2000 pushed a commit to SuperSandro2000/nixpkgs that referenced this pull request Jan 8, 2024
SuperSandro2000 pushed a commit to SuperSandro2000/nixpkgs that referenced this pull request Jan 8, 2024
SuperSandro2000 pushed a commit to SuperSandro2000/nixpkgs that referenced this pull request Jan 8, 2024
SuperSandro2000 pushed a commit to SuperSandro2000/nixpkgs that referenced this pull request Jan 8, 2024
infinisil added a commit that referenced this pull request Jan 8, 2024
This very weirdly broke the channel evaluation: https://hydra.nixos.org/build/245871962/nixlog/1

It appears that this attribute is only evaluated by Hydra, _not_ by
ofborg. So this wouldn't have been detected by CI anyways in the PR that
introduced the problem: #276800.

However, due to #271123 (comment),
the channel only broke once that was fixed with #278777

Whether the fix is good, I don't know, but the failing-on-darwin attribute
doesn't exist anymore with this commit, making the tarball build succeed
again:

    nix-build pkgs/top-level/release.nix -A tarball
github-actions bot pushed a commit to arcnmx/nixpkgs-lib that referenced this pull request Jan 9, 2024
SuperSandro2000 pushed a commit to SuperSandro2000/nixpkgs that referenced this pull request Jan 9, 2024
Was broken in 9fddd79
but not caught due to NixOS#271123 (comment)
SuperSandro2000 pushed a commit to SuperSandro2000/nixpkgs that referenced this pull request Jan 9, 2024
This was not caught when the package was introduced because of
NixOS#271123 (comment)
SuperSandro2000 pushed a commit to SuperSandro2000/nixpkgs that referenced this pull request Jan 9, 2024
SuperSandro2000 pushed a commit to SuperSandro2000/nixpkgs that referenced this pull request Jan 9, 2024
SuperSandro2000 pushed a commit to SuperSandro2000/nixpkgs that referenced this pull request Jan 9, 2024
SuperSandro2000 pushed a commit to SuperSandro2000/nixpkgs that referenced this pull request Jan 9, 2024
SuperSandro2000 pushed a commit to SuperSandro2000/nixpkgs that referenced this pull request Jan 10, 2024
Was broken in 9fddd79
but not caught due to NixOS#271123 (comment)
SuperSandro2000 pushed a commit to SuperSandro2000/nixpkgs that referenced this pull request Jan 10, 2024
This was not caught when the package was introduced because of
NixOS#271123 (comment)
SuperSandro2000 pushed a commit to SuperSandro2000/nixpkgs that referenced this pull request Jan 10, 2024
SuperSandro2000 pushed a commit to SuperSandro2000/nixpkgs that referenced this pull request Jan 10, 2024
SuperSandro2000 pushed a commit to SuperSandro2000/nixpkgs that referenced this pull request Jan 10, 2024
SuperSandro2000 pushed a commit to SuperSandro2000/nixpkgs that referenced this pull request Jan 10, 2024
bryango pushed a commit to bryango/nixpkgs that referenced this pull request Jan 10, 2024
Was broken since introduced, but not caught due to
NixOS#271123 (comment)

(cherry picked from commit 6946071)
slam-bert pushed a commit to slam-bert/nixpkgs that referenced this pull request Jan 11, 2024
This very weirdly broke the channel evaluation: https://hydra.nixos.org/build/245871962/nixlog/1

It appears that this attribute is only evaluated by Hydra, _not_ by
ofborg. So this wouldn't have been detected by CI anyways in the PR that
introduced the problem: NixOS#276800.

However, due to NixOS#271123 (comment),
the channel only broke once that was fixed with NixOS#278777

Whether the fix is good, I don't know, but the failing-on-darwin attribute
doesn't exist anymore with this commit, making the tarball build succeed
again:

    nix-build pkgs/top-level/release.nix -A tarball
github-actions bot pushed a commit to nix-community/nixpkgs.lib that referenced this pull request Jan 14, 2024
github-actions bot pushed a commit to Mic92/nixpkgs that referenced this pull request Jan 14, 2024
This very weirdly broke the channel evaluation: https://hydra.nixos.org/build/245871962/nixlog/1

It appears that this attribute is only evaluated by Hydra, _not_ by
ofborg. So this wouldn't have been detected by CI anyways in the PR that
introduced the problem: NixOS#276800.

However, due to NixOS#271123 (comment),
the channel only broke once that was fixed with NixOS#278777

Whether the fix is good, I don't know, but the failing-on-darwin attribute
doesn't exist anymore with this commit, making the tarball build succeed
again:

    nix-build pkgs/top-level/release.nix -A tarball
flandweber pushed a commit to flandweber/nixpkgs that referenced this pull request Jan 21, 2024
This very weirdly broke the channel evaluation: https://hydra.nixos.org/build/245871962/nixlog/1

It appears that this attribute is only evaluated by Hydra, _not_ by
ofborg. So this wouldn't have been detected by CI anyways in the PR that
introduced the problem: NixOS#276800.

However, due to NixOS#271123 (comment),
the channel only broke once that was fixed with NixOS#278777

Whether the fix is good, I don't know, but the failing-on-darwin attribute
doesn't exist anymore with this commit, making the tarball build succeed
again:

    nix-build pkgs/top-level/release.nix -A tarball
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.

None yet

4 participants