nixpkgs icon indicating copy to clipboard operation
nixpkgs copied to clipboard

python3Packages: Fix and document the removal of the local `.overrideAttrs` attribute

Open ShamrockLee opened this issue 1 year ago • 4 comments

This PR cleans up the leftover of #333670.

After #333670, buildPythonPackage and buildPythonApplication no longer apply lib.makeOverridable themselves and no longer provide .override and .overrideDerivation of the returning package.

For Python packages provided by Nixpkgs, the local .override attribute is almost always shadowed by the one provided by callPackage, and so does the .overrideDerivation attribute. I didn't find any use of the local override in Nixpkgs, but @trofi reported the evaluation failure of python3Packages.pythonCatchConflictsHook.tests.catches-conflict-multiple-chains due to the loss of the local overrideDerivation.

This PR uses lib.overrideDerivation instead of the overrideDerivation attribute to fix the evaluation of python3Packages.pythonCatchConflictsHook.tests.catches-conflict-multiple-chains and document the removal of the buildPython*-generated local overrideDerivation attribute in the release note.

Things done

  • Built on platform(s)
    • [X] 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
  • [X] Tested, as applicable:
  • [x] 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.11 Release Notes (or backporting 23.11 and 24.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
  • [x] Fits CONTRIBUTING.md.

Add a :+1: reaction to pull requests you find important.

ShamrockLee avatar Oct 21 '24 02:10 ShamrockLee

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review


x86_64-linux

:fast_forward: 1 package blacklisted:
  • nixos-install-tools

ShamrockLee avatar Oct 21 '24 17:10 ShamrockLee

The change fixes the eval failures for that attribute for me. Thank you!

But I'm afraid I found another eval failure:

$ nix-instantiate -A python3Packages.scikit-image.tests.all-tests
error: attribute 'override' missing
       at /tmp/z/nixpkgs/pkgs/development/python-modules/scikit-image/default.nix:160:19:
          159|     passthru.tests = {
          160|       all-tests = self.override { doCheck = true; };
             |                   ^
          161|     };

It bisects down to the same 58bfe741230877e32be930a581bd2f89980b40d5 buildPython*: Deprecate and remove (buildPython* { ... }).override. Worth fixing it as well?

trofi avatar Oct 21 '24 21:10 trofi

@trofi I'll fix it as well.

Would you mind sharing how you found those evaluation errors in <pkg>.tests? The nixpkgs-review PR (Mic92/nixpkgs-review#397) to examine them hasn't been merged yet.

ShamrockLee avatar Oct 22 '24 05:10 ShamrockLee

@trofi I'll fix it as well.

Would you mind sharing how you found those evaluation errors in <pkg>.tests? The nixpkgs-review PR (Mic92/nixpkgs-review#397) to examine them hasn't been merged yet.

I'm using this script: https://github.com/trofi/nixpkgs-overlays/blob/main/lib/all-attrs-iter.bash and run it as:

$ ./all-attrs-iter.bash -I nixpkgs=~/n --arg maxDepth 4 --arg verbose 3 --arg ignoreDrvAttrs false

~/n contains nixpkgs from staging branch. The caveat is that I have a bunch of patches against nixpkgs to skip other known eval failures.

trofi avatar Oct 22 '24 09:10 trofi

Tested the attribute evaluation against the current branch state. All good now. Thank you!

trofi avatar Oct 23 '24 12:10 trofi

@trofi, could you officially approve this PR using the GitHub Review? This would make the approval more visible.

ShamrockLee avatar Oct 27 '24 04:10 ShamrockLee

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-already-reviewed/2617/2063

nixos-discourse avatar Oct 27 '24 04:10 nixos-discourse

As the Nixpkgs 24.11 branch-off is made, this PR should be backported onto the release-24.11 branch once it is merged.

ShamrockLee avatar Nov 18 '24 20:11 ShamrockLee

Cc: @natsukagami

ShamrockLee avatar Nov 18 '24 20:11 ShamrockLee

@natsukium 👋

natsukagami avatar Nov 18 '24 21:11 natsukagami

@natsukium 👋

Oops! Sorry for bothering and thank you for the redirection.

ShamrockLee avatar Nov 19 '24 01:11 ShamrockLee

Successfully created backport PR for release-24.11:

  • #357196

github-actions[bot] avatar Nov 19 '24 08:11 github-actions[bot]

Git push to origin failed for release-24.11 with exitcode 1

github-actions[bot] avatar Nov 19 '24 08:11 github-actions[bot]