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

treewide: Make all usage of python2Packages explicit #61142

Open
wants to merge 1 commit into
base: master
from

Conversation

@adisbladis
Copy link
Member

commented May 8, 2019

Motivation for this change

This is in preparation for Python2 going EOL in 2020.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nix-review --run "nix-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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@grahamc

This comment has been minimized.

Copy link
Member

commented May 8, 2019

I mentioned this on IRC, but just so it isn't lost (and so other people can give opinions too):

I wonder if the expression being callPackaged should change from pythonPackages to python2Packages. I say this remembering back to several PRs now moving overrides from all-packages.nix in to the per-package expression.

@adisbladis suggested using pythonXPackages.callPackage, but I'm not familiar with that.

@volth

This comment has been minimized.

Copy link
Contributor

commented May 8, 2019

It may be better to do simple s/pythonPackages/python2Packages/ inside every of the affected some-package.nix (instead of callPackage-overriding at call-sites like all-packages.nix)

@ofborg ofborg bot added the 6.topic: xfce label May 8, 2019

@adisbladis

This comment has been minimized.

Copy link
Member Author

commented May 8, 2019

It may be better to do simple s/pythonPackages/python2Packages/ inside every of the affected some-package.nix (instead of callPackage-overriding at call-sites like all-packages.nix)

I think I'll rebase this on #61139 and use python2Packages.callPackage so we are guaranteed to get the correct python/pythonPackages attributes in every derivation.

@volth

This comment has been minimized.

Copy link
Contributor

commented May 8, 2019

I think I'll rebase this on #61139 and use python2Packages.callPackage so we are guaranteed to get the correct python/pythonPackages attributes in every derivation.

I meant moving towards removal of pythonPackages treewide (in all-packages.nix and in small .nix-files) keeping only python2Packages and python3Packages.

There is very little number of software which works well with both python2 and python3 and may leverage that polymorphism of python and pythonPackages (without version numbers) .
With deprecation of python2 they will be python3Packages-only.

pythonPackages (without version number) might have sense inside python-packages.nix but leaking it into the packages of software written only in python2 or python3 is questionable

@adisbladis

This comment has been minimized.

Copy link
Member Author

commented May 9, 2019

I meant moving towards removal of pythonPackages treewide (in all-packages.nix and in small .nix-files) keeping only python2Packages and python3Packages.

I'm with you on this one, that's exactly the end goal.

It may be better to do simple s/pythonPackages/python2Packages/ inside every of the affected some-package.nix (instead of callPackage-overriding at call-sites like all-packages.nix)

I disagree, it's too easy to accidentally mismatch python and pythonPackages, that's why I'm considering to change all
somePackage = callPackage ./path { pythonPackages = python2Packages; };
to
somePackage = python2Packages.callPackage ./path { };
so we are always consistent.

@volth

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

I disagree, it's too easy to accidentally mismatch python and pythonPackages, that's why I'm considering to change all
somePackage = callPackage ./path { pythonPackages = python2Packages; };
to
somePackage = python2Packages.callPackage ./path { };
so we are always consistent.

That keeps the polymorphic pythonPackages (or direct names of python packages as they are in the new scope) and left room for

somePackage = python2Packages.callPackage ./path { };
somePackageWithP3 = python3Packages.callPackage ./path { };

If there is no intension to make a package .nix-file polymorhic which works with both Pythons, it seems excessive

@edolstra

This comment has been minimized.

Copy link
Member

commented May 9, 2019

👎 on this approach since it adds a huge amount of clutter to all-packages.nix. Much better to change Nix expressions to take python2Packages as an argument.

@edolstra

This comment has been minimized.

Copy link
Member

commented May 9, 2019

I disagree, it's too easy to accidentally mismatch python and pythonPackages, that's why I'm considering to change all
somePackage = callPackage ./path { pythonPackages = python2Packages; };
to
somePackage = python2Packages.callPackage ./path { };
so we are always consistent.

That seems very ad hoc to me. Just because a package uses python doesn't mean I should have to use pythonPackages.callPackage to call it. I mean, suppose a package uses both Python and Perl, should I use pythonPackages.callPackage or perlPackages.callPackage to instantiate it? This doesn't look like a composable mechanism.

@adisbladis adisbladis force-pushed the adisbladis:pythonpackages-2 branch from a2b3fc0 to ed400ea May 9, 2019

@adisbladis

This comment has been minimized.

Copy link
Member Author

commented May 9, 2019

Fair enough. I've pushed a a commit with the nix expressions changed instead.

treewide: Make all usage of python2Packages explicit
This is in preparation for Python2 going EOL in 2020

@adisbladis adisbladis force-pushed the adisbladis:pythonpackages-2 branch from ed400ea to e23256d May 9, 2019

@adisbladis adisbladis requested a review from basvandijk as a code owner May 9, 2019

@adisbladis adisbladis requested a review from FRidh May 9, 2019

@FRidh

This comment has been minimized.

Copy link
Member

commented May 10, 2019

It's good that we're going to be explicit about which version is used, however, I don't think this is the right approach.

In the past many expressions were modified to use python2Packages to indicate they are not compatible with Python 3, with the idea that we may eventually set python = python3;.

The latter never happened and is unlikely to happen as well, however, by making the proposed change we may be taking a step backwards in identifying what packages can use Python 3.

Therefore, I think instead we should set python = python3; (in a temporary branch/job), identify what packages get broken by this, and use python2Packages for those.

@@ -1,4 +1,4 @@
{ stdenv, runtimeShell, lib, fetchurl, python, pythonPackages, unzip }:
{ stdenv, runtimeShell, lib, fetchurl, python, python2Packages, unzip }:

This comment has been minimized.

Copy link
@volth

volth May 10, 2019

Contributor

maybe python -> python2 as well

@@ -10344,7 +10346,9 @@ in
libXpm = null;
};

gdal = callPackage ../development/libraries/gdal { };
gdal = callPackage ../development/libraries/gdal {

This comment has been minimized.

Copy link
@volth

volth May 10, 2019

Contributor

this remains from the first attempt

grib-api = callPackage ../development/libraries/grib-api { };
grib-api = callPackage ../development/libraries/grib-api {
pythonPackages = python2Packages;
};

This comment has been minimized.

Copy link
@volth

volth May 10, 2019

Contributor

this remains from the first attempt too

@@ -12330,6 +12338,7 @@ in
opencv4 = callPackage ../development/libraries/opencv/4.x.nix {
inherit (darwin) cf-private;
inherit (darwin.apple_sdk.frameworks) AVFoundation Cocoa QTKit VideoDecodeAcceleration;
pythonPackages = python2Packages;

This comment has been minimized.

Copy link
@volth

volth May 10, 2019

Contributor

this remains from the first attempt

@volth

This comment has been minimized.

Copy link
Contributor

commented May 10, 2019

Also, since recently we have python2Packages.python == python2 and python2.pkgs == python2Packages (same for python3 and perl), so probably using two arguments python and pythonPackages are not needed and even dangerous, as one of them might be overriden at call-site resulting it version mismatch;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.