python.pkgs: msgpack changes and neovim update #34809

typetetris · 2018-02-10T11:24:54Z

Motivation for this change

provide pythonPackages.neovim at 0.2.1 so CheckHealth in nvim doesn't complain any more.
(And deoplete-clang2 works as expected with this.)

As pythonPackages.neovim at 0.2.1 needs msgpack >= 5.0.0, bump msgpack to 0.5.4

This changed the package name (in the python ecosystem) from msgpack-python to msgpack,
so provide msgpack-transitional which provides the python package msgpack under its
deprecated name msgpack-python and change referrers to depend on that instead.

Things done

Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-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 nox --run "nox-review wip"
- some, not all, as many failed on my because of async-timeout or libQt
Tested execution of all binary files (usually in ./result/bin/)
Fits CONTRIBUTING.md.

FRidh

Please check the contributing guidelines
- https://nixos.org/nixpkgs/manual/#chap-submitting-changes
- https://nixos.org/nixpkgs/manual/#contributing
Commit per logical change, so per expression/package
We are moving expressions out of python-packages.nix

This changed the package name (in the python ecosystem) from msgpack-python to msgpack,
so provide msgpack-transitional which provides the python package msgpack under its
deprecated name msgpack-python and change referrers to depend on that instead.

The PyPI name is changed, but the module name seems to be the same. No need to provide both then.

FRidh · 2018-02-10T11:27:02Z

pkgs/top-level/python-packages.nix

