Skip to content

utils/pypi: ensure pure Python wheels support py3 #19984

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

Merged
merged 8 commits into from
Jul 16, 2025

Conversation

dtrodrigues
Copy link
Member

@dtrodrigues dtrodrigues commented May 21, 2025

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?

Supports Homebrew/homebrew-core#224034 . Without this, there's the possibility of picking up a py2-none-any.whl when a py3-none-any.whl is available, eg for https://pypi.org/project/aenum/3.1.16/#files .

@p-linnane p-linnane requested a review from woodruffw May 21, 2025 06:02
@Bo98
Copy link
Member

Bo98 commented May 27, 2025

This might need to also accept e.g. py313.

Technically py3.py2 is also valid - though not sure if any such actually exist. https://blog.yossarian.net/2024/06/12/Python-wheel-filenames-have-no-canonical-form

@woodruffw
Copy link
Member

Yeah, technically there are all kinds of compressed and minor version variants here that would be good to handle -- one "good enough" approach would probably be to check that the Python tag is not just py2, i.e. allow anything so long as some form of py3* is present in the Python tag component.

This is a good candidate for factoring out more generally, since there's a place in language/python.rb where we do the same suffix check:

target /= t.downloader.basename if t.url&.end_with?("-none-any.whl")

@dtrodrigues
Copy link
Member Author

@woodruffw @Bo98 Does the updated regex of py3.*-none-any.whl$ look reasonable?

@woodruffw
Copy link
Member

Does the updated regex of py3.*-none-any.whl$ look reasonable?

Looks pretty good to me, although I think someone could contrive a wheel that incorrectly matches it like so:

py3.foobar-1.2.3-py2-none-any.whl

(This is technically invalid because py3.foobar should be normalized to py3_foobar, but Python packaging as a whole is still permissive of these kinds of non-normalized names. It's also arguably pretty niche, since we're dealing with a priori trusted deps here.)

In terms of locking it down, my suggestion would be:

^.+-py3.*-none-any\.whl$

@Bo98
Copy link
Member

Bo98 commented Jun 6, 2025

py3[^-]* maybe?

@woodruffw
Copy link
Member

py3[^-]* maybe?

No harm in that, yeah -- technically we should never see that since PyPI would reject py3-blah-none-any.whl as malformed, but it doesn't hurt to be precise here 🙂

@T0BEY17

This comment was marked as spam.

@Bo98
Copy link
Member

Bo98 commented Jun 8, 2025

I think we want to also match -py2.py3-none-any.whl so ^.+-py3.*-none-any\.whl$ would be too restrictive for that.

[.-]py3[^-]*-none-any\.whl$ seems like it would work

Copy link

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@github-actions github-actions bot added the stale No recent activity label Jul 16, 2025
@woodruffw woodruffw removed the stale No recent activity label Jul 16, 2025
@woodruffw
Copy link
Member

Sorry for the delay here @dtrodrigues!

I just went back and confirmed this works as expected on the various patterns we want to support here:

Correct matches:

aenum-3.1.16-py3-none-any.whl
aenum-3.1.16-py2.py3-none-any.whl 
aenum-3.1.16-py3.py2-none-any.whl 
aenum-3.1.16-py313-none-any.whl 
aenum-3.1.16-py313.py2-none-any.whl 

Correct non-matches:

aenum-3.1.16-py2-none-any.whl

@woodruffw woodruffw added the python Homebrew/brew's python support label Jul 16, 2025
Copy link
Member

@woodruffw woodruffw left a comment

Choose a reason for hiding this comment

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

Two small escapes needed:

@MikeMcQuaid
Copy link
Member

Two small escapes needed:

@woodruffw not according to rubocop 😁

@woodruffw
Copy link
Member

@woodruffw not according to rubocop 😁

Yeah, it'd be great if it could catch that...I think the reason it doesn't flag that is because . could be what the user intended, so it's hard to suggest \. universally.

(I guess we could maybe add a an extra cop for that, but it would have a lot of false positives, e.g. on the earlier part of that same regex 😅)

@woodruffw woodruffw added this pull request to the merge queue Jul 16, 2025
Merged via the queue into Homebrew:main with commit 4631429 Jul 16, 2025
62 of 68 checks passed
@Bo98
Copy link
Member

Bo98 commented Jul 16, 2025

That'll be because "\." goes through Ruby string-escaping rules rather than regex rules will translate to "."

You want /\./ instead.

@woodruffw
Copy link
Member

Ah right, I missed that these were quoted strings and not regexp objects. I'll send a PR fixing that (and yeah, that would indeed be a good cop then -- my bad!)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
python Homebrew/brew's python support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants