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

fix PyPy SOABI parsing #484

Merged
merged 4 commits into from
Nov 2, 2022
Merged

fix PyPy SOABI parsing #484

merged 4 commits into from
Nov 2, 2022

Conversation

mattip
Copy link
Contributor

@mattip mattip commented Nov 2, 2022

In #373 I started fixing the SOABI parsing to trim off the platform tag from the ABI tag. I used the wrong logic: the soabi.startswith("pypy-") would not be True. This means the code on line 96 was used for PyPy. No harm was done, but if PyPy updates its SOABI tag to fix this issue, the fix here will be needed.

Edit: to be clear: the SOABI is currently pypy36-pp73, which does not start with pypy-

@agronholm
Copy link
Contributor

If the logic was indeed wrong and this was not caught by the test suite, how do I know future refactorings won't break it?

@codecov
Copy link

codecov bot commented Nov 2, 2022

Codecov Report

Base: 66.85% // Head: 67.70% // Increases project coverage by +0.85% 🎉

Coverage data is based on head (b76f509) compared to base (7627548).
Patch coverage: 75.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #484      +/-   ##
==========================================
+ Coverage   66.85%   67.70%   +0.85%     
==========================================
  Files          12       12              
  Lines         902      901       -1     
==========================================
+ Hits          603      610       +7     
+ Misses        299      291       -8     
Impacted Files Coverage Δ
src/wheel/bdist_wheel.py 50.19% <75.00%> (+2.90%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@mattip
Copy link
Contributor Author

mattip commented Nov 2, 2022

Good question. I will add tests to test_bdist_wheel.

@agronholm agronholm merged commit 747e1f6 into pypa:main Nov 2, 2022
@agronholm
Copy link
Contributor

Thanks!

@@ -69,8 +68,8 @@ def get_flag(var, fallback, expected=True, warn=True):


def get_abi_tag():
"""Return the ABI tag based on SOABI (if available) or emulate SOABI (PyPy)."""
soabi = get_config_var("SOABI")
"""Return the ABI tag based on SOABI (if available) or emulate SOABI (PyPy2)."""
Copy link
Contributor

Choose a reason for hiding this comment

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

(not really related, just noticed this) Is PyPy2 really still supported by wheel? I thought it requires Python 3.7+?

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 think all platforms these days have a valid SOABI, even windows. So "PyPy2" is more of a generic placeholder for "weird platform".

agronholm added a commit that referenced this pull request Nov 4, 2022
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.

3 participants