vint: init at 0.3.11 #22497

andsild · 2017-02-06T15:16:17Z

Motivation for this change

New package

I tested using python35 and python36: nix-env -f $NIXPKGS --option build-use-chroot true --option use-binary-caches false -i vim-vint, nix-shell -I nixpkgs=. --pure -p vim-vint, and nix-build -A vim-vint && ./result/bin/vint

I made sure that the resulting executable worked on vimrc-files, e.g. vint ~/.vimrc --color

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
- Linux
Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
Tested execution of all binary files (usually in ./result/bin/)
Fits CONTRIBUTING.md.

andsild · 2017-02-06T17:00:11Z

For now I am not able to reproduce the travis error locally. Seems to be OSX related, or just pathlib not supporting long filenames. If anyone has feedback that would be great.

Meanwhile, I'll see if I can be creative enough to think of another solution..

joachifm · 2017-02-06T23:12:02Z

Note that if you're using nix-daemon, passing build-use-sandbox to nix-env probably won't do what you expect.

Mic92 · 2017-02-06T23:13:11Z

pkgs/top-level/python-packages.nix

@@ -26314,6 +26314,25 @@ in {
    };
  };

+  vim-vint = buildPythonPackage rec {


The linter looks like an executable not a python library to me. We usually do not put applications into python-packages.nix but into a dedicate file, which is referenced in all-packages.nix.

Okay. I thought it made sense, I also saw for example pylint being in python-packages.nix. But since the resulting binary isn't strictly related to python, I see why it should be a dedicated package. I can try that 👍

Mic92 · 2017-02-06T23:23:11Z

It looks like the temporary path chosen to build this library adds a prefix, which exceeds the character limit of OSX for unix socket paths. One option could be to ignore this test conditionally on OSX by renaming the test methods: ex.: test_is_socket_true -> skip_test_is_socket_true.

andsild · 2017-02-11T12:41:58Z

@Mic92
I couldnt just rename the tests since they were not a part of the vim-vint package (unless I modified pathlib in python-packages.nix, which maybe is permissible?). So instead, I used python3 which has pathlib as part of the standard library.

Now I got some new errors for OSX: SSL: CERTIFICATE_VERIFY_FAILED. This seems to be related to NixOS/nix#921. I amended this by putting in the dependencies as buildInputs.

FRidh · 2017-02-13T20:21:06Z

pkgs/development/tools/vim-vint/default.nix

+    sha256 = "cda7090116b3aea12bdfe8ed44e71ead739df4e7e0ad24e27bb87754dedb613b";
+  };
+
+  # OSX keeps getting error: `Download error on https://pypi.python.org/simple/coverage: [SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed (_ssl.c:720) -- Some packages may not be found!`


pip automatically attempts to download dependencies but we've blocked that. You should provide the dependencies via nix.

FRidh · 2017-02-13T20:21:36Z

pkgs/development/tools/vim-vint/default.nix

+  };
+
+  # OSX keeps getting error: `Download error on https://pypi.python.org/simple/coverage: [SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed (_ssl.c:720) -- Some packages may not be found!`
+  # To amend this, we use nix for the packages (instead of letting setup.py fetch them).


That's indeed how we do it. No need to put it here :)

FRidh · 2017-02-13T20:22:23Z

pkgs/development/tools/vim-vint/default.nix

+  patchPhase = stdenv.lib.optionalString stdenv.isDarwin ''
+    substituteInPlace setup.py --replace "load_requires_from_file('test-requirements.txt')" "[]"
+    '';
+  buildInputs = with python3Packages; stdenv.lib.optionals stdenv.isDarwin [ coverage pytest pytestcov ];


why are these test inputs only needed for Darwin? That seems incorrect to me, the tests should run on any platform.

I had it off for all, but in travis the OSX tests failed.

I can remove the stdenv.lib.optionalString anyway, since the linux builds also should run the tests using the libraries.

Thanks for the feedback, I'll look into all the comments.

