pylode: init at 2.8.6 #95863

koslambrou · 2020-08-20T15:28:01Z

Motivation for this change

added new package: pylode at version 2.8.6

Things done

drewrisinger

It looks like this is supposed to be used as a standalone package. I recommend following the example of https://github.com/NixOS/nixpkgs/blob/master/pkgs/misc/autotiling/default.nix.

In all-packages.nix, it's called as: autotiling = python3Packages.callPackage PATH_TO_NIX_FILE { };

I'll review this more thoroughly once it more closely matches existing best practices.

pkgs/applications/misc/pylode/default.nix

koslambrou · 2020-08-21T19:29:33Z

@drewrisinger I modified the PR.

I tried following best practices from https://github.com/NixOS/nixpkgs/blob/master/pkgs/misc/autotiling/default.nix.

One thing I couldn't do was use fetchPypi instead of fetchFromGithub as it produces the following error:

Executing setuptoolsBuildPhase
Traceback (most recent call last):
  File "nix_run_setup", line 8, in <module>
    exec(compile(getattr(tokenize, 'open', open)(__file__).read().replace('\\r\\n', '\\n'), __file__, 'exec'))
  File "setup.py", line 20, in <module>
    with open_local(['requirements.txt']) as req:
  File "setup.py", line 14, in open_local
    return codecs.open(path, mode, encoding)
  File "/nix/store/fjgnz0xfl04hsblsi4ym5y5akfh6mlmy-python3-3.8.5/lib/python3.8/codecs.py", line 905, in open
    file = builtins.open(filename, mode, buffering)
FileNotFoundError: [Errno 2] No such file or directory: '/build/pyLODE-2.8.3/requirements.txt'
builder for '/nix/store/5ccdmbk2298755s4h1mg7q8qdiglfj81-pyLODE-2.8.3.drv' failed with exit code 1
error: build of '/nix/store/5ccdmbk2298755s4h1mg7q8qdiglfj81-pyLODE-2.8.3.drv' failed

zowoq · 2020-08-22T01:26:51Z

@ofborg eval

drewrisinger · 2020-08-24T11:50:53Z

@drewrisinger I modified the PR.

I tried following best practices from https://github.com/NixOS/nixpkgs/blob/master/pkgs/misc/autotiling/default.nix.

One thing I couldn't do was use fetchPypi instead of fetchFromGithub as it produces the following error:

Executing setuptoolsBuildPhase
Traceback (most recent call last):
  File "nix_run_setup", line 8, in <module>
    exec(compile(getattr(tokenize, 'open', open)(__file__).read().replace('\\r\\n', '\\n'), __file__, 'exec'))
  File "setup.py", line 20, in <module>
    with open_local(['requirements.txt']) as req:
  File "setup.py", line 14, in open_local
    return codecs.open(path, mode, encoding)
  File "/nix/store/fjgnz0xfl04hsblsi4ym5y5akfh6mlmy-python3-3.8.5/lib/python3.8/codecs.py", line 905, in open
    file = builtins.open(filename, mode, buffering)
FileNotFoundError: [Errno 2] No such file or directory: '/build/pyLODE-2.8.3/requirements.txt'
builder for '/nix/store/5ccdmbk2298755s4h1mg7q8qdiglfj81-pyLODE-2.8.3.drv' failed with exit code 1
error: build of '/nix/store/5ccdmbk2298755s4h1mg7q8qdiglfj81-pyLODE-2.8.3.drv' failed

Thanks. I meant generally with respect to formatting & layout. I'll leave comments.

drewrisinger

Few formatting fixes
Move package call to different location in all-packages.nix
Run tests
Haven't tested build yet, waiting on tests.

drewrisinger · 2020-08-24T11:51:27Z

pkgs/misc/pylode/default.nix

+  propagatedBuildInputs = [
+    rdflib
+    rdflib-jsonld
+    requests
+    dateutil
+    markdown
+    jinja2
+    gunicorn
+    pytest
+    falcon
+    isodate
+    six
+  ];


Sort alphabetically.

Pytest shouldn't need to be a runtime input (should probably be a checkInput)

drewrisinger · 2020-08-24T11:52:23Z

pkgs/misc/pylode/default.nix

+  postPatch = ''
+    substituteInPlace requirements.txt --replace "argparse" ""
+  '';


Does it actually fail to build with this in the requirements? If it builds, I'd say leave it in. argparse is a built-in python library.

Yes it actually fails if I leave argparse in the requirements

Huh. Strange. Thanks for checking 👍

drewrisinger · 2020-08-24T11:52:33Z

pkgs/misc/pylode/default.nix

+    sha256 = "01a6b916r32irnllfr0rxz2mn9v6xn94lrgjdb8dhkki9ninqddy";
+  };
+
+  # Waiting for argparse to be removed in requirements.txt from upstream package