@@ -10552,12 +10552,29 @@ in {
  };

  msgpack = buildPythonPackage rec {


We are moving Python expressions out of pkgs/top-level/python-packages.nix into pkgs/development/python-modules/<module>/default.nix.

Please move the expression there, and call it from pkgs/top-level/python-packages.nix using callPackage ../development/python-modules/<package> { };.

FRidh · 2018-02-10T11:27:26Z

pkgs/top-level/python-packages.nix

+    name = "msgpack-${version}";
+    version = "0.5.4";
+
+    src = pkgs.fetchurl {


FRidh · 2018-02-10T11:27:32Z

pkgs/top-level/python-packages.nix

+      py.test
+    '';
+
+    buildInputs = with self; [ pytest ];


checkInputs

FRidh · 2018-02-10T11:27:50Z

pkgs/top-level/python-packages.nix

+    propagatedBuildInputs = with self; [ ];
+  };
+
+  msgpack-transitional = buildPythonPackage rec {


We are moving Python expressions out of pkgs/top-level/python-packages.nix into pkgs/development/python-modules/<module>/default.nix.

Please move the expression there, and call it from pkgs/top-level/python-packages.nix using callPackage ../development/python-modules/<package> { };.

FRidh · 2018-02-10T11:28:01Z

pkgs/top-level/python-packages.nix

@@ -20422,12 +20439,12 @@ EOF
  trollius = callPackage ../development/python-modules/trollius {};

  neovim = buildPythonPackage rec {


We are moving Python expressions out of pkgs/top-level/python-packages.nix into pkgs/development/python-modules/<module>/default.nix.

Please move the expression there, and call it from pkgs/top-level/python-packages.nix using callPackage ../development/python-modules/<package> { };.

dotlambda

Please remove name from all expressions and only provide pname and version

typetetris · 2018-02-10T11:41:49Z

The module name has changed. If you compare the downloaded tarballs for msgpack and msgpack-transitional you will see that they differ in module name and in the setup.py file. For msgpack-python it contains an additional TRANSITIONAL=true.

If I only had one of them either the other packages now depending on msgpack-transitional failed or neovim-0.2.1 failed.

typetetris · 2018-02-10T11:41:56Z

Will do the other changes.

FRidh · 2018-02-10T11:49:25Z

So, actually, the module name has not changed, but the package name has. And that name is relevant for setuptools and pip, sigh.

I suggest having a msgpack attribute, and a msgpack-python attribute (instead of msgpack-transitional).

msgpack = callPackage ...;

msgpack-python = super.msgpack.overridePythonAttrs {
  pname = "msgpack-python";
  postPatch = ''
    substituteInPlace setup.py --replace "TRANSITIONAL = False" "TRANSITIONAL = True"
  '';
};

typetetris · 2018-02-10T11:57:05Z

That is fine by me.

Why is that postPatch thing necessary? Shouldn't changing the pname result in the other tarball being downloaded and everything is fine for msgpack-python?

FRidh · 2018-02-10T12:01:37Z

Why is that postPatch thing necessary? Shouldn't changing the pname result in the other tarball being downloaded and everything is fine for msgpack-python?

If you want to download also the tarball, one would have to write src = oldAttrs.override {inherit pname; sha256 = "..."};. In this case, there is no need for it because the only change is this constant which we can easily modify ourselves.

typetetris · 2018-02-10T12:37:18Z

Made (hopefully) all the changes, please have a look, it it is ok.

Didn't know whom to bother with maintainership.

dotlambda · 2018-02-10T12:38:13Z

Please make one commit per package

dotlambda · 2018-02-10T12:39:53Z

pkgs/top-level/python-packages.nix

@@ -20422,12 +20414,12 @@ EOF
  trollius = callPackage ../development/python-modules/trollius {};

  neovim = buildPythonPackage rec {


Please move neovim out of python-packages.nix

dotlambda · 2018-02-10T12:40:04Z

pkgs/top-level/python-packages.nix

@@ -20422,12 +20414,12 @@ EOF
  trollius = callPackage ../development/python-modules/trollius {};

  neovim = buildPythonPackage rec {
-    version = "0.2.0";
+    version = "0.2.1";
    name = "neovim-${version}";


dotlambda · 2018-02-10T12:40:13Z

pkgs/top-level/python-packages.nix

@@ -20422,12 +20414,12 @@ EOF
  trollius = callPackage ../development/python-modules/trollius {};

  neovim = buildPythonPackage rec {
-    version = "0.2.0";
+    version = "0.2.1";
    name = "neovim-${version}";

    src = pkgs.fetchurl {


fetchPypi

typetetris · 2018-02-10T12:46:51Z

Sorry, totally forgot about neovim expression too.

Just to be sure:
You want me to clean my git history to have one commit per package?

Would the following structure be ok?

move msgpack expression out of python-packages
bump version of msgpack
add msgpack-python
bump version of neovim

dotlambda · 2018-02-10T12:51:53Z

Yes, that seems fine. However, remeber to name your commits e.g. pythonPackages.msgpack: rename to msgpack-python. See https://nixos.org/nixpkgs/manual/#chap-submitting-changes.

dotlambda · 2018-02-10T12:53:24Z

Actually, I think you should have the following commits:

bump neovim (and move it at the same time)
remame msgpack
add msgpack

typetetris · 2018-02-10T13:00:25Z

bump neovim first would be a breaking commit, as it needs the bumped version of msgpack.

dotlambda · 2018-02-10T13:01:26Z

Ah I see. Then do that at the end

FRidh · 2018-02-10T13:02:22Z

@typetetris your proposal is good.

typetetris · 2018-02-10T13:25:41Z

Sorry, now it is a bit of both, but hopefully the cleanest git history I could come up with. Will rework the neovim commit now and alter that in next few minutes.

dotlambda · 2018-02-10T13:27:24Z

pkgs/development/python-modules/msgpack/default.nix

+      sha256 = "13ckbs2qc4dww7fddnm9cw116j4spgxqab49ijmj6jr178ypwl80";
+    };
+
+    propagatedBuildInputs = [ ];


If it's empty, you can leave it out completely.

dotlambda · 2018-02-10T13:27:33Z

pkgs/development/python-modules/msgpack/default.nix

+buildPythonPackage rec {
+    pname = "msgpack";
+    version = "0.5.4";
+    name = "${pname}-${version}";


dotlambda · 2018-02-10T13:29:13Z

pkgs/development/python-modules/msgpack/default.nix

+      homepage = https://github.com/msgpack/msgpack-python;
+      description = "MessagePack serializer implementation for Python";
+      license = lib.licenses.asl20;
+      # maintainer =  ?? ;


How about adding yourself as maintainer? Btw, the attribute should be called maintainers.

Hmm, I don't use it myself. Just wanted to update the version to get the neovim python client going again at version 0.2.1. Isn't that a bit weak motivation to declare myself maintainer and actually I am just trying nixos at the moment. I don't know how long I will be around at nixos.

Well, if you're not sure, then better remove the line completely. But feel free to add yourself once you're convinced to stick with NixOS :)
At the very least, you're using it because you're using neovim. So a maintained package does benefit you (and others).

typetetris · 2018-02-10T13:42:18Z

There is a comment in the neovim nix expression, that we can't do the checks as they need pkgs.neovim which would create a circular dependency. Is it the same with checkInputs or could I add pkgs.neovim there and activate the tests?

typetetris · 2018-02-10T13:46:19Z

wait a moment, something odd happened ...

typetetris · 2018-02-10T13:50:18Z

So, done as far as I could.

Open points:

maintainers for msgpack
comment in pkgs/developent/python-modules/neovim/default.nix about tests and circular dependencies.

FRidh

puh hopefully my pr is acceptable now :)

Almost :)

FRidh · 2018-02-10T14:05:20Z

pkgs/development/python-modules/msgpack/default.nix

+}:
+
+buildPythonPackage rec {
+    pname = "msgpack";


2 space indentation

FRidh · 2018-02-10T14:05:33Z

pkgs/top-level/python-packages.nix

-    checkPhase = ''
-      py.test
+ msgpack-python = self.msgpack.overridePythonAttrs {
+    pname = "msgpack-python";


2 space indentation

FRidh · 2018-02-10T14:05:46Z

pkgs/development/python-modules/neovim/default.nix

+}:
+
+buildPythonPackage rec {
+    pname = "neovim";


2 space indentation

and refactored to reuse the nix expression of pythonPackages.msgpack

changed dependency msgpack-python to msgpack to make it build

typetetris · 2018-02-10T14:12:39Z

corrected indentation

typetetris requested a review from FRidh as a code owner February 10, 2018 11:24

FRidh requested changes Feb 10, 2018

View reviewed changes

dotlambda reviewed Feb 10, 2018

View reviewed changes

typetetris force-pushed the master branch from ab45637 to 1c8f180 Compare February 10, 2018 13:24

dotlambda reviewed Feb 10, 2018

View reviewed changes

typetetris force-pushed the master branch from 1c8f180 to 9b40a1d Compare February 10, 2018 13:45

pythonPackages.msgpack: renamed to msgpack-python

e285906

typetetris force-pushed the master branch from 9b40a1d to 3998eb3 Compare February 10, 2018 13:47

FRidh requested changes Feb 10, 2018

View reviewed changes

typetetris added 3 commits February 10, 2018 15:10

pythonPackages.msgpack: init at 0.5.4

b020aef

pythonPackages.msgpack-python: 0.4.7 -> 0.5.4

4193774

and refactored to reuse the nix expression of pythonPackages.msgpack

pythonPackages.neovim: 0.2.0 -> 0.2.1

230b640

changed dependency msgpack-python to msgpack to make it build

typetetris force-pushed the master branch from 3998eb3 to 230b640 Compare February 10, 2018 14:11

FRidh changed the title ~~bumped pythonPackages.msgpack, pythonPackages.neovim and created pythonPackages.msgpack-transitional~~ python.pkgs: msgpack changes and neovim update Feb 10, 2018

FRidh merged commit a4db058 into NixOS:master Feb 10, 2018

aneeshusa mentioned this pull request Feb 27, 2018

salt: 2017.7.2 -> 2017.7.4, fix msgpack dependency #35891

Merged

8 tasks

Janik-Haag added the 12. first-time contribution label Jun 12, 2023

		@@ -10552,12 +10552,29 @@ in {
		};

		msgpack = buildPythonPackage rec {

		@@ -20422,12 +20439,12 @@ EOF
		trollius = callPackage ../development/python-modules/trollius {};

		neovim = buildPythonPackage rec {

		@@ -20422,12 +20414,12 @@ EOF
		trollius = callPackage ../development/python-modules/trollius {};

		neovim = buildPythonPackage rec {

python.pkgs: msgpack changes and neovim update #34809

python.pkgs: msgpack changes and neovim update #34809

Conversation

typetetris commented Feb 10, 2018

Motivation for this change

Things done

FRidh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dotlambda left a comment

Choose a reason for hiding this comment

typetetris commented Feb 10, 2018

typetetris commented Feb 10, 2018

FRidh commented Feb 10, 2018 • edited

typetetris commented Feb 10, 2018

FRidh commented Feb 10, 2018

typetetris commented Feb 10, 2018

dotlambda commented Feb 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

typetetris commented Feb 10, 2018

dotlambda commented Feb 10, 2018

dotlambda commented Feb 10, 2018

typetetris commented Feb 10, 2018

dotlambda commented Feb 10, 2018

FRidh commented Feb 10, 2018

typetetris commented Feb 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

typetetris commented Feb 10, 2018

typetetris commented Feb 10, 2018

typetetris commented Feb 10, 2018

FRidh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

typetetris commented Feb 10, 2018

FRidh commented Feb 10, 2018 •

edited