Skip to content

macosx platform tag do not support minor bugfix release#317

Merged
agronholm merged 9 commits into
pypa:masterfrom
Czaki:master
Jan 3, 2020
Merged

macosx platform tag do not support minor bugfix release#317
agronholm merged 9 commits into
pypa:masterfrom
Czaki:master

Conversation

@Czaki

@Czaki Czaki commented Nov 3, 2019

Copy link
Copy Markdown
Contributor

I found that python does not support information about bugfix release of macosx, so adding information about third nonzero value from tuple version create wheel which can not be installed. Example here. The example is created with MACOSX_DEPLOYMENT_TARGET setted to "10.9".

Then I think that removing this information is best idea. Then it may occurs that platform tag suggest that wheel should run, but increasing major version of system is bad idea for me.

@codecov

codecov Bot commented Nov 3, 2019

Copy link
Copy Markdown

Codecov Report

Merging #317 into master will increase coverage by 0.24%.
The diff coverage is 95.23%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #317      +/-   ##
==========================================
+ Coverage   64.89%   65.13%   +0.24%     
==========================================
  Files          13       13              
  Lines         997     1001       +4     
==========================================
+ Hits          647      652       +5     
+ Misses        350      349       -1
Impacted Files Coverage Δ
wheel/macosx_libfile.py 92.23% <100%> (-0.5%) ⬇️
wheel/pep425tags.py 51.28% <93.75%> (+0.95%) ⬆️
wheel/metadata.py 98.86% <0%> (+1.36%) ⬆️

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 9b416dd...0570084. Read the comment docs.

@agronholm

Copy link
Copy Markdown
Contributor

The tests passed before though – is there any way to add a regression test for this?

@Czaki Czaki force-pushed the master branch 2 times, most recently from f30ecb3 to 1a907d3 Compare November 26, 2019 11:21
@Czaki

Czaki commented Nov 26, 2019

Copy link
Copy Markdown
Contributor Author

Ok. I build proper dylib file and add tests for regression.

test file is from delocate project
@Czaki

Czaki commented Dec 21, 2019

Copy link
Copy Markdown
Contributor Author

I found that during refactor of code I remove handling of None return value from extract_macosx_min_system_version function.

I cannot create proper test file but I found such in delocate package. I add missed test case.

@joerick joerick left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hello @agronholm and @Czaki , I took a look at this just now and have a few comments - I hope it's helpful!

Comment thread tests/test_macosx_libfile.py Outdated
Comment thread tests/test_macosx_libfile.py Outdated
Comment thread wheel/pep425tags.py
if filename.endswith('.dylib') or filename.endswith('.so'):
lib_path = os.path.join(dirpath, filename)
versions_dict[lib_path] = extract_macosx_min_system_version(lib_path)
min_ver = extract_macosx_min_system_version(lib_path)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think the last field of the version tuple is irrelevant to the compatibility question? If so, should it be moved up here i.e. min_ver = min_ver[0:2]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It need changes in more places. I push this changes in separate commit.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍

@agronholm

Copy link
Copy Markdown
Contributor

@Czaki so do you consider this ready for merging or do you need to make more changes still?

Comment thread wheel/pep425tags.py
if len(deploy_target) == 2:
deploy_target = deploy_target + (0,)
if len(deploy_target) >= 2:
deploy_target = deploy_target[0:2]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

So the information we lose here is not important? There's never an important difference between e.g. 10.14.0 and 10.14.2?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

So I'm wondering if we should work with full 3-part versions to make all checks, and only cut off the last version later, when creating the tag string?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

OK, yes, the documentation on this is so crappy that except for that old version of Apple's docs, all I can seem to find is a bunch of SO questions and issues/PRs on GitHub :-|

I was mainly looking at https://stackoverflow.com/a/25362535 where 10.3.9 is mentioned, but I can't find that anywhere else (also tried the man page on a mac I have access to; doesn't show the same message anymore).

It's also quite funny that the versions in the dylibs are stored as x.y.z, then? (cfr.

def parse_version(version):
zz = version & 2**9-1
version >>= 8
yy = version & 2**9-1
version >>= 8
return version, yy, zz
)

At any rate, maybe the value from the environment variable should be checked, then, to make sure it only contains 2 or 3 parts and not more or less? This is some for of user input, after all.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Whether or not the bugfix might contain relevant information, the platform_tag in Python considers all bugfix releases within the same minor release of macOS as equivalent.

I tend to agree with this, FWIW. I ran a regex on the macOS 10.15 SDK and I can't find any APIs that specify a macOS version to the bugfix release.

Speculating, I think that sometimes Apple do bugfix releases to target new hardware, and those would require such a detailed version string. But I don't think it's used when building against the public SDK.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Whether or not the bugfix might contain relevant information, the platform_tag in Python considers all bugfix releases within the same minor release of macOS as equivalent.

OK, nice find! Better ignore it then, and have a bit of consistency :-)

@xavfernandez

Copy link
Copy Markdown
Member

Please note that some python packaging projects (like pip) are planning to (or already) use https://github.com/pypa/packaging/blob/master/packaging/tags.py so most improvements would need to be replicated there.

@agronholm

Copy link
Copy Markdown
Contributor

I would be thrilled if I could throw out the wheel.tags module, but IIRC there were some differences that made that impossible. I should revisit this idea some time.

@Czaki

Czaki commented Jan 3, 2020

Copy link
Copy Markdown
Contributor Author

Last 6 days I was out of internet. Thanks @YannickJadoul for review and especially for language mistakes.

@agronholm In this moment I do not see any contradictions to merge. The only thing about I think is if using struct (suggested by @YannickJadoul) can simplify code and simplify maintaining.

@agronholm agronholm merged commit 882650d into pypa:master Jan 3, 2020
@agronholm

Copy link
Copy Markdown
Contributor

Thanks!

abravalheri pushed a commit to abravalheri/setuptools that referenced this pull request Apr 28, 2025
abravalheri pushed a commit to abravalheri/setuptools that referenced this pull request Apr 30, 2025
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.

5 participants