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

python: unsupported platforms check and buildPythonApplication support #128

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions explanations/python-unsupported-platforms.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
The python builder automatically sets `meta.platforms` to that of the interpreter, so the only reason to set it is to further limit the supported platforms to a subset of those supported by the python interpreter.
1 change: 1 addition & 0 deletions lib/default.nix
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ rec {
lib.genAttrs pythonPackageSetNames (pythonName:
prev.${pythonName}.override (oldOverrides: {
packageOverrides = lib.composeExtensions (oldOverrides.packageOverrides or idOverlay) (final: prev: {
buildPythonApplication = wrapFunctionWithChecks prev.buildPythonApplication check;
buildPythonPackage = wrapFunctionWithChecks prev.buildPythonPackage check;
});
})
Expand Down
13 changes: 10 additions & 3 deletions overlays/python-inconsistent-interpreters.nix
Original file line number Diff line number Diff line change
Expand Up @@ -9,21 +9,28 @@ let
checkInputs = drvArgs.checkInputs or [];
runtimeInputs = checkInputs ++ propagatedBuildInputs;
inputPythonInterpreters = map (p: p.pythonModule) (builtins.filter (p: p ? pythonModule) runtimeInputs);
allPythonInterpreters = lib.unique (inputPythonInterpreters ++ [ drv.pythonModule ]);
allPythonInterpreters = lib.unique (inputPythonInterpreters ++ lib.optionals (drv ? pythonModule) [ drv.pythonModule ]);
in
lib.singleton {
name = "python-inconsistent-interpreters";
cond = builtins.length allPythonInterpreters > 1;
msg =
let
allPythonNames = map (p: p.name) allPythonInterpreters;
sources = [
"buildPythonPackage"
sources = if drv ? pythonModule
then [ "buildPythonPackage"
Copy link
Owner

Choose a reason for hiding this comment

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

What is the goal of including this in the error message?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not convinced the inclusion provides any value but I followed the existing pattern. Here's an example error message as it currently stands:

$ nix run ../nixpkgs-hammering hindsight
warning: Git tree '/home/user/nixpkgs-hammering' is dirty
error: build log of '/nix/store/z36zmlak3h7x5kxcg4mr3fm7pj6w0zbf-hindsight-2021.01.16' is not available
error: build log of '/nix/store/z36zmlak3h7x5kxcg4mr3fm7pj6w0zbf-hindsight-2021.01.16' is not available
When evaluating attribute ‘hindsight’:
warning: python-inconsistent-interpreters
Between buildPythonApplication and propagatedBuildInputs, this derivation seems
to be simultaneously trying to use packages from multiple different Python package sets.
Mixing package sets like this is not supported.

Detected Python packages:

- python3-3.8.7
- python-2.7.18

See: https://github.com/jtojnar/nixpkgs-hammering/blob/master/explanations/python-inconsistent-interpreters.md

Shall I remove "buildPythonPackage" and "buildPythonApplication" from these messages?

] ++ lib.optionals (builtins.any (p: p.pythonModule or drv.pythonModule != drv.pythonModule) propagatedBuildInputs) [
"propagatedBuildInputs"
] ++ lib.optionals (builtins.any (p: p.pythonModule or drv.pythonModule != drv.pythonModule) checkInputs) [
"checkInputs"
]
else [ "buildPythonApplication"
] ++ lib.optionals (builtins.length (lib.unique(map (p: p.pythonModule) propagatedBuildInputs)) >= 2) [
"propagatedBuildInputs"
] ++ lib.optionals (builtins.length (lib.unique(map (p: p.pythonModule) checkInputs)) >= 2) [
"checkInputs"
];

in ''
Between ${lib.concatStringsSep " and " sources}, this derivation seems
to be simultaneously trying to use packages from multiple different Python package sets.
Expand Down
30 changes: 30 additions & 0 deletions overlays/python-unsupported-platforms.nix
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
final: prev:
Copy link
Owner

Choose a reason for hiding this comment

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

In the future, we will probably want the same rule for other languages like Go and Rust. I wonder if we should name this rule e.g. language-builder-unsupported-platforms to allow easily extending it in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe but it seems like it could get messy trying to get the default platform of every builder in one overlay. Perhaps it would make more sense to abstract the shared logic into a checkDefaultPlatformsForBuilder method in lib?

let
inherit (prev) lib;
inherit (import ../lib { inherit lib; }) checkBuildPythonPackageFor;

checkDerivation = drvArgs: drv:
let
# We can access python with the `pythonModule` attribute of `buildPythonPackage`
# derivations, but it is not so simple with `buildPythonApplication`. Here we rely on the
# performance hack in nixpkgs/pkgs/development/interpreters/python/mk-python-derivation.nix
# which injects python into propagatedBuildInputs.
python = lib.lists.last (builtins.filter (p: p ? pythonVersion) drv.propagatedBuildInputs);
Copy link
Owner

Choose a reason for hiding this comment

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

I am not very confident about this. Maybe add a comment pointing to the source code so that we can easily update this when it breaks: https://github.com/NixOS/nixpkgs/blob/dbe1337750f324137ca2ba78f20a3be00e16481a/pkgs/development/interpreters/python/mk-python-derivation.nix#L155

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I had not seen the comments added by NixOS/nixpkgs#179318 which does seem to draw into question the long-term inclusion of python as an input.

drvPlatforms = drvArgs.meta.platforms or [ ];
unsupportedPlatforms = lib.lists.subtractLists python.meta.platforms drvPlatforms;
in lib.singleton {
name = "python-unsupported-platforms";
cond = (unsupportedPlatforms != [ ]) || (lib.lists.naturalSort drvPlatforms == lib.lists.naturalSort python.meta.platforms);
msg = if unsupportedPlatforms != [ ] then ''
The following meta.platforms are not supported by the ${python} interpreter:
[ ${builtins.toString unsupportedPlatforms} ]
'' else ''
The meta.platforms attribute is set to the same value by default.
'';
locations = [
(builtins.unsafeGetAttrPos "platforms" python.meta)
jtojnar marked this conversation as resolved.
Show resolved Hide resolved
(builtins.unsafeGetAttrPos "platforms" drvArgs.meta)
];
};
in
checkBuildPythonPackageFor checkDerivation final prev
15 changes: 15 additions & 0 deletions run-tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,19 @@ def __iter__(self):
],
)

yield make_test_rule(
'python-unsupported-platforms',
[
'platforms-all',
"build-python-application-platforms-all",
],
[
'platforms-linux',
'unspecified',
"build-python-application-platforms-linux",
]
)

yield make_test_rule(
"python-include-tests",
[
Expand All @@ -291,9 +304,11 @@ def __iter__(self):
[
"mixed-1",
"mixed-2",
"build-python-application-mixed",
],
[
"normal",
"build-python-application-normal",
],
)

Expand Down
1 change: 1 addition & 0 deletions tests/default.nix
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
python-imports-check-typo = pkgs.recurseIntoAttrs (pkgs.python3.pkgs.callPackage ./python-imports-check-typo { });
python-include-tests = pkgs.recurseIntoAttrs (pkgs.python3.pkgs.callPackage ./python-include-tests { });
python-inconsistent-interpreters = pkgs.recurseIntoAttrs (pkgs.python3.pkgs.callPackage ./python-inconsistent-interpreters { });
python-unsupported-platforms = pkgs.recurseIntoAttrs (pkgs.python3.pkgs.callPackage ./python-unsupported-platforms { });
stale-substitute = pkgs.recurseIntoAttrs (pkgs.callPackage ./stale-substitute { });
unclear-gpl = pkgs.recurseIntoAttrs (pkgs.callPackage ./unclear-gpl { });
unnecessary-parallel-building = pkgs.recurseIntoAttrs (pkgs.callPackage ./unnecessary-parallel-building { });
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
{ buildPythonApplication
, python37Packages
, python39Packages
}:

buildPythonApplication rec {
name = "package";

propagatedBuildInputs = [
python37Packages.numpy
python39Packages.scipy
];
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
{ buildPythonApplication
, numpy
, scipy
}:

buildPythonApplication rec {
name = "package";

propagatedBuildInputs = [
numpy
scipy
];
}
2 changes: 2 additions & 0 deletions tests/python-inconsistent-interpreters/default.nix
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@
# positive cases
mixed-1 = callPackage ./mixed-1.nix { };
mixed-2 = callPackage ./mixed-2.nix { };
build-python-application-mixed = callPackage ./build-python-application-mixed.nix { };

# negative cases
normal = callPackage ./normal.nix { };
build-python-application-normal = callPackage ./build-python-application-normal.nix { };
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
{ lib
, buildPythonApplication
}:

buildPythonApplication rec {
pname = "package";
version = "0.1.0";

meta = with lib; {
platforms = platforms.all;
};
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
{ lib
, buildPythonApplication
}:

buildPythonApplication rec {
pname = "package";
version = "0.1.0";

meta = with lib; {
platforms = platforms.linux;
};
}
13 changes: 13 additions & 0 deletions tests/python-unsupported-platforms/default.nix
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
{ callPackage
}:

{
# positive cases
platforms-all = callPackage ./platforms-all.nix { };
build-python-application-platforms-all = callPackage ./build-python-application-platforms-all.nix { };

# negative cases
platforms-linux = callPackage ./platforms-linux.nix { };
unspecified = callPackage ./unspecified.nix { };
build-python-application-platforms-linux = callPackage ./build-python-application-platforms-linux.nix { };
}
12 changes: 12 additions & 0 deletions tests/python-unsupported-platforms/platforms-all.nix
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
{ lib
, buildPythonPackage
}:

buildPythonPackage rec {
pname = "package";
version = "0.1.0";

meta = with lib; {
platforms = platforms.all;
};
}
12 changes: 12 additions & 0 deletions tests/python-unsupported-platforms/platforms-linux.nix
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
{ lib
, buildPythonPackage
}:

buildPythonPackage rec {
pname = "package";
version = "0.1.0";

meta = with lib; {
platforms = platforms.linux;
};
}
7 changes: 7 additions & 0 deletions tests/python-unsupported-platforms/unspecified.nix
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{ buildPythonPackage
}:

buildPythonPackage rec {
pname = "package";
version = "0.1.0";
}