Reference pull request/issue if there is one.

drewrisinger · 2020-08-24T11:53:42Z

pkgs/misc/pylode/default.nix

+{ lib, buildPythonApplication, fetchFromGitHub, rdflib, rdflib-jsonld, requests, dateutil, markdown, jinja2, isodate, gunicorn, falcon, pytest, six }:
+


Break onto separate lines and sort the packages alphabetically like:

Suggested change

{ lib, buildPythonApplication, fetchFromGitHub, rdflib, rdflib-jsonld, requests, dateutil, markdown, jinja2, isodate, gunicorn, falcon, pytest, six }:

{ lib

, buildPythonApplication

, fetchFromGitHub

, dateutil

, falcon

, ...

}:

drewrisinger · 2020-08-24T11:56:57Z

pkgs/misc/pylode/default.nix

+  ];
+
+  # Import errors in pylode tests
+  doCheck = false;


Try like this (replacing pytest in arguments with pytestCheckHook:

Suggested change

doCheck = false;

checkInputs = [ pytestCheckHook ];

My guess is that you're getting version conflicts between the installed version of pyLODE in $out and the tests in the source directory. See #95689 (comment) (esp. 5.) for a possible solution.

Tests are generally preferred if they exist. If this was a python package vs application, I'd recommend doing pythonImportsCheck = [ "name_of_python_package_here" ];

I checked in the upstream pyLODE package and it seems that the tests are failing over there with an import error. Opened an issue over there.

@koslambrou would you like to update this PR with tests enabled and version 2.8.4?

@doronbehar Still have to wait for 2.8.5. The tests are failing in 2.8.4 and the commit that solves this was added the day after the 2.8.4 release.

The tests are failing in 2.8.4 and the commit that solves this was added the day after the 2.8.4 release.

Well then it should be fairly easy to grab that commit fetched from upstream. When doing it we usually put a comment that the referenced commit is included in 2.8.5 and should be removed with the update to a version newer then 2.8.4

drewrisinger · 2020-08-24T11:57:11Z

pkgs/misc/pylode/default.nix

+    homepage = "https://github.com/RDFLib/pyLODE";
+    description = "An OWL ontology documentation tool using Python and templating, based on LODE";


Typically description is before homepage.

drewrisinger · 2020-08-24T11:59:23Z

pkgs/top-level/all-packages.nix

+  pylode = python3Packages.callPackage ../misc/pylode { };
+


These are loosely sorted by directory. Your package probably doesn't belong here with the rest of the native python packages. Look for a place in the range of lines 15000+

drewrisinger · 2020-08-24T12:04:42Z

pkgs/misc/pylode/default.nix

+  meta = with lib; {
+    homepage = "https://github.com/RDFLib/pyLODE";
+    description = "An OWL ontology documentation tool using Python and templating, based on LODE";
+    license = licenses.gpl3;


Use of this is deprecated per https://discourse.nixos.org/t/lib-licenses-gpl3-co-are-now-deprecated/8206

It looks like gpl3Only vs gpl3Plus is appropriate here based on the license text.

drewrisinger · 2020-08-24T16:49:38Z

Last comment I have is maybe this belongs in https://github.com/NixOS/nixpkgs/tree/master/pkgs/applications/science/biology because it's a taxonomy package? Doesn't seem like there's much in ./misc other than emulators

drewrisinger · 2020-08-24T16:51:40Z

I'd also still prefer to see some sort of self-test because python applications are fairly unstable due to constant library updates.

koslambrou · 2020-08-24T17:01:14Z

Then, for the tests, I'll wait until the're fixed in the pyLODE package before requesting another review.

This application is a documentation generator for ontology (taxonomy) web formats (.owl). However, it is a general purpose package and not specifically related to biology. You think pkgs/applications/science/biology is still the best location ?

drewrisinger · 2020-08-24T18:25:13Z

I'd suggest either pkgs/applications/science/biology or pkgs/applications/science/misc. I'm not too well-versed with best practices for those folders.

koslambrou · 2021-03-05T20:31:43Z

@drewrisinger Updated to pyLODE v2.8.6 and all issues were resolved.

pkgs/top-level/all-packages.nix

drewrisinger

Diff LGTM, minus changelog & @doronbehar comments
Commits LGTM
Builds via nix-review on x86_64-linux:

1 package built:
pylode

drewrisinger · 2021-03-06T13:25:56Z

pkgs/misc/pylode/default.nix

+
+  meta = with lib; {
+    description = "An OWL ontology documentation tool using Python and templating, based on LODE";
+    homepage = "https://github.com/RDFLib/pyLODE";


Best practice now is to add a meta.changelog attribute if it exists.

Suggested change

homepage = "https://github.com/RDFLib/pyLODE";

homepage = "https://github.com/RDFLib/pyLODE";

changelog = "https://github.com/RDFlib/pyLODE...";

Or a more user-friendly documentation site if it exists, e.g. readthedocs

The package has nothing of the sort. The only existing documentation is the README of the Github repository.

pkgs/top-level/all-packages.nix

doronbehar · 2021-03-07T13:08:37Z

Result of nixpkgs-review pr 95863 run on x86_64-linux 1

1 package built:

pylode

koslambrou mentioned this pull request Aug 20, 2020

Packaging request : pyLODE #84196

Closed

ofborg bot added 8.has: package (new) 11.by: package-maintainer 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 10.rebuild-linux: 1 labels Aug 21, 2020

drewrisinger suggested changes Aug 21, 2020

View reviewed changes

pkgs/applications/misc/pylode/default.nix Outdated Show resolved Hide resolved

koslambrou requested a review from drewrisinger August 24, 2020 10:54

drewrisinger suggested changes Aug 24, 2020

View reviewed changes

drewrisinger reviewed Aug 24, 2020

View reviewed changes

koslambrou requested a review from drewrisinger August 24, 2020 13:54

koslambrou changed the title ~~pylode: init at 2.8.3~~ pylode: init at 2.8.6 Mar 5, 2021

koslambrou requested a review from doronbehar March 5, 2021 20:31

doronbehar reviewed Mar 6, 2021

View reviewed changes

pkgs/top-level/all-packages.nix Outdated Show resolved Hide resolved

drewrisinger suggested changes Mar 6, 2021

View reviewed changes

doronbehar requested changes Mar 6, 2021

View reviewed changes

pkgs/top-level/all-packages.nix Outdated Show resolved Hide resolved

doronbehar reviewed Mar 6, 2021

View reviewed changes

pkgs/top-level/all-packages.nix Outdated Show resolved Hide resolved

pylode: init at 2.8.6

975ec90

doronbehar approved these changes Mar 7, 2021

View reviewed changes

doronbehar merged commit 95c170b into NixOS:master Mar 7, 2021

koslambrou deleted the add-pylode-package branch March 7, 2021 13:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pylode: init at 2.8.6 #95863

pylode: init at 2.8.6 #95863

koslambrou commented Aug 20, 2020 •

edited

drewrisinger left a comment

koslambrou commented Aug 21, 2020

zowoq commented Aug 22, 2020

drewrisinger commented Aug 24, 2020

drewrisinger left a comment

drewrisinger Aug 24, 2020 •

edited

drewrisinger Aug 24, 2020

koslambrou Aug 24, 2020

drewrisinger Aug 24, 2020

drewrisinger Aug 24, 2020

drewrisinger Aug 24, 2020

drewrisinger Aug 24, 2020 •

edited

koslambrou Aug 24, 2020

doronbehar Dec 18, 2020

koslambrou Dec 27, 2020

doronbehar Dec 27, 2020

drewrisinger Aug 24, 2020

drewrisinger Aug 24, 2020

drewrisinger Aug 24, 2020

drewrisinger commented Aug 24, 2020

drewrisinger commented Aug 24, 2020

koslambrou commented Aug 24, 2020

drewrisinger commented Aug 24, 2020

koslambrou commented Mar 5, 2021

drewrisinger left a comment

drewrisinger Mar 6, 2021

koslambrou Mar 6, 2021

doronbehar commented Mar 7, 2021

		{ lib, buildPythonApplication, fetchFromGitHub, rdflib, rdflib-jsonld, requests, dateutil, markdown, jinja2, isodate, gunicorn, falcon, pytest, six }:

		homepage = "https://github.com/RDFLib/pyLODE";
		description = "An OWL ontology documentation tool using Python and templating, based on LODE";

	homepage = "https://github.com/RDFLib/pyLODE";
	homepage = "https://github.com/RDFLib/pyLODE";
	changelog = "https://github.com/RDFlib/pyLODE...";

pylode: init at 2.8.6 #95863

pylode: init at 2.8.6 #95863

Conversation

koslambrou commented Aug 20, 2020 • edited

Motivation for this change

Things done

drewrisinger left a comment

Choose a reason for hiding this comment

koslambrou commented Aug 21, 2020

zowoq commented Aug 22, 2020

drewrisinger commented Aug 24, 2020

drewrisinger left a comment

Choose a reason for hiding this comment

drewrisinger Aug 24, 2020 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drewrisinger Aug 24, 2020 • 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

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

drewrisinger commented Aug 24, 2020

drewrisinger commented Aug 24, 2020

koslambrou commented Aug 24, 2020

drewrisinger commented Aug 24, 2020

koslambrou commented Mar 5, 2021

drewrisinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

doronbehar commented Mar 7, 2021

koslambrou commented Aug 20, 2020 •

edited

drewrisinger Aug 24, 2020 •

edited

drewrisinger Aug 24, 2020 •

edited