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

Support PEP 600 tags #293

Merged
merged 6 commits into from
May 27, 2020
Merged

Support PEP 600 tags #293

merged 6 commits into from
May 27, 2020

Conversation

mattip
Copy link
Contributor

@mattip mattip commented Apr 8, 2020

xref gh-280. This adds a whole bunch of PEP 600 tags (my python reports `os.confstr('CS_GNU_LIBC_VERSION') of 2.27): EDIT: after reversing the order in f432bcd

>>> from packaging import tags
>>> print ('\n'.join([str(t) for t in tags.sys_tags()]))
cp37-cp37m-manylinux_2_27_x86_64
cp37-cp37m-manylinux_2_26_x86_64
cp37-cp37m-manylinux_2_25_x86_64
cp37-cp37m-manylinux_2_24_x86_64
cp37-cp37m-manylinux_2_23_x86_64
cp37-cp37m-manylinux_2_22_x86_64
cp37-cp37m-manylinux_2_21_x86_64
cp37-cp37m-manylinux_2_20_x86_64
cp37-cp37m-manylinux_2_19_x86_64
cp37-cp37m-manylinux_2_18_x86_64
cp37-cp37m-manylinux_2_17_x86_64
cp37-cp37m-manylinux2014_x86_64
cp37-cp37m-manylinux_2_16_x86_64
cp37-cp37m-manylinux_2_15_x86_64
cp37-cp37m-manylinux_2_14_x86_64
cp37-cp37m-manylinux_2_13_x86_64
cp37-cp37m-manylinux_2_12_x86_64
cp37-cp37m-manylinux2010_x86_64
cp37-cp37m-manylinux_2_11_x86_64
cp37-cp37m-manylinux_2_10_x86_64
cp37-cp37m-manylinux_2_9_x86_64
cp37-cp37m-manylinux_2_8_x86_64
cp37-cp37m-manylinux_2_7_x86_64
cp37-cp37m-manylinux_2_6_x86_64
cp37-cp37m-manylinux_2_5_x86_64
cp37-cp37m-manylinux1_x86_64
cp37-cp37m-linux_x86_64
cp37-abi3-manylinux_2_27_x86_64
...

@mattip mattip force-pushed the pep-600 branch 2 times, most recently from 958576a to 372650d Compare April 8, 2020 14:09
@mattip
Copy link
Contributor Author

mattip commented Apr 8, 2020

It seems I need to mock some failures to get coverage to 100%

@brettcannon
Copy link
Member

Thanks for taking a stab at this! I haven't looked at the code, but a tweak we might want to make to the output is to emit the manylinux1, manylinux2010, and manylinux2014 tags after the matching perennial manylinux tag. We also want to emit the tags in priority order, so newer glibc versions should come first.

@brettcannon brettcannon self-requested a review April 8, 2020 18:28
@mattip
Copy link
Contributor Author

mattip commented Apr 9, 2020

Ahh, so the correct order, assuming precedence for the perennial tags, would be this?

cp37-cp37m-manylinux_2_27_x86_64
cp37-cp37m-manylinux_2_26_x86_64
cp37-cp37m-manylinux_2_25_x86_64
cp37-cp37m-manylinux_2_24_x86_64
cp37-cp37m-manylinux_2_23_x86_64
cp37-cp37m-manylinux_2_22_x86_64
cp37-cp37m-manylinux_2_21_x86_64
cp37-cp37m-manylinux_2_20_x86_64
cp37-cp37m-manylinux_2_19_x86_64
cp37-cp37m-manylinux_2_18_x86_64
cp37-cp37m-manylinux_2_17_x86_64
cp37-cp37m-manylinux2014_x86_64
cp37-cp37m-manylinux_2_16_x86_64
cp37-cp37m-manylinux_2_15_x86_64
cp37-cp37m-manylinux_2_14_x86_64
cp37-cp37m-manylinux_2_13_x86_64
cp37-cp37m-manylinux_2_12_x86_64
cp37-cp37m-manylinux2010_x86_64
cp37-cp37m-manylinux_2_11_x86_64
cp37-cp37m-manylinux_2_10_x86_64
cp37-cp37m-manylinux_2_9_x86_64
cp37-cp37m-manylinux_2_8_x86_64
cp37-cp37m-manylinux_2_7_x86_64
cp37-cp37m-manylinux_2_6_x86_64
cp37-cp37m-manylinux_2_5_x86_64
cp37-cp37m-manylinux1_x86_64
cp37-cp37m-linux_x86_64

@brettcannon
Copy link
Member

@mattip yep, exactly! Basically the way to think about the order is if I got every wheel file name for a specific version, I should be able to iterate through what sys_tags() returns and as soon as I have a tag match with a file I stop looking because I found the best possible match for my environment.

This is also why everything in packaging.tags is a generator: so you can short-circuit the search and potentially avoid costly environment checks.

@mattip
Copy link
Contributor Author

mattip commented Apr 10, 2020

I reversed the tags and added tests for lines that weren't in the coverage report. However coverage is now saying the critical part of the code that I added is not covered. How can that be when the tests fail if I remove it? Edit: fixed this in c7d1851.

@mattip
Copy link
Contributor Author

mattip commented Apr 16, 2020

Any thoughts? CI is passing, even though travis thinks it is still pending.

@brettcannon brettcannon reopened this Apr 16, 2020
@brettcannon
Copy link
Member

Sorry, I've personally been swamped lately on top of vacation, so I just haven't had time to review the PR yet.

Copy link
Member

@brettcannon brettcannon left a comment

Choose a reason for hiding this comment

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

Just some tweaks to generalize this to > glibc 2.

packaging/tags.py Outdated Show resolved Hide resolved
packaging/tags.py Outdated Show resolved Hide resolved
packaging/tags.py Outdated Show resolved Hide resolved
packaging/tags.py Outdated Show resolved Hide resolved
packaging/tags.py Outdated Show resolved Hide resolved
packaging/tags.py Outdated Show resolved Hide resolved
packaging/tags.py Outdated Show resolved Hide resolved
plat, (2, glibc_minor)
)
if manylinux_compat:
yield linux.replace("linux", plat)
Copy link
Member

Choose a reason for hiding this comment

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

Instead of doing the replacing, maybe the map should just hard-code all possible supported architectures since there aren't that many.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would only save one instance of replace, the PEP 600 tags do not use the map.

@mattip mattip force-pushed the pep-600 branch 2 times, most recently from c7b2582 to 3e8aa45 Compare April 19, 2020 20:23
@mattip
Copy link
Contributor Author

mattip commented Apr 20, 2020

Tests are passing after fixes as per the review. The code should support newer major versions of glibc, but there still is a logic failure. We assume sys_tags can emit the entire set of valid tags as listed in the comments above. However, if the platform has moved to glibc3.2, what is the highest Y for manylinux_2_Y? We have no way of knowing what the "last glibc2.Y" is. I could just "assume" an arbitrary large number (99?) and a future PR could adapt that to the situation when it arises?

@brettcannon
Copy link
Member

Huh, good point about the cap on glibc 2's minor number if glibc 3 ever becomes a thing. Maybe we can't generalize this for glibc 3 without hard-coding that? I guess the best fall-back is to go with your suggestion, Matti, and just assume 99 as an upper-bound and if the day comes that glibc 3 exists we can hard-code it going forward. That way we somewhat future-proof for people who don't upgrade while not making it ridiculous for us forever.

@mattip
Copy link
Contributor Author

mattip commented Apr 28, 2020

Added a _LAST_GLIBC_MINOR default dictionary that defaults to 50, and generalized the looping to handle maximum versions of each glibc major version (including using (2, 5) or (2,17) as the last one, depending on architecture). Probably overkill, but should not cause failures. Of course this assumes glibc will make good on their backward-compatibility promise.

packaging/tags.py Show resolved Hide resolved
tests/test_tags.py Outdated Show resolved Hide resolved
packaging/tags.py Show resolved Hide resolved
packaging/tags.py Outdated Show resolved Hide resolved
@mattip
Copy link
Contributor Author

mattip commented Apr 28, 2020

Requested changes made, there is one test failure with no log.

@pradyunsg
Copy link
Member

Triggered a re-run for all jobs.

@pradyunsg pradyunsg closed this Apr 28, 2020
@pradyunsg pradyunsg reopened this Apr 28, 2020
@pradyunsg
Copy link
Member

Triggered a "bigger" re-run. :)

@mattip
Copy link
Contributor Author

mattip commented Apr 28, 2020

Trying again

@mattip mattip closed this Apr 28, 2020
@mattip mattip reopened this Apr 28, 2020
Copy link
Contributor Author

@mattip mattip left a comment

Choose a reason for hiding this comment

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

The refactor to allow calling a _manylinux.manylinux_compatible function required a bit of refactoring. So basically we are back to a full review, sorry.

packaging/tags.py Show resolved Hide resolved
packaging/tags.py Outdated Show resolved Hide resolved
packaging/tags.py Outdated Show resolved Hide resolved
packaging/tags.py Show resolved Hide resolved
packaging/tags.py Show resolved Hide resolved
tests/test_tags.py Show resolved Hide resolved
tests/test_tags.py Show resolved Hide resolved
tests/test_tags.py Show resolved Hide resolved
@mattip
Copy link
Contributor Author

mattip commented May 3, 2020

All but one macOS job passes

@mattip
Copy link
Contributor Author

mattip commented May 15, 2020

Any thoughts?

@brettcannon
Copy link
Member

Any thoughts?

That I wish I had less deadlines at work? 😉

This is at the top of my review queue, but this month is not good for me for open source time.

Copy link
Member

@brettcannon brettcannon left a comment

Choose a reason for hiding this comment

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

Very small change to make to use monkeypatch in a test and then a clarifying question on a comment. Otherwise LGTM!

packaging/tags.py Show resolved Hide resolved
Comment on lines 296 to 301
def test_module_declaration(self, manylinux_module, attribute, glibc, tf):
manylinux = "manylinux{}_compatible".format(attribute)
setattr(manylinux_module, manylinux, tf)
res = tags._is_manylinux_compatible(manylinux, "x86_64", glibc)
assert tf is res
delattr(manylinux_module, manylinux)
Copy link
Member

Choose a reason for hiding this comment

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

It's easiest to use pytest's monkeypatch for temporarily setting an attribute:

Suggested change
def test_module_declaration(self, manylinux_module, attribute, glibc, tf):
manylinux = "manylinux{}_compatible".format(attribute)
setattr(manylinux_module, manylinux, tf)
res = tags._is_manylinux_compatible(manylinux, "x86_64", glibc)
assert tf is res
delattr(manylinux_module, manylinux)
def test_module_declaration(self, monkeypatch, manylinux_module, attribute, glibc, tf):
manylinux = "manylinux{}_compatible".format(attribute)
monkeypatch.setattr(manylinux_module, "manylinux", tf)
res = tags._is_manylinux_compatible(manylinux, "x86_64", glibc)
assert tf is res

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried that. monkypatch cannot use setattr on an attribute that was not already present, I get

>       monkeypatch.setattr(manylinux_module, manylinux, tf)
E       AttributeError: <module '_manylinux'> has no attribute \
                     'manylinux2014_compatible'

Copy link
Member

Choose a reason for hiding this comment

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

tests/test_tags.py Show resolved Hide resolved
tests/test_tags.py Show resolved Hide resolved
@mattip
Copy link
Contributor Author

mattip commented May 23, 2020

Sorry, I lost the thread of comments that this question relates to:

Do you have something specific you want to change that you haven't and so the comment is on its previous state? Or is there something you want to do that you haven't yet?

Comment on lines 296 to 301
def test_module_declaration(self, manylinux_module, attribute, glibc, tf):
manylinux = "manylinux{}_compatible".format(attribute)
setattr(manylinux_module, manylinux, tf)
res = tags._is_manylinux_compatible(manylinux, "x86_64", glibc)
assert tf is res
delattr(manylinux_module, manylinux)
Copy link
Member

Choose a reason for hiding this comment

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

@brettcannon
Copy link
Member

@mattip

Sorry, I lost the thread of comments that this question relates to:
Do you have something specific you want to change that you haven't and so the comment is on its previous state? Or is there something you want to do that you haven't yet?

It's in response to a comment on test_tags.py, line 587 (if you look under the "Files Changed" tab you can find it again):

This is a weird test. Previously it depended on the monkeypatch on line 42 to set _have_compatible_glibc in order to reject any glibc value.

@mattip
Copy link
Contributor Author

mattip commented May 24, 2020

Tests are passing with monkeypatch and raising=False

@brettcannon brettcannon self-requested a review May 25, 2020 02:05
@brettcannon
Copy link
Member

@pradyunsg did you want to do another review, or should I go ahead and merge this?

@pradyunsg
Copy link
Member

Go ahead. I'm tied up with college reports and submissions for the next few days.

@brettcannon brettcannon merged commit 28a2e2b into pypa:master May 27, 2020
@brettcannon
Copy link
Member

Thanks for all the work on this, @mattip !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants