mkPythonDerivation: validate propagatedBuildInputs and buildInputs use a matching python #120220

grahamc · 2021-04-22T14:58:27Z

It isn't so difficult to accidentally mix and match Python packages from different pythons. This can cause fairly mysterious build time and run time errors. This PR adds a trace-only warning:

trace: warning: Warning: Python version mismatch in 'hi-bogus':

The Python derivation 'hi-bogus' depends on a Python derivation
named 'foooo', but the two derivations use different versions
of python:

    hi-bogus uses /nix/store/d44wd6n98f93hjr6q1d1phhh1hw7a17d-python3-3.8.8
       foooo uses /nix/store/pp0fp5zds7wqhyx6zqnnxl2aqzf4a51r-python-2.7.18

Suggestion: change hi-bogus's propagatedBuildInputs to use a 'foooo'
built from the same version of Python in /home/grahamc/projects/github.com/NixOS/nixpkgs/test.nix:5:3.

which will prevent these mistakes from entering Nixpkgs but only present as a nuisance for users outside of nixpkgs.

Open questions for me:

Should this refuse to build?
Should there be a way to turn it off for specific inputs?
Should this check apply to other inputs?

This has been made on behalf of Flox.

Things done

pkgs/development/interpreters/python/mk-python-derivation.nix

FRidh

Good idea!

pkgs/development/interpreters/python/mk-python-derivation.nix

FRidh · 2021-04-22T16:35:06Z

Should this refuse to build?

In principle I should say yes, because this will lead to mistakes. A potential, but bad case, is where one wants to rely on wrapPythonPrograms to add applications of different interpreters in propagatedBuildInputs to $PATH. Note this wouldn't detect it because pythonModule = false;; adding all propagatedBuildInputs to $PATH but its how it is now and out of scope here.

Should there be a way to turn it off for specific inputs?

Until someone reports a valid case where it is needed, I don't think we should.

Should this check apply to other inputs?

This is sufficient I think. nativeBuildInputs can indeed contain 2 and 3 mixed. Other cross inputs I think we can ignore.

FRidh · 2021-04-22T16:40:10Z

Should this refuse to build?

Note if it would block to build, it would need an entry in the changelog.

lukegb · 2021-04-22T16:40:13Z

nit: propogatedBuildInputs --> propagatedBuildInputs in the PR subject

grahamc · 2021-04-22T16:42:33Z

nit: propogatedBuildInputs --> propagatedBuildInputs in the PR subject

https://logs.nix.samueldr.com/nixos-dev/2021-04-20#4878138;

pkgs/development/interpreters/python/mk-python-derivation.nix

grahamc · 2021-04-22T17:45:05Z

This PR has an updated message, and uses throw now:

error: Python version mismatch in 'hi-bogus':

The Python derivation 'hi-bogus' depends on a Python derivation
named 'flake8', but the two derivations use different versions
of Python:

    'hi-bogus' uses /nix/store/d44wd6n98f93hjr6q1d1phhh1hw7a17d-python3-3.8.8
      'flake8' uses /nix/store/pp0fp5zds7wqhyx6zqnnxl2aqzf4a51r-python-2.7.18

Possible solutions:

  * If 'flake8' is a Python library, change the reference to 'flake8'
    in the propagatedBuildInputs of 'hi-bogus' to use a 'flake8' built from the same
    version of Python

  * If 'flake8' provides executables, move the reference to 'flake8' in
    'hi-bogus' from propagatedBuildInputs to nativeBuildInputs

 at /home/grahamc/projects/github.com/NixOS/nixpkgs/test.nix:5:3

(use '--show-trace' to show detailed location information)

grahamc · 2021-04-22T17:50:10Z

Note if it would block to build, it would need an entry in the changelog.

It now blocks the build, @FRidh. Which changelog do you mean? NixOS? Nixpkgs hasn't really had a changelog in years.

FRidh · 2021-04-22T18:09:42Z

It now blocks the build, @FRidh. Which changelog do you mean? NixOS? Nixpkgs hasn't really had a changelog in years.

Yes, NixOS, so it is add least findable without having to resort to git log and wade through all the merge commits.

If 'flake8' provides executables, move the reference to 'flake8' in
'hi-bogus' from propagatedBuildInputs to nativeBuildInputs

If 'flake8' is used as a tool during the build, move the reference to 'flake8' in
'hi-bogus' from propagatedBuildInputs to nativeBuildInputs
If 'flake8' provides executables that need to be available during run-time, pass makeWrapperArgs = [ "--prefix PATH : ${lib.makeBinPath [ flake8 ]}" ];

