-
-
Notifications
You must be signed in to change notification settings - Fork 15.1k
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
[wip] Proposal/fetchpypi generalized fetching from pypi #22257
[wip] Proposal/fetchpypi generalized fetching from pypi #22257
Conversation
@RonnyPfannschmidt, thanks for your PR! By analyzing the history of the files in this pull request, we identified @edolstra, @peti, @cmfwyp and @FRidh to be potential reviewers. |
current failures are because setuptools/wheel are not implied build dependencies of pip, i beleive i should split the pr |
pkgs/top-level/python-packages.nix
Outdated
@@ -37,6 +37,20 @@ let | |||
|
|||
graphiteVersion = "0.9.15"; | |||
|
|||
fetchPypiMirror = ({ name ? null , pname ? null, version ? null, kind, ext, sha256}: | |||
let | |||
_name = if name == null then "${pname}-${version}" else name; |
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.
you can do this with the default values of the attributeset
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 dont follow
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 think he means
fetchPypiMirror = ({kind, ext, sha256, ...} @ args:
let
_name = args.name or "${pname}-${version}";
pkgs/top-level/python-packages.nix
Outdated
url = "mirror://pypiio/${kind}/${builtins.substring 0 1 _name}/${_pdrv.name}/${_name}${ext}"; | ||
}); | ||
|
||
fetchSource = args: fetchPypiMirror (args // { kind = "source"; ext=".tar.gz"; }); |
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 used a types
attributeset with a bunch of common extensions, that might be a bit more extensible
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.
care to elaborate
|
||
pypiio = [ | ||
https://pypi.io/packages/ | ||
]; | ||
pypi = [ |
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.
shouldn't pypi.io be part of the pypi mirror?
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.
different root base url, the pypi mirror is having a bad starting segment not useful for the different types
pkgs/top-level/python-packages.nix
Outdated
in | ||
pkgs.fetchurl { | ||
inherit sha256; | ||
url = "mirror://pypiio/${kind}/${builtins.substring 0 1 _name}/${_pdrv.name}/${_name}${ext}"; |
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.
this actually won't work in all cases :) I know your code should work, but I found out that there are many different ways how URL is generated on pypi.python.org. Also, the below code should make fetchSourceZip obsolete.
fetchpypi = name: version: hash: pkgs.fetchurl {
urls = [
"https://pypi.python.org/packages/source/${builtins.substring 0 1 name}/${name}/${name}-${version}.tar.gz"
"https://pypi.python.org/packages/source/${builtins.substring 0 1 name}/${name}/${pkgs.lib.toLower name}-${version}.tar.gz"
"https://pypi.python.org/packages/source/${builtins.substring 0 1 name}/${name}/${name}-${version}.tgz"
"https://pypi.python.org/packages/source/${builtins.substring 0 1 name}/${name}/${pkgs.lib.toLower name}-${version}.tgz"
"https://pypi.python.org/packages/source/${builtins.substring 0 1 name}/${name}/${name}-${version}.zip"
"https://pypi.python.org/packages/source/${builtins.substring 0 1 name}/${name}/${pkgs.lib.toLower name}-${version}.zip"
];
md5 = "${hash}";
};
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 should work if if normalized names are used, pip does the same since 9.x.
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 know it should work with normalized names, but my experience tells me different :)
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.
Oh, perhaps it's only for the metadata then? That's why I used pip download
for my implementation.
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.
lovely idea
Inspired by #22257. It is recommend to use fetchpypi and, when fetching a wheel, to pass `format = "wheel"` (typically you can just `inherit format;`). The hash type is fixed (for now) because this is the hash type PyPI uses.
I intend to merge 695ff0d |
Though maybe more urls have to be added as @garbas wrote. |
i haven't gotten around updating, i#ll try to take a look after work |
@FRidh i belive this is obsolete now? |
Yes, I think it is. Thanks for the proposal though! |
Motivation for this change
add a more general tool to fetch packages from pypi using fetchurl
step1 is adding the pypi.io base mirror and testing the first helper on a few packages
the next steps are adding wheel fetching and updating more packages
followup to #22256 22256
Things done
(nix.useSandbox on NixOS,
or option
build-use-sandbox
innix.conf
on non-NixOS)
nix-shell -p nox --run "nox-review wip"
./result/bin/
)