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
catfish: 1.4.2 -> 1.4.4 #34899
catfish: 1.4.2 -> 1.4.4 #34899
Conversation
@@ -45,6 +45,9 @@ pythonPackages.buildPythonApplication rec { | |||
done | |||
''; | |||
|
|||
# Disable tests, they don't exist and break the builds (according to debian) | |||
doCheck = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better check yourself. Are there tests or are there no tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testing gives the following errors:
running install tests
ERROR: Python module helpers not found
ERROR: Python module Window not found
ERROR: Python module catfishconfig not found
ERROR: Python module catfishconfig not found
ERROR: Python module Builder not found
ERROR: Python module helpers not found
ERROR: Python module catfishconfig not found
ERROR: Python module zeitgeist.client not found
ERROR: Python module zeitgeist.datamodel not found
ERROR: Python module zeitgeist not found
ERROR: Python module helpers not found
Those modules are installed at $out/lib/python2.7/site-packages/catfish_lib/
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the tarball, I don't see any test. Do you know what command needs to be run for the tests? If not, I think you can honestly say there are no tests. Please leave a comment why you disable them in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also do not see any test in the tarball.
@@ -3,13 +3,13 @@ | |||
|
|||
pythonPackages.buildPythonApplication rec { | |||
majorver = "1.4"; | |||
minorver = "2"; | |||
minorver = "4"; | |||
version = "${majorver}.${minorver}"; | |||
name = "catfish-${version}"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please get rif of the name
here and only specify pname
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the rationale behind that? I failed to find any documentation on it. Notice that name
is used in the src
attribute.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not necessary anymore for buildPython*
. Without name, it easier to override the attributes for using a custom version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And how should the url be written in this case? When I remove name
and add pname
, I get
error: undefined variable ‘name’ at /alt/nixpkgs/pkgs/applications/search/catfish/default.nix:11:84
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use ${pname}-${version}
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the tarball, bin/catfish
has a shebang of #!/usr/bin/python3
. So, don't you think this should use Python 3?
@@ -45,6 +45,9 @@ pythonPackages.buildPythonApplication rec { | |||
done | |||
''; | |||
|
|||
# Disable tests, they don't exist and break the builds (according to debian) | |||
doCheck = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the tarball, I don't see any test. Do you know what command needs to be run for the tests? If not, I think you can honestly say there are no tests. Please leave a comment why you disable them in this case.
What do you think about using Python 3? @GrahamcOfBorg build catfish |
Success on x86_64-linux (full log) Partial log (click to expand)
|
When running
|
Maybe @FRidh can help. |
I made it work by adding |
Failure on aarch64-linux (full log) Partial log (click to expand)
|
Now it works with both Python 2 and Python 3. |
Done |
Maybe it can be merged now. |
merging! Thanks for the contribution! |
Motivation for this change
Update to version 1.4.4
Things done
build-use-sandbox
innix.conf
on non-NixOS)nix-shell -p nox --run "nox-review wip"
./result/bin/
)