grahamc · 2021-04-22T18:49:00Z

If 'flake8' provides executables that need to be available during run-time, pass makeWrapperArgs = [ "--prefix PATH : ${lib.makeBinPath [ flake8 ]}" ];

This one is tricky, we don't actually know what the attribute name of the derivation named 'flake8', and I'm not sure how to word this otherwise.

FRidh · 2021-04-22T19:14:14Z

I noticed the same when working on error messages on mk-python-derivation.nix in #102613 (e.g. 2a1f172) and chose to just use the pname. To me that seems "good enough". Yes, it will be confusing at times, but in my opinion better than nothing. Otherwise replace the "flake8" there with "<your package>".

grahamc · 2021-04-22T19:16:29Z

How about an ofborg check that really wants pname == attrname

grahamc · 2021-04-22T19:22:34Z

Current error looks ilke:

error: Python version mismatch in 'hi-bogus':

The Python derivation 'hi-bogus' depends on a Python derivation
named 'flake8', but the two derivations use different versions
of Python:

    'hi-bogus' uses /nix/store/d44wd6n98f93hjr6q1d1phhh1hw7a17d-python3-3.8.8
      'flake8' uses /nix/store/pp0fp5zds7wqhyx6zqnnxl2aqzf4a51r-python-2.7.18

Possible solutions:

  * If 'flake8' is a Python library, change the reference to 'flake8'
    in the propagatedBuildInputs of 'hi-bogus' to use a 'flake8' built from the same
    version of Python

  * If 'flake8' is used as a tool during the build, move the reference to
    'flake8' in 'hi-bogus' from propagatedBuildInputs to nativeBuildInputs

  * If 'flake8' provides executables that are called at run time, pass its
    bin path to makeWrapperArgs:

        makeWrapperArgs = [ "--prefix PATH : ${lib.makeBinPath [ flake8 ] }" ];

 at /home/grahamc/projects/github.com/NixOS/nixpkgs/test.nix:6:3

FRidh · 2021-04-22T19:26:17Z

Current error looks ilke:

Looks great to me!

How about an ofborg check that really wants pname == attrname

That would also need to consider variants, which would then be forced to fix their pname, for example like

