Skip to content

macosx platform tag do not support minor bugfix release #317

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.

Sign up for GitHub

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
agronholm merged 9 commits into from
Jan 3, 2020
Merged

macosx platform tag do not support minor bugfix release #317

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
3e213af
macosx platform tag do not support minor bugfix release
Czaki Nov 3, 2019
1a907d3
add regression test for minor bugfix release problem
Nov 26, 2019
0dd9f3e
Fix for files without specified minimum version.
Dec 21, 2019
25c8fb6
change test to use full version number
Czaki Dec 28, 2019
80eac7b
change to use 2 version number all time
Czaki Dec 28, 2019
b1f3999
fixed typos indicated by YannickJadoul
Jan 3, 2020
496c5c6
remove unused assignment
Jan 3, 2020
b66add5
change message for to low MACOSX_DEPLOYMENT_TARGET to more verbose
Jan 3, 2020
0570084
change parse_version to be more clear.
Jan 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 35 additions & 15 deletions tests/test_macosx_libfile.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,31 +6,35 @@
from wheel.pep425tags import get_platform


def test_read_from_dynlib():
def test_read_from_dylib():
dirname = os.path.dirname(__file__)
dylib_dir = os.path.join(dirname, "testdata",
"macosx_minimal_system_version")
versions = [
("test_lib_10_6_fat.dylib", "10.6"),
("test_lib_10_10_fat.dylib", "10.10"),
("test_lib_10_14_fat.dylib", "10.14"),
("test_lib_10_6.dylib", "10.6"),
("test_lib_10_10.dylib", "10.10"),
("test_lib_10_14.dylib", "10.14"),
("test_lib_10_6_386.dylib", "10.6"),
("test_lib_10_10_386.dylib", "10.10"),
("test_lib_10_14_386.dylib", "10.14"),
("test_lib_multiple_fat.dylib", "10.14")
("test_lib_10_6_fat.dylib", "10.6.0"),
("test_lib_10_10_fat.dylib", "10.10.0"),
("test_lib_10_14_fat.dylib", "10.14.0"),
("test_lib_10_6.dylib", "10.6.0"),
("test_lib_10_10.dylib", "10.10.0"),
("test_lib_10_14.dylib", "10.14.0"),
("test_lib_10_6_386.dylib", "10.6.0"),
("test_lib_10_10_386.dylib", "10.10.0"),
("test_lib_10_14_386.dylib", "10.14.0"),
("test_lib_multiple_fat.dylib", "10.14.0"),
("test_lib_10_10_10.dylib", "10.10.10")
]
for file_name, ver in versions:
extracted = extract_macosx_min_system_version(
os.path.join(dylib_dir, file_name)
)
str_ver = str(extracted[0]) + "." + str(extracted[1])
str_ver = ".".join([str(x) for x in extracted])
assert str_ver == ver
assert extract_macosx_min_system_version(
os.path.join(dylib_dir, "test_lib.c")
) is None
assert extract_macosx_min_system_version(
os.path.join(dylib_dir, "test_lib.c")
) is None
assert extract_macosx_min_system_version(
os.path.join(dylib_dir, "libb.dylib")
) is None


def return_factory(return_val):
Expand Down Expand Up @@ -95,6 +99,22 @@ def test_bump_platform_tag_by_env_variable(self, monkeypatch, capsys):
captured = capsys.readouterr()
assert captured.err == ""

def test_bugfix_release_platform_tag(self, monkeypatch, capsys):
dirname = os.path.dirname(__file__)
dylib_dir = os.path.join(dirname, "testdata", "macosx_minimal_system_version")
monkeypatch.setattr(distutils.util, "get_platform", return_factory("macosx-10.9-x86_64"))
monkeypatch.setattr(os, "walk", return_factory(
[(dylib_dir, [], ["test_lib_10_6.dylib", "test_lib_10_6_fat.dylib",
"test_lib_10_10_10.dylib"])]
))
assert get_platform(dylib_dir) == "macosx_10_10_x86_64"
captured = capsys.readouterr()
assert "This wheel needs higher macosx version" in captured.err
monkeypatch.setenv("MACOSX_DEPLOYMENT_TARGET", "10.9")
assert get_platform(dylib_dir) == "macosx_10_10_x86_64"
captured = capsys.readouterr()
assert "This wheel needs higher macosx version" in captured.err