An even better solution: realize that the pypi version of this pacakge (vim-vint) doesn't ship with tests, so I can just skip the buildinputs anyway, for as long as I do the patchPhase. Waiting for travis to verify that it still works...

I'm not sure, is it better to get this version of vim-vint from github, where they ship with a test folder containing unit tests?

FRidh · 2017-02-13T20:22:44Z

pkgs/development/tools/vim-vint/default.nix

@@ -0,0 +1,28 @@
+{ lib, pkgs, python3Packages, stdenv }:


never pass in pkgs, just pass in fetchurl.

FRidh · 2017-02-14T07:30:40Z

Packages without tests can fail to build/work without us knowing why or that it even happened. It's up to you as maintainer to choose. We prefer to have tests.

andsild · 2017-02-14T09:16:08Z

@FRidh I agree, its nice with tests. I'll change it to be github-based later today.

My reasoning to choose pypi was that its the same store/mirror as what pip uses. Therefore, if anyone had a problem with this nix package, they wouldnt have to speculate on differences between github-version and pypi version of vim-vint.

andsild · 2017-02-20T20:44:08Z

@FRidh I made fixes for the changes if you want to review

jagajaga · 2017-02-15T15:22:06Z

pkgs/development/tools/vim-vint/default.nix

+  '';
+
+  meta = with lib; {
+    description = "Fast and Highly Extensible Vim script Language Lint implemented by Python.";


Remove dot at the end please.

Mic92 reviewed Feb 6, 2017

View reviewed changes

andsild force-pushed the vint branch 6 times, most recently from 10aba8a to 6376322 Compare February 11, 2017 12:29

andsild force-pushed the vint branch from 6376322 to b8caada Compare February 11, 2017 13:23

vrthra added 0.kind: enhancement 8.has: package (new) labels Feb 11, 2017

FRidh added the 6.topic: python label Feb 13, 2017

FRidh requested changes Feb 13, 2017

View reviewed changes

andsild force-pushed the vint branch 2 times, most recently from 531215d to 6454218 Compare February 13, 2017 23:36

andsild force-pushed the vint branch 2 times, most recently from 9006cf3 to ff5e594 Compare February 15, 2017 09:58

jagajaga reviewed Feb 20, 2017

View reviewed changes

vint: init at 0.3.11

88b8f38

andsild force-pushed the vint branch from ff5e594 to 88b8f38 Compare February 21, 2017 11:48

jagajaga merged commit c3aa109 into NixOS:master Feb 21, 2017

andsild deleted the vint branch February 21, 2017 14:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

vint: init at 0.3.11 #22497

vint: init at 0.3.11 #22497

andsild commented Feb 6, 2017 •

edited

andsild commented Feb 6, 2017 •

edited

joachifm commented Feb 6, 2017

Mic92 Feb 6, 2017

andsild Feb 7, 2017

Mic92 commented Feb 6, 2017

andsild commented Feb 11, 2017 •

edited

FRidh Feb 13, 2017

FRidh Feb 13, 2017

FRidh Feb 13, 2017

andsild Feb 13, 2017

andsild Feb 13, 2017 •

edited

FRidh Feb 13, 2017

FRidh commented Feb 14, 2017

andsild commented Feb 14, 2017

andsild commented Feb 20, 2017

jagajaga Feb 15, 2017

andsild Feb 21, 2017

vint: init at 0.3.11 #22497

vint: init at 0.3.11 #22497

Conversation

andsild commented Feb 6, 2017 • edited

Motivation for this change

Things done

andsild commented Feb 6, 2017 • edited

joachifm commented Feb 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mic92 commented Feb 6, 2017

andsild commented Feb 11, 2017 • edited

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

andsild Feb 13, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FRidh commented Feb 14, 2017

andsild commented Feb 14, 2017

andsild commented Feb 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andsild commented Feb 6, 2017 •

edited

andsild commented Feb 6, 2017 •

edited

andsild commented Feb 11, 2017 •

edited

andsild Feb 13, 2017 •

edited