hdf5-mpi = appendToName "mpi" (hdf5.override { ...

That seems like a good idea.

A problematic case would be Python packages. Their pname is actually different; the function you're editing now prefixes pythonX.Y to its name. An option there is to make that pname for example pythonXYPackages.flake. But then how do we deal with a dot in a pname? This clearly could get complex.

grahamc · 2021-04-22T19:27:47Z

Agreed, I don't think it would be reasonable to ensure pname == attrname. It might be a useful lint as part of a "packaging health report", though... Not sure. Something like this would be advisory, not errory.

pkgs/development/interpreters/python/mk-python-derivation.nix

SuperSandro2000 · 2021-04-23T03:46:23Z

How about an ofborg check that really wants pname == attrname

Yes please. or nixpkgs-hammering.

lukegb · 2021-05-01T18:09:34Z

Any chance we can land this? The error looks good - I think we might want to file a followup issue against OfBorg for the pname==attrname check.

stale · 2021-10-30T17:09:13Z

I marked this as stale due to inactivity. → More info

RaitoBezarius · 2022-12-01T19:46:26Z

Can I do something to help get this merged?

Artturin · 2023-04-15T23:06:40Z

pkgs/development/interpreters/python/mk-python-derivation.nix

+        len = lib.max (lib.stringLength name) (lib.stringLength against);
+      in lib.strings.fixedWidthString len " " name;
+
+    drvName = drv: drv.pname or drv.name;


Either this should just be .name or both should use lib.getName, depends on if we want versions

error: Python version mismatch in 'xpybutil-0.0.6': The Python derivation 'xpybutil-0.0.6' depends on a Python derivation named 'xpybutil', but the two derivations use different versions of Python: 'xpybutil-0.0.6' uses /nix/store/fdqpyj613dr0v1l1lrzqhzay7sk4xg87-python3-3.10.10 'xpybutil' uses /nix/store/v7h4276ij0sj48qwrf4g4cm3pfc86mmw-python3-3.9.16 Possible solutions: * If 'xpybutil' is a Python library, change the reference to 'xpybutil' in the propagatedBuildInputs of 'xpybutil-0.0.6' to use a 'xpybutil' built from the same version of Python * If 'xpybutil' is used as a tool during the build, move the reference to 'xpybutil' in 'xpybutil-0.0.6' from propagatedBuildInputs to nativeBuildInputs * If 'xpybutil' provides executables that are called at run time, pass its bin path to makeWrapperArgs: makeWrapperArgs = [ "--prefix PATH : ${lib.makeBinPath [ xpybutil ] }" ];

diff --git a/pkgs/development/python-modules/xpybutil/default.nix b/pkgs/development/python-modules/xpybutil/default.nix index cc574c58241..9236df77d84 100644 --- a/pkgs/development/python-modules/xpybutil/default.nix +++ b/pkgs/development/python-modules/xpybutil/default.nix @@ -1,4 +1,4 @@ -{ lib, buildPythonPackage, fetchFromGitHub, xcffib, pillow }: +{ lib, buildPythonPackage, fetchFromGitHub, xcffib, pillow, python39 }: buildPythonPackage rec { pname = "xpybutil"; @@ -13,7 +13,7 @@ buildPythonPackage rec { }; # pillow is a dependency in image.py which is not listed in setup.py - propagatedBuildInputs = [ pillow xcffib ]; + propagatedBuildInputs = [ pillow xcffib python39.pkgs.xpybutil ]; propagatedNativeBuildInputs = [ xcffib ];

changed to .name and the suggestion below to lib.getName

FRidh

I have not tested this but the change looks good.

mkPythonDerivation: apply checks to buildInputs as well fixup: add a suggestion about nativeBuildInputs Clean up the wording, add reasons for the suggestions switch to throw Adjust error to clarify build-time vs. run-time

Artturin · 2023-04-28T15:09:35Z

error: Python version mismatch in 'python3.10-xpybutil-0.0.6':

The Python derivation 'python3.10-xpybutil-0.0.6' depends on a Python derivation
named 'python3.9-i3ipc-2.2.1', but the two derivations use different versions
of Python:

    'python3.10-xpybutil-0.0.6' uses /nix/store/fdqpyj613dr0v1l1lrzqhzay7sk4xg87-python3-3.10.10
        'python3.9-i3ipc-2.2.1' uses /nix/store/v7h4276ij0sj48qwrf4g4cm3pfc86mmw-python3-3.9.16

Possible solutions:

  * If 'python3.9-i3ipc-2.2.1' is a Python library, change the reference to i3ipc
    in the propagatedBuildInputs of 'python3.10-xpybutil-0.0.6' to use a i3ipc built from the same
    version of Python

  * If 'python3.9-i3ipc-2.2.1' is used as a tool during the build, move the reference to
    i3ipc in 'python3.10-xpybutil-0.0.6' from propagatedBuildInputs to nativeBuildInputs

  * If 'python3.9-i3ipc-2.2.1' provides executables that are called at run time, pass its
    bin path to makeWrapperArgs:

        makeWrapperArgs = [ "--prefix PATH : ${lib.makeBinPath [ i3ipc ] }" ];

grahamc · 2023-04-28T15:50:55Z

neat!

github-actions bot added the 6.topic: python label Apr 22, 2021

grahamc force-pushed the python-kvetch-on-mismatch branch 2 times, most recently from 384f1c1 to b9c8d5b Compare April 22, 2021 15:48

ofborg bot added 10.rebuild-darwin: 0 10.rebuild-linux: 0 labels Apr 22, 2021

grahamc changed the title ~~mkPythonDerivation: validate propogatedBuildInputs use a matching python~~ mkPythonDerivation: validate propogatedBuildInputs and buildInputs use a matching python Apr 22, 2021

cole-h reviewed Apr 22, 2021

View reviewed changes

pkgs/development/interpreters/python/mk-python-derivation.nix Outdated Show resolved Hide resolved

grahamc force-pushed the python-kvetch-on-mismatch branch 2 times, most recently from a13e85d to 85b19a5 Compare April 22, 2021 16:17

grahamc marked this pull request as ready for review April 22, 2021 16:17

grahamc requested a review from FRidh as a code owner April 22, 2021 16:17

grahamc commented Apr 22, 2021

View reviewed changes

pkgs/development/interpreters/python/mk-python-derivation.nix Outdated Show resolved Hide resolved

grahamc force-pushed the python-kvetch-on-mismatch branch from c580db8 to 5c4b982 Compare April 22, 2021 16:24

FRidh reviewed Apr 22, 2021

View reviewed changes

pkgs/development/interpreters/python/mk-python-derivation.nix Show resolved Hide resolved

grahamc changed the title ~~mkPythonDerivation: validate propogatedBuildInputs and buildInputs use a matching python~~ mkPythonDerivation: validate propagatedBuildInputs and buildInputs use a matching python Apr 22, 2021

grahamc force-pushed the python-kvetch-on-mismatch branch from 5c4b982 to 62f3120 Compare April 22, 2021 16:45

cole-h reviewed Apr 22, 2021

View reviewed changes

pkgs/development/interpreters/python/mk-python-derivation.nix Show resolved Hide resolved

pkgs/development/interpreters/python/mk-python-derivation.nix Outdated Show resolved Hide resolved

veprbl reviewed Apr 22, 2021

View reviewed changes

pkgs/development/interpreters/python/mk-python-derivation.nix Outdated Show resolved Hide resolved

FRidh added this to the 21.05 milestone Apr 23, 2021

stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Oct 30, 2021

stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Dec 1, 2022

Artturin modified the milestones: 21.05, 23.05 Dec 31, 2022

Artturin force-pushed the python-kvetch-on-mismatch branch from 11a5f56 to 4de2076 Compare April 15, 2023 23:04

Artturin reviewed Apr 15, 2023

View reviewed changes

Artturin force-pushed the python-kvetch-on-mismatch branch from 4de2076 to 6284642 Compare April 15, 2023 23:08

Artturin requested review from Artturin, FRidh, veprbl and cole-h April 25, 2023 13:44

Artturin force-pushed the python-kvetch-on-mismatch branch from 6284642 to 8ce82e3 Compare April 25, 2023 13:49

FRidh approved these changes Apr 25, 2023

View reviewed changes

Artturin force-pushed the python-kvetch-on-mismatch branch from 8ce82e3 to d44390a Compare April 28, 2023 14:59

Artturin merged commit 44075d6 into NixOS:master Apr 28, 2023
16 checks passed

grahamc deleted the python-kvetch-on-mismatch branch April 28, 2023 15:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mkPythonDerivation: validate propagatedBuildInputs and buildInputs use a matching python #120220

mkPythonDerivation: validate propagatedBuildInputs and buildInputs use a matching python #120220

grahamc commented Apr 22, 2021 •

edited

FRidh left a comment

FRidh commented Apr 22, 2021 •

edited

FRidh commented Apr 22, 2021

lukegb commented Apr 22, 2021

grahamc commented Apr 22, 2021

grahamc commented Apr 22, 2021

grahamc commented Apr 22, 2021

FRidh commented Apr 22, 2021

grahamc commented Apr 22, 2021

FRidh commented Apr 22, 2021 •

edited

grahamc commented Apr 22, 2021

grahamc commented Apr 22, 2021

FRidh commented Apr 22, 2021

grahamc commented Apr 22, 2021

SuperSandro2000 commented Apr 23, 2021

lukegb commented May 1, 2021

stale bot commented Oct 30, 2021

RaitoBezarius commented Dec 1, 2022

Artturin Apr 15, 2023 •

edited

Artturin Apr 25, 2023

FRidh left a comment

Artturin commented Apr 28, 2023

grahamc commented Apr 28, 2023

mkPythonDerivation: validate propagatedBuildInputs and buildInputs use a matching python #120220

mkPythonDerivation: validate propagatedBuildInputs and buildInputs use a matching python #120220

Conversation

grahamc commented Apr 22, 2021 • edited

Things done

FRidh left a comment

Choose a reason for hiding this comment

FRidh commented Apr 22, 2021 • edited

FRidh commented Apr 22, 2021

lukegb commented Apr 22, 2021

grahamc commented Apr 22, 2021

grahamc commented Apr 22, 2021

grahamc commented Apr 22, 2021

FRidh commented Apr 22, 2021

grahamc commented Apr 22, 2021

FRidh commented Apr 22, 2021 • edited

grahamc commented Apr 22, 2021

grahamc commented Apr 22, 2021

FRidh commented Apr 22, 2021

grahamc commented Apr 22, 2021

SuperSandro2000 commented Apr 23, 2021

lukegb commented May 1, 2021

stale bot commented Oct 30, 2021

RaitoBezarius commented Dec 1, 2022

Artturin Apr 15, 2023 • edited

Choose a reason for hiding this comment

Artturin Apr 25, 2023

Choose a reason for hiding this comment

FRidh left a comment

Choose a reason for hiding this comment

Artturin commented Apr 28, 2023

grahamc commented Apr 28, 2023

grahamc commented Apr 22, 2021 •

edited

FRidh commented Apr 22, 2021 •

edited

FRidh commented Apr 22, 2021 •

edited

Artturin Apr 15, 2023 •

edited