-
-
Notifications
You must be signed in to change notification settings - Fork 14.5k
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
python312Packages.papis: 0.13 -> 0.14 #354823
Conversation
0e64844
to
d3c173b
Compare
Thanks for the feedback @steeleduncan.
no `with lib` #208242
Done on all three packages mentioned.
`refs/tags/${version}` is safer should there be a branch with the same name as the tag
Unfortunately there is none [1].
[1]: https://github.com/lukasschwab/arxiv.py/branches/all
…--
octvs
|
Understood, but if one were to be added in the future it would not be clear what Specifying |
Specifying `refs/tags/...` is more precise, and leaves the derivation safe
against such a change
Oh, I understood this one just now. Checking how github forms download links
for branches and tags, and then skimming over `fetchFromGitHub` source
clarified [1].
Thanks, fixed!
[1]: https://github.com/NixOS/nixpkgs/blob/4ac89e5e22d3e225057d90824618feeb7059947d/pkgs/build-support/fetchgithub/default.nix
|
144bc1a
to
a04feec
Compare
Since |
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.
mis-approved previously.
```suggestion
version = "2.1.0";
pyproject = true;
```
As far as I understand from arxiv repo, they use `setup.py`, there is no
`pyproject.toml` present, should it be still set to true?
> - disabled = pythonOlder "3.7";
+ disabled = pythonOlder "3.8";
You can also just remove this line entirely. It is not that useful.
I couldn't find explanation of the behaviour of `pythonOlder` either through
google or `grep`ing through `nixpkgs`. Therefore wasn't sure if it is `<=` or
`<`.
Just to understand it: Is it not useful because current oldest python version
available on nixpkgs is 3.9? (except `python2`).
>
- disabled = pythonOlder "3.7";
+ disabled = pythonOlder "3.8";
src = fetchFromGitHub {
owner = "papis";
repo = pname;
```suggestion
repo = "papis";
```
Again to understand: is there a reason to avoid using `pname` variable there?
Others have been fixed, thanks a lot for the feedback!
|
Yes, it should work nonetheless. (Although quite counter intuitive).
Yes, it could lead to a broken build if the name of the package is changed. It does not occur often, but this is considered as best practice. Also, it is better to reply to the change requests directly in their individual message thread. |
> As far as I understand from arxiv repo, they use `setup.py`, there is no `pyproject.toml` present, should it be still set to true?
Yes, it should work nonetheless. (Although quite counter intuitive).
Now it fails to build with
```sh
$ nix build .#python3Packages.arxiv
warning: Git tree '/home/oct/git/nixpkgs' is dirty
error: builder for '/nix/store/wam7hlzgi1mcbfmzycbdns3sjzah4l3j-python3.12-arxiv-2.1.0.drv' failed with exit code 1;
last 25 log lines:
writing requirements to arxiv.egg-info/requires.txt
writing top-level names to arxiv.egg-info/top_level.txt
reading manifest file 'arxiv.egg-info/SOURCES.txt'
adding license file 'LICENSE.txt'
writing manifest file 'arxiv.egg-info/SOURCES.txt'
Copying arxiv.egg-info to build/bdist.linux-x86_64/wheel/./arxiv-2.1.0-py3.12.egg-info
running install_scripts
creating build/bdist.linux-x86_64/wheel/arxiv-2.1.0.dist-info/WHEEL
creating '/build/source/dist/.tmp-51_34jd5/arxiv-2.1.0-py3-none-any.whl' and adding 'build/bdist.linux-x86_64/wheel' to it
adding 'arxiv/__init__.py'
adding 'arxiv/arxiv.py'
adding 'arxiv-2.1.0.dist-info/LICENSE.txt'
adding 'arxiv-2.1.0.dist-info/METADATA'
adding 'arxiv-2.1.0.dist-info/WHEEL'
adding 'arxiv-2.1.0.dist-info/top_level.txt'
adding 'arxiv-2.1.0.dist-info/RECORD'
removing build/bdist.linux-x86_64/wheel
Successfully built arxiv-2.1.0-py3-none-any.whl
Finished creating a wheel...
Finished executing pypaBuildPhase
Running phase: pythonRuntimeDepsCheckHook
Executing pythonRuntimeDepsCheck
Checking runtime dependencies for arxiv-2.1.0-py3-none-any.whl
- feedparser==6.0.10 not satisfied by version 6.0.11
- requests==2.31.0 not satisfied by version 2.32.3
For full logs, run 'nix log /nix/store/wam7hlzgi1mcbfmzycbdns3sjzah4l3j-python3.12-arxiv-2.1.0.drv'.
```
Somehow the `~=` clauses on `requirements.txt` of arxiv [1] does get understood
as `==` which fails the build. It builds fine without `pyproject=true`.
Beyond the build failure, can one find the reasoning [here][2] maybe?
> I couldn't find explanation of the behaviour of `pythonOlder` either through google or `grep`ing through `nixpkgs`. Therefore wasn't sure if it is `<=` or `<`.
`pythonOlder` litterally means "if older", so `<`. "3.11" is not older than "3.11", itself.
Thanks, should it rather be removed still?
> Just to learn it fully: is there a reason to avoid using `pname` variable there?
Yes, it could lead to a broken build if the name of the package is changed. It does not occur often, but this is considered as best practice.
That makes sense, thanks for the explanation. Let me know if I should do the
same for the `meta.changelog`, which also refers back to `meta.homepage`.
_Although I can't imagine a similar scenario for this case._
[1]: https://github.com/lukasschwab/arxiv.py/blob/a1180d0ec6851be4e8aaa5e11a1e5212724f1e8e/requirements.txt
[2]: #253154
|
These failed builds are due to test cases of `arxiv.py` which establishes
http connections to arxiv api, which in sandbox fails.
I built the package with
```sh
$ nix build --option sandbox false .#python3Packages.arxiv
```
Should I disable these tests on `arxiv.py` derivation or somehow disable
sandbox on build?
|
You can first try adding |
If it is not enough, then you can just add those tests to `disabledTests`.
I've added tests that are touching network to the `disabledTests`. I can't run
`nixpkgs-review` locally since its memory usage climbs steadily until it would
crash just as any other described here[1].
If the ofborg runs are done in the morning I can give another try to run it
locally.
But now it builds without `--option sandbox false`.
[1]: Mic92/nixpkgs-review#103
|
fa2e3e5
to
6f5cdeb
Compare
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.
LGTM
|
Please leave a comment on why disable these tests.
Just done, thanks for the input.
|
@octvs I had force-pushed some (mostly cosmetic) changes to your branch. In your last-push you got rid of those changes. |
@octvs I had force-pushed some (mostly cosmetic) changes to your branch.
I've received an e-mail about that force-push, but I couldn't manage my way
around GitHub well enough to see any changes.
In your last-push you got rid of those changes.
Yeah I also force-pushed for @natsukium's feedback, without the knowledge of
your changes, so they got lost. Sorry.
As far as GitHub UI lets me see those changes [1], I will try to recover them.
```diff
diff --git a/pkgs/development/python-modules/arxiv/default.nix b/pkgs/development/python-modules/arxiv/default.nix
index 462090a9fd51..87628f6d2cb6 100644
--- a/pkgs/development/python-modules/arxiv/default.nix
+++ b/pkgs/development/python-modules/arxiv/default.nix
@@ -2,11 +2,17 @@
lib,
buildPythonPackage,
fetchFromGitHub,
+
+ # build-system
+ setuptools,
+
+ # dependencies
feedparser,
+ requests,
+
+ # tests
mock,
pytestCheckHook,
- requests,
- setuptools,
}:
buildPythonPackage rec {
pname = "arxiv";
```
I haven't encountered these comments on inputs before, but if it is
required/requested I will add them.
```diff
diff --git a/pkgs/development/python-modules/papis/default.nix b/pkgs/development/python-modules/papis/default.nix
index 135ad8498cd3..ed4045e381cf 100644
--- a/pkgs/development/python-modules/papis/default.nix
+++ b/pkgs/development/python-modules/papis/default.nix
@@ -1,6 +1,5 @@
{
lib,
- stdenv,
arxiv,
beautifulsoup4,
bibtexparser,
@@ -9,7 +8,6 @@
click,
colorama,
configparser,
- docutils,
dominate,
fetchFromGitHub,
filetype,
@@ -28,11 +26,14 @@
pythonOlder,
pyyaml,
requests,
- sphinx,
- sphinx-click,
stevedore,
typing-extensions,
whoosh,
+
+ # tests
+ docutils,
+ sphinx,
+ sphinx-click,
}:
buildPythonPackage rec {
pname = "papis";
```
`stdenv` is needed on L103 [2]. Either your local repo is outdated or GitHub is
messing with the information.
So the resulting patch looks like this
```diff
diff --git a/pkgs/development/python-modules/arxiv/default.nix b/pkgs/development/python-modules/arxiv/default.nix
index 462090a9fd51..87628f6d2cb6 100644
--- a/pkgs/development/python-modules/arxiv/default.nix
+++ b/pkgs/development/python-modules/arxiv/default.nix
@@ -2,11 +2,17 @@
lib,
buildPythonPackage,
fetchFromGitHub,
+
+ # build-system
+ setuptools,
+
+ # dependencies
feedparser,
+ requests,
+
+ # tests
mock,
pytestCheckHook,
- requests,
- setuptools,
}:
buildPythonPackage rec {
pname = "arxiv";
diff --git a/pkgs/development/python-modules/papis/default.nix b/pkgs/development/python-modules/papis/default.nix
index 135ad8498cd3..145920bf28c8 100644
--- a/pkgs/development/python-modules/papis/default.nix
+++ b/pkgs/development/python-modules/papis/default.nix
@@ -9,7 +9,6 @@
click,
colorama,
configparser,
- docutils,
dominate,
fetchFromGitHub,
filetype,
@@ -28,11 +27,14 @@
pythonOlder,
pyyaml,
requests,
- sphinx,
- sphinx-click,
stevedore,
typing-extensions,
whoosh,
+
+ # tests
+ docutils,
+ sphinx,
+ sphinx-click,
}:
buildPythonPackage rec {
pname = "papis";
@@ -85,8 +87,11 @@ buildPythonPackage rec {
'';
nativeCheckInputs = [
- pytestCheckHook
+ docutils
git
+ pytestCheckHook
+ sphinx
+ sphinx-click
];
preCheck = ''
```
Let me know if you want me to push them.
[1]: https://github.com/NixOS/nixpkgs/compare/fa2e3e50733ff191cb1ef0cd4773e652a3be9973..6f5cdeb728689ff87fbe7cbaaf271bda1d0b4e9a?diff=unified&w=
[2]: https://github.com/NixOS/nixpkgs/blob/11faa20dff7862100b92d8bb6eabdc9aef18b21d/pkgs/development/python-modules/papis/default.nix#L103
|
ed19395
to
bbca6d9
Compare
|
Hmm, looks like I am not able to force-push anymore... (permission denied) |
Papis had a recent minor release, the relevant parts of the 0.14 release
note are
I've added dependencies
arxiv.py
andsphinx-click
which are mostly takenfrom the upstream flake.
Initially I tried having
fetchFromGithub
onsphinx-click
so in essencehaving
However this fails with
As far as I can find over
the internet
pbr
tries to figure out the version, probably in nix environment it fails todo so.
Asking this around on matrix room, I was advised to stick by
fetchPypi
.Note: First time contributor, comments & feedback highly welcome!
Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.