def test_warning_on_to_low_env_variable(self, monkeypatch, capsys):
dirname = os.path.dirname(__file__)
dylib_dir = os.path.join(dirname, "testdata", "macosx_minimal_system_version")
Expand Down
Binary file added tests/testdata/macosx_minimal_system_version/libb.dylib
View file
Open in desktop
Binary file not shown.
Binary file added tests/testdata/macosx_minimal_system_version/test_lib_10_10_10.dylib
View file
Open in desktop
Binary file not shown.
24 changes: 11 additions & 13 deletions wheel/pep425tags.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,15 +115,15 @@ def calculate_macosx_platform_tag(archive_root, platform_tag):
"""
prefix, base_version, suffix = platform_tag.split('-')
base_version = tuple([int(x) for x in base_version.split(".")])
if len(base_version) == 2:
base_version = base_version + (0,)
if len(base_version) >= 2:
base_version = base_version[0:2]

assert len(base_version) == 3
assert len(base_version) == 2
if "MACOSX_DEPLOYMENT_TARGET" in os.environ:
deploy_target = tuple([int(x) for x in os.environ[
"MACOSX_DEPLOYMENT_TARGET"].split(".")])
if len(deploy_target) == 2:
deploy_target = deploy_target + (0,)
if len(deploy_target) >= 2:
deploy_target = deploy_target[0:2]

@YannickJadoul YannickJadoul Dec 28, 2019

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?

@YannickJadoul YannickJadoul Dec 28, 2019

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?

@joerick joerick Dec 29, 2019

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 that this option is only valid with major and a minor version numbers

Apologies for old docs, it's all I can find atm
https://developer.apple.com/library/archive/documentation/DeveloperTools/Reference/XcodeBuildSettingRef/1-Build_Setting_Reference/build_setting_ref.html#//apple_ref/doc/uid/TP40003931-CH3-SW129

@YannickJadoul YannickJadoul Dec 29, 2019

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.

wheel/wheel/macosx_libfile.py

Lines 345 to 350 in fcd78f3

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.

@joerick joerick Jan 1, 2020

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.

@YannickJadoul YannickJadoul Jan 2, 2020

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 :-)

if deploy_target < base_version:
sys.stderr.write(
"[WARNING] MACOSX_DEPLOYMENT_TARGET is set "
Expand All @@ -132,24 +132,22 @@ def calculate_macosx_platform_tag(archive_root, platform_tag):
else:
base_version = deploy_target

assert len(base_version) == 3
assert len(base_version) == 2
start_version = base_version
versions_dict = {}
for (dirpath, dirnames, filenames) in os.walk(archive_root):
for filename in filenames:
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)

@joerick joerick Dec 27, 2019

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]

@Czaki Czaki Dec 28, 2019

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.

@joerick joerick Dec 29, 2019

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.

👍

if min_ver is not None:
versions_dict[lib_path] = min_ver[0:2]

if len(versions_dict) > 0:
base_version = max(base_version, max(versions_dict.values()))

if base_version[-1] == 0:
fin_base_version = base_version[:-1]
else:
fin_base_version = base_version

fin_base_version = "_".join([str(x) for x in fin_base_version])
# macosx platform tag do not support minor bugfix release
fin_base_version = "_".join([str(x) for x in base_version])
if start_version < base_version:
problematic_files = [k for k, v in versions_dict.items() if v > start_version]
problematic_files = "\n".join(problematic_files)
Expand Down