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

pythonPackages: Add aliases 🎉 #116367

Merged
merged 3 commits into from May 22, 2021
Merged

Conversation

SuperSandro2000
Copy link
Member

@SuperSandro2000 SuperSandro2000 commented Mar 15, 2021

Motivation for this change

Just removing or renaming packages is not nice so I thought we want aliases for python packages. With this PR I included one package I used for testing. Most of the python-aliases.nix is copied from aliases.nix. If this PR is merged I am going to move some aliases which are currently in python-packages. I am probably not renaming all the packages though.

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.

pkgs/top-level/python-aliases.nix Outdated Show resolved Hide resolved
pkgs/top-level/python-aliases.nix Outdated Show resolved Hide resolved
pkgs/top-level/python-aliases.nix Outdated Show resolved Hide resolved
pkgs/top-level/python-aliases.nix Outdated Show resolved Hide resolved
pkgs/development/interpreters/python/default.nix Outdated Show resolved Hide resolved
@r-rmcgibbo
Copy link

r-rmcgibbo commented Mar 15, 2021

Result of nixpkgs-review pr 116367 at d1a4de3 run on x86_64-linux 1

2 packages built successfully:
1 suggestion:
  • warning: unclear-gpl

    lgpl21 is a deprecated license, check if project uses lgpl21Plus or lgpl21Only and change meta.license accordingly.

    Near pkgs/development/python-modules/gensim/default.nix:40:5:

       |
    40 |     license = lib.licenses.lgpl21;
       |     ^
    

Result of nixpkgs-review pr 116367 at d1a4de3 run on aarch64-linux 1

2 packages built successfully:
1 suggestion:
  • warning: unclear-gpl

    lgpl21 is a deprecated license, check if project uses lgpl21Plus or lgpl21Only and change meta.license accordingly.

    Near pkgs/development/python-modules/gensim/default.nix:40:5:

       |
    40 |     license = lib.licenses.lgpl21;
       |     ^
    

@FRidh FRidh self-assigned this Mar 15, 2021
@SuperSandro2000 SuperSandro2000 added this to the 21.05 milestone Mar 17, 2021
@FRidh
Copy link
Member

FRidh commented Mar 19, 2021

NIce work!

diff --git a/pkgs/development/interpreters/python/default.nix b/pkgs/development/interpreters/python/default.nix
index 1925089921c..0e5ebfaa4ff 100644
--- a/pkgs/development/interpreters/python/default.nix
+++ b/pkgs/development/interpreters/python/default.nix
@@ -69,7 +69,7 @@ with pkgs;
               recursivePthLoader
             ;
           };
-          aliases = self: super: lib.optionalAttrs (config.allowAliases or true) (import ../../../top-level/python-aliases.nix lib super);
+          aliases = lib.optionalAttrs (config.allowAliases or true) (import ../../../top-level/python-aliases.nix lib);
         in lib.makeScopeWithSplicing
           pkgs.splicePackages
           pkgs.newScope
diff --git a/pkgs/top-level/python-aliases.nix b/pkgs/top-level/python-aliases.nix
index 10db7748fe9..76338061ff2 100644
--- a/pkgs/top-level/python-aliases.nix
+++ b/pkgs/top-level/python-aliases.nix
@@ -16,7 +16,7 @@ let
 
   # Make sure that we are not shadowing something from
   # python-packages.nix.
-  checkInPkgs = n: alias: if builtins.hasAttr n self
+  checkInPkgs = n: alias: if builtins.hasAttr n super
                           then throw "Alias ${n} is still in python-packages.nix"
                           else alias;

@sternenseemann
Copy link
Member

sternenseemann commented Mar 19, 2021

+          aliases = lib.optionalAttrs (config.allowAliases or true) (import ../../../top-level/python-aliases.nix lib);

This won't work, since aliases == {} if config.AllowAliases == false when it should be a function in that case.

@SuperSandro2000
Copy link
Member Author

SuperSandro2000 commented Mar 21, 2021

+          aliases = lib.optionalAttrs (config.allowAliases or true) (import ../../../top-level/python-aliases.nix lib);

This won't work, since aliases == {} if config.AllowAliases == false when it should be a function in that case.

What would be the correct syntax or does it not require any change at all?

@SuperSandro2000 SuperSandro2000 force-pushed the python-aliases branch 2 times, most recently from e271e3b to 97e003c Compare March 21, 2021 05:16
@SuperSandro2000
Copy link
Member Author

I don't fully understand why and how ofborg fails and I can't locally run the ofborg checks because I only have ~16 GB available.

let
# Removing recurseForDerivation prevents derivations of aliased attribute
# set to appear while listing all the packages available.
removeRecurseForDerivations = alias: removeAttrs alias [ "recurseForDerivations" ];
Copy link
Member

Choose a reason for hiding this comment

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

The eval error was caused by this, sorry about that. Somehow blacked out and forgot that pythonPackages also contains functions and such.

Suggested change
removeRecurseForDerivations = alias: removeAttrs alias [ "recurseForDerivations" ];
removeRecurseForDerivations = alias:
if lib.isAttrs alias # removeAttrs throws in all other cases
then builtins.removeAttrs alias [ "recurseForDerivations" ]
else alias;

Copy link
Member Author

Choose a reason for hiding this comment

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

I used the variant that is used in aliases.nix because I don't fully understand this part to be honest.

Copy link
Member

Choose a reason for hiding this comment

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

The code from top level removes the attribute recurseForDerivations from an attribute set if it exists and is true. My code tries to always remove the recurseForDerivations attribute if it encounters an attribute set.

Shouldn't make a derifference in the grand scheme of things and only matters if we have sub attribute sets in pythonPackages anyways.

pkgs/development/interpreters/python/default.nix Outdated Show resolved Hide resolved
@ofborg ofborg bot requested review from Helkafen, veprbl and renatoGarcia May 22, 2021 18:31
Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

diff LGTM

seems to work as expected:

[nix-shell:~/.cache/nixpkgs-review/pr-116367]$ nix-build ./nixpkgs/ -A python3Packages.smart_open
/nix/store/yrnw7sl5iaqb1xvyjbj0fmjny6lph37l-python3.8-smart-open-4.2.0

[nix-shell:~/.cache/nixpkgs-review/pr-116367]$ nix-build ./nixpkgs/ -A python3Packages.smart-open
/nix/store/yrnw7sl5iaqb1xvyjbj0fmjny6lph37l-python3.8-smart-open-4.2.0

[nix-shell:~/.cache/nixpkgs-review/pr-116367]$ nix-build -E 'with import ./nixpkgs { config.allowAliases = false; };  python3Packages.smart_open'
error: attribute 'smart_open' missing

       at «string»:1:58:

            1| with import ./nixpkgs { config.allowAliases = false; };  python3Packages.smart_open
             | 

[nix-shell:~/.cache/nixpkgs-review/pr-116367]$ nix-build -E 'with import ./nixpkgs { config.allowAliases = true; };  python3Packages.smart_open'
/nix/store/yrnw7sl5iaqb1xvyjbj0fmjny6lph37l-python3.8-smart-open-4.2.0

@jonringer jonringer merged commit 939c4c1 into NixOS:master May 22, 2021
@SuperSandro2000 SuperSandro2000 deleted the python-aliases branch May 22, 2021 22:03
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

5 participants