Skip to content
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

Implement pep 503 data-requires-python #3877

Merged
merged 9 commits into from
Aug 11, 2016

Conversation

Carreau
Copy link
Contributor

@Carreau Carreau commented Jul 26, 2016

Allows pip to understand the data-requires-python metadata
information that can be set on a simple repository. This allows pip to
ignore any release or file that would not be compatible with the current
Python version even before trying to download and install this version.

Relevant extract of pep 503 at the time of this writing.

A repository MAY include a data-requires-python attribute on a file
link. This exposes the Requires-Python metadata field, specified in PEP
345 , for the corresponding release. Where this is present, installer
tools SHOULD ignore the download when installing to a Python version
that doesn't satisfy the requirement. For example:

<a href="..." data-requires-python="&gt;=3">...</a>
In the attribute value, < and > have to be HTML encoded as &lt; and &gt;
, respectively.

This can mostly be used to, for example, mark a new sdist of a new
package version as requires-python >3.4, and not fail to install or
upgrade on users systems.

This will require extra patches to PyPI-legacy and warehouse to be
usable. Though releasing a version of pip that understand this feature
is necessary to have wide adoption at the time when these metadata get
actually published.


This will partially replace #3847 (from @takluyver ) once pypi-legacy and warehouse support exposing this metadata as well. I suppose this will be of interest for @astrofrog.

@michaelpacer and I are trying to get a the data-requires-python into pypi-legacy, though we have issue to run it locally. So we tested with a "Fake" local PyPI that just exposes static webpages that we editted by hand to include 'date-requires-python'.

Carreau and others added 6 commits July 26, 2016 16:02
This allows pip to understand the `data-requires-python` metadata
information that can be set on a simple repository. This allows pip to
ignore any release or file that would not be compatible with the current
Python version even before trying to download and install this version.

Relevant extract of pep 503 at the time of this writing.

    A repository MAY include a data-requires-python attribute on a file
    link. This exposes the Requires-Python metadata field, specified in PEP
    345 , for the corresponding release. Where this is present, installer
    tools SHOULD ignore the download when installing to a Python version
    that doesn't satisfy the requirement. For example:

    <a href="..." data-requires-python="&gt;=3">...</a>
    In the attribute value, < and > have to be HTML encoded as &lt; and &gt;
    , respectively.

This can mostly be used to, for example, mark a new sdist of a new
package version as requires-python >3.4, and not  fail to install or
upgrade on users systems.

This will require extra patches to PyPI-legacy and warehouse to be
usable. Though releasing a version of pip that understand this feature
is necessary to have wide adoption at the time when these metadata get
actually published.
@xavfernandez xavfernandez added this to the 8.2 milestone Jul 27, 2016
@@ -828,7 +836,8 @@ def links(self):
url = self.clean_link(
urllib_parse.urljoin(self.base_url, href)
)
yield Link(url, self)
pyrequire = anchor.get('data-requires-python')
yield Link(url, self, requires_python=pyrequire)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would put the unescape here since we are already dealing with html here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@takluyver
Copy link
Member

Thanks for jumping on this; I'm keen to get it into pip as soon as possible so it can start getting to users before we rely on it.

There are still a few Travis failures in running PEP-8.

move the unescape outside of Link class.

reraise using raise that is available on Python 2.6
@Carreau Carreau force-pushed the implement-pep-503-data-requires branch from c8433b8 to 1d10fca Compare July 27, 2016 19:18
@Carreau
Copy link
Contributor Author

Carreau commented Jul 27, 2016

There are still a few Travis failures in running PEP-8.

Yep, saw that, should be fixed as well.

Thanks for jumping on this; I'm keen to get it into pip as soon as possible so it can start getting to users before we rely on it.

This is the exact reason why I did that. Thank for pushing the fixes to pep 503.

@@ -828,7 +841,8 @@ def links(self):
url = self.clean_link(
urllib_parse.urljoin(self.base_url, href)
)
yield Link(url, self)
pyrequire = unescape(anchor.get('data-requires-python'))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to check that the attribute is not None before passing it to unescape, because unescape doesn't like None.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

@xavfernandez
Copy link
Member

A few tests would also be nice.
You can create an index.html file containing a few links and pass the file path to PackageFinder as find_links.
Then check that find_all_candidates returns the expected InstallationCandidate list

@Carreau
Copy link
Contributor Author

Carreau commented Jul 28, 2016

A few tests would also be nice.
You can create an index.html file containing a few links and pass the file path to PackageFinder as find_links.
Then check that find_all_candidates returns the expected InstallationCandidate list

Should be done. Waiting for test to finish, but the one I wrote seems to pass.

@@ -365,6 +366,27 @@ def test_finder_only_installs_stable_releases(data):
assert link.url == "https://foo/bar-1.0.tar.gz"


def test_finder_only_installs_data_require(data):
"""
"""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for empty docstring ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, likely did not commit the description of the tests.

@xavfernandez
Copy link
Member

@Carreau Thanks for your work !
A small changelog in CHANGES.txt and we should be good to go I think.
cc @dstufft for an other review ?

@Carreau
Copy link
Contributor Author

Carreau commented Jul 29, 2016

Docstring fixed, and added an item to CHANGES.txt.

@Carreau
Copy link
Contributor Author

Carreau commented Aug 3, 2016

Is there anything I can do ?

@takluyver
Copy link
Member

This looks good to me.

@Carreau
Copy link
Contributor Author

Carreau commented Aug 11, 2016

Gentle nudge to know if there is anything I can do...

@pfmoore
Copy link
Member

pfmoore commented Aug 11, 2016

I've checked the code and it LGTM. @xavfernandez did you want to wait for @dstufft to OK this?

@xavfernandez
Copy link
Member

@pfmoore I wanted to wait for a second check but yours is fine too :)

@xavfernandez xavfernandez merged commit b506992 into pypa:master Aug 11, 2016
@xavfernandez
Copy link
Member

@Carreau Thanks again 👍

@takluyver
Copy link
Member

Thanks for landing this :-). Now we just need to implement the server-side part.

@Carreau Carreau deleted the implement-pep-503-data-requires branch August 11, 2016 21:27
@Carreau
Copy link
Contributor Author

Carreau commented Aug 11, 2016

Thanks a lot !

I'm trying to get a fix on PyPI-legacy, but I can't figure out how to run it locally... if any of you have insights... I would appreciate help there.

@takluyver
Copy link
Member

I was just about to say that maybe we should focus on making Warehouse respect it and hope that it fully takes over as PyPI soon, but it sounds like you're making some progress with getting legacy PyPI running.

@Carreau
Copy link
Contributor Author

Carreau commented Aug 14, 2016

I was just about to say that maybe we should focus on making Warehouse respect it and hope that it fully takes over as PyPI soon, but it sounds like you're making some progress with getting legacy PyPI running.

Yes, I have an ugly patch running, I need to comment a bunch of code that try to access EC2, but once you get what's wrong it's not that hard to run locally.

I'll document what I'm doing, but we can also implement the simple repo on warehouse.
I'm just wondering is this is the best format for pip to use, I was wondering if "just" a json API would be better on warehouse. It's seem simpler to parse, and easier to extend... but I'm far from knowledgeable enough to make this decision.

I'll be low bandwidth until next monday so It will take me time to clean things up; you can see the branch there though.

@dstufft
Copy link
Member

dstufft commented Aug 14, 2016

We will eventually replace this with a newer API, but that is a longer term effort. Right now that is out of scope of any current efforts.

Sent from my iPhone

On Aug 14, 2016, at 2:28 PM, Matthias Bussonnier notifications@github.com wrote:

I was just about to say that maybe we should focus on making Warehouse respect it and hope that it fully takes over as PyPI soon, but it sounds like you're making some progress with getting legacy PyPI running.

Yes, I have an ugly patch running, I need to comment a bunch of code that try to access EC2, but once you get what's wrong it's not that hard to run locally.

I'll document what I'm doing, but we can also implement the simple repo on warehouse.
I'm just wondering is this is the best format for pip to use, I was wondering if "just" a json API would be better on warehouse. It's seem simpler to parse, and easier to extend... but I'm far from knowledgeable enough to make this decision.

I'll be low bandwidth until next monday so It will take me time to clean things up; you can see the branch there though.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked Outdated issues that have been locked by automation type: enhancement Improvements to functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants