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

PEP 656 musllinux support #411

Merged
merged 7 commits into from
May 3, 2021
Merged

PEP 656 musllinux support #411

merged 7 commits into from
May 3, 2021

Conversation

uranusjr
Copy link
Member

@uranusjr uranusjr commented Mar 18, 2021

@uranusjr

This comment has been minimized.

@uranusjr
Copy link
Member Author

uranusjr commented Apr 7, 2021

Alright, I figured out how to do build test binaries from Docker and wrote tests against them. Logic-wise I think this is ready, but there’s some duplicated logic parsing ELF header between packaging.tags and the _musllinux implementation that I think can be refactored.

@uranusjr uranusjr marked this pull request as ready for review April 7, 2021 01:05
@uranusjr uranusjr force-pushed the musllinux branch 3 times, most recently from cfeeff0 to 30705f7 Compare April 7, 2021 01:19
@EpicWink
Copy link

EpicWink commented Apr 7, 2021

Is there a benefit to testing the tags returned when running in a python:alpine image?

@uranusjr
Copy link
Member Author

uranusjr commented Apr 7, 2021

Maybe, but probably marginal. I didn’t do it (or alpine:latest) since the result is essentially the same as multiarch/alpine:x86_64-edge. The Python inside the container is irralevant; the tests are run on the host Python, against the ELF file built in the container.

@brettcannon brettcannon self-requested a review April 7, 2021 22:06
lines = [n for n in (n.strip() for n in output.splitlines()) if n]
if len(lines) < 2 or lines[0][:4] != "musl":
return None
m = re.match(r"Version (\d+)\.(\d+)", lines[1])
Copy link
Member

Choose a reason for hiding this comment

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

Worth compiling the regex globally?

Copy link
Member

@pradyunsg pradyunsg Apr 9, 2021

Choose a reason for hiding this comment

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

The compiled versions of the most recent patterns passed to re.match(), re.search() or re.compile() are cached, so programs that use only a few regular expressions at a time needn’t worry about compiling regular expressions.

Meh, it can't hurt but I won't block the PR on that (which, I guess is the same as you).

tests/test_musllinux.py Show resolved Hide resolved
tests/musllinux/build.sh Show resolved Hide resolved
@uranusjr
Copy link
Member Author

Ready if the tests pass!

@brettcannon
Copy link
Member

@pradyunsg did you want to review at all? Otherwise I'm happy to accept this.

@pradyunsg
Copy link
Member

No concerns on merging this without me reviewing.

@brettcannon
Copy link
Member

I just realized it's probably better to wait until https://www.python.org/dev/peps/pep-0656/ is actually accepted. 😄

@MrMino
Copy link
Contributor

MrMino commented Apr 22, 2021

Hey @brettcannon, friendly reminder - PEP-656 is in, is there anything else needed to merge this?

@brettcannon
Copy link
Member

@MrMino CI to pass again with the update to match main, otherwise it should be good to merge!

@h-vetinari
Copy link

@pradyunsg: No concerns on merging this without me reviewing.

Sounds like someone just needs to push the button...? 🙃
Or is this waiting for someone else's review?

@brettcannon brettcannon reopened this Apr 30, 2021
@brettcannon
Copy link
Member

Unfortunately there looks to have been a bad merge from main has distutils is still being shown in tests/test_tags.py.

@brettcannon
Copy link
Member

Actually, not a bad merge; this PR predates the removal of distutils, so now it has a dangling reference to it in a test that needs updating to use sysconfig.

@brettcannon brettcannon self-requested a review April 30, 2021 21:21
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.

There's still a reference to distutils in tests/test_tags.py.

tests/test_tags.py Outdated Show resolved Hide resolved
This is probably slightly faster and more robust than the ldd method
since it does not require ldd on PATH, and avoids some terminal encoding
issues.

The ldd approach is kept in edge cases where the executable is somehow
not readable. I suspect ldd would not be very useful in this scenario
either, but there's not harm being safe?
@brettcannon brettcannon merged commit c32c969 into pypa:main May 3, 2021
@brettcannon
Copy link
Member

Thanks!

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

Successfully merging this pull request may close these issues.

None yet

6 participants