takluyver / pynsist

Build Windows installers for Python applications
https://pynsist.readthedocs.io/
Other
930 stars 123 forks source link

Handle properly compatibility for wheels of ABI type "abi3" #222

Closed adferrand closed 3 years ago

adferrand commented 3 years ago

Wheels whose ABI tag is abi3 are wheels compiled against the Stable Application Binary Interface: with dedicated compiler tags set, a given non-universal wheel is ensured to work on a wide range of Python version for the target platform (this mechanism is available on Linux and Windows).

By default the compatibility is provided for Python 3.2+. However a developer can decide to compile against a more up-to-date set of a stable interfaces that are available only starting to a more recent version of Python. In this case, the interpreter flag located before the ABI tag is set to that version of Python (eg. cp36 for Python 3.6).

Finally a tag like cp36-abi3 must be read as: for the specified platform, this non-universal wheel is compatible with all Python 3.6+ versions.

See https://docs.python.org/3/c-api/stable.html and https://www.python.org/dev/peps/pep-0425/

This approach is taken by the cryptography project, which exposes (since version 3.3) one unique abi3 compatible wheel for each Windows platform: cryptography-X.X-cp36-abi3-win32.whl and cryptography-X.X-cp36-abi3-win_amd64.whl.

Currently pynsist does not take this into account, and always reads the interpreter flag as a strict requirement for the Python version (eg. cp36 gives a compatibility for Python 3.6 only).

My PR modifies the CompatibilityScorer class to take this specification into account: when the ABI tag is abi3, the interpreter tag is evaluated as a lower bound for the Python version instead of a strict requirement. I also calculate a score that will favor the most recent Python version propose amoung the wheels (eg. cp38-abi3 will be prefered over cp36-abi3 if these two wheels exist for a package).

Basic unit tests are also provided.

adferrand commented 3 years ago

NB: My PR makes a wheel named wheel-XX-py3-abi3-win32.whl invalid. Given the specifications and the few examples I have, it does not make sense to have py3 as an interpreter flag for an ABI3 wheel, since it is not universal and contains C bindings. Then only valid interpreters in this context are cpXX.

codecov[bot] commented 3 years ago

Codecov Report

Merging #222 (73cbd1e) into master (89431e7) will increase coverage by 0.08%. The diff coverage is 93.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #222      +/-   ##
==========================================
+ Coverage   88.24%   88.32%   +0.08%     
==========================================
  Files           8        8              
  Lines         791      805      +14     
==========================================
+ Hits          698      711      +13     
- Misses         93       94       +1     
Impacted Files Coverage Δ
nsist/wheels.py 90.67% <93.33%> (+0.13%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 89431e7...73cbd1e. Read the comment docs.

adferrand commented 3 years ago

Hello @takluyver! Just to ping you about my PR, if you would find the time to review it :)

takluyver commented 3 years ago

Hi @adferrand, thanks for working on this, and sorry for not responding sooner.

I've chosen to copy some logic from the packaging.tags module (in PR #227), because the consensus seems to be that that package has the definitive answers in the absence of a spec properly defining what tags are compatible with what platforms. PEP 425 defines the format of tags and how they're meant to be used, but doesn't go into details on compatibility.

I've opted for that approach in the hope that it will avoid similar problems in the future. It appears that my approach of checking compatibility for the three tag parts (interpreter, ABI, platform) separately was misguided - the allowable values for one part can depend on the values in other parts. So I hope you don't mind me closing this PR as superseded. :slightly_smiling_face:

adferrand commented 3 years ago

Yes of course, since you found an implementation that constitutes the effective reference, it will be better than my (or any other) assumptions based on the general statements from PEPs or user documentations.

I have one question however regarding the precise range of a compatibility for abi3 tagged wheels. For the binary interface indeed, it is quite clear that the ABI3 ensures a stable interface up to Python 3.2, as for Linux with manylinux. However, does it mean that the Python code of the module is still compatible with Python 3.2? For instance, cryptography dropped support for older Python versions, and declares wheels tagged as cp36-abi3. Does it mean that pynsist, for a build targeting Python 3.5 (if it would make sense with an EOL version), should accept this wheel as a valid one?

takluyver commented 3 years ago

No, a wheel tagged cp36-abi3 should only be accepted on (C)Python 3.6 and above. The abi3 tag by itself could be compatible with any Python from 3.2, but the cp36 part additionally means that the package is only for 3.6 onwards.

That rule is handled here in Pynsist:

https://github.com/takluyver/pynsist/blob/d3af32159cfba6eb7f09a44758dec451324149a7/nsist/wheels.py#L409-L412

If we're building for Python 3.5, that should yield tags cp35, cp34, cp33, cp32 as acceptable (in combination with abi3). Since cp36 isn't there, the cp36-abi3 wheel should be rejected as incompatible. If it's accepted for Python 3.5, that's a bug.

Here's the corresponding code in packaging.tags, for reference: https://github.com/pypa/packaging/blob/b5878c977206f60302536db969a8cef420853ade/packaging/tags.py#L242-L248

adferrand commented 3 years ago

Ok got it, the function is returning the list of acceptable wheels tags to search for. I was not reading the meaning of this output properly. Thanks a lot, it is clear and totally consistent for me now!