Better discovery of macos system version to determine proper platform tag#314
Conversation
| pos = lib_file.tell() | ||
| segment_base = read_data(SegmentBase, lib_file) | ||
| lib_file.seek(pos) | ||
| if segment_base.cmd == LC_VERSION_MIN_MACOSX: |
There was a problem hiding this comment.
Is it guaranteed that LC_VERSION_MIN_MACOSX always appears before LC_BUILD_VERSION?
There was a problem hiding this comment.
They are disjoint. form macosx version 10.8 to 10.13 there is LC_VERSION_MIN_MACOSX section and since 10.14 (current) there is LC_BUILD_VERSION section.
As you can see both contains return statement so if function found one of them then it finish.
Codecov Report
@@ Coverage Diff @@
## master #314 +/- ##
==========================================
+ Coverage 59.71% 64.64% +4.92%
==========================================
Files 12 13 +1
Lines 844 987 +143
==========================================
+ Hits 504 638 +134
- Misses 340 349 +9
Continue to review full report at Codecov.
|
…on and description on begin of Mach-O parser
| if base_version[-1] == 0: | ||
| base_version = base_version[:-1] | ||
| base_version = "_".join([str(x) for x in base_version]) | ||
| fin_base_version = base_version[:-1] |
There was a problem hiding this comment.
Looks like fin_base_version is undefined if the if-condition is not true. This would break the code below, right? Is there any reason why you try to support x.y.z instead of just x.y?
There was a problem hiding this comment.
I use x.y.z because this is format used inside libraries header. This is 190 line of file lib_file_analyse.py
uint32_t version; /* X.Y.Z is encoded in nibbles xxxx.yy.zz */
I push fix for undefined variable.
Codecov Report
@@ Coverage Diff @@
## master #314 +/- ##
==========================================
+ Coverage 59.71% 64.89% +5.17%
==========================================
Files 12 13 +1
Lines 844 997 +153
==========================================
+ Hits 504 647 +143
- Misses 340 350 +10
Continue to review full report at Codecov.
|
|
All improvements comes from request from the discussion in matthew-brett/delocate#56 |
agronholm
left a comment
agronholm
left a comment
There was a problem hiding this comment.
Code style changes required. As for the actual logic, I am not qualified to review that.
| dirname = os.path.dirname(__file__) | ||
| dylib_dir = os.path.join(dirname, "testdata", | ||
| "macos_minimal_system_version") | ||
| with mocker.patch("distutils.util.get_platform", return_value="macosx-10.14-x86_64"): |
There was a problem hiding this comment.
If you're using mocks in with blocks, what's the point of using mocker in the first place?
There was a problem hiding this comment.
I'm learning how to use mock. If I good understand in python 2.7 there is preference to use mock library and in python 3.4+ use unittests.mock.
And I do not have idea how test this functions without mock.
How to fix it in best way. change dependency on mock. Do some if depends on python version?
There was a problem hiding this comment.
Python 2.6 did not have unittest.mock. But Python 2.7 does. The mocker fixture, on the other hand, works like pytest's built-in monkeypatch fixture in that it automatically undoes all the mocks that it sets up during the test request.
There was a problem hiding this comment.
I rewrite tests to base on monkeypatch. First I think that it is not enough, but maybe I better understand it.
|
Close enough. Now I'd like to find at least one qualified reviewer for the actual business logic. |
|
A moment ago I realized that I'm not sure if my change do not break My changes in Could you check it? |
|
Okay, I'll check. |
|
The way I see it is that if someone has explicitly set a platform tag starting with |
|
It not fix the poroblem. From the comment on begin of And I check that on begin of Maybe finally some patch should be applied to python, but this take very long time (waiting on updates on CI servers, distribution repos etc.) and fixes of wheel package can be used earlier. |
|
@agronholm I found that if user set platform name by cli argument then |
|
Did I need introduce any changes? |
|
No, I've been on holiday for some days and I will review this shortly. Would be nice to also get a statement from someone who understands macOS binary compatibility. |
|
I can send you macos headers (*.h) on which I base during writing. |
|
I honestly don't think they would help as I have no expertise on macOS specific development. I'll ask on the distutils-sig mailing list and see if I can get someone with that expertise to review this. Other than that I have no objections to merging this. |
|
Thank you! Now let's just hope that the (mac) world doesn't burn because of this 😄 |
|
Could you inform me when new release was created? I wait for this to update pypa/cibuildwheel#156 |
|
I'll let you know once I've made a new release. |
|
Just a question: any idea or plan when this new version of wheel would be released? |
|
I'm waiting for PR #317 to be finished so I can merge it. I don't want to ship this feature in its current state. |
|
Thanks! I had no clue that PR wasn't ready yet. But good to know this is in @Czaki's hands, then; he's also the author of the PR on the |
|
@YannickJadoul Could you try to review all changes? (both PR). I testing it in my projects and found two bugs (fixed in PR #317) but maybe it is better to someone with more macos experience take a look on it. @agronholm In current time I do not see any problem in PR #317 but I learn MacOs on example, so sometimes something surprise me. In this moment I do not have plans for next commits, but I use modified version of wheel in projects to find any problems. |
Your last message ("I cannot create proper test file but I found such in delocate package. I add missed test case.") indicates that you plan to add a test. Is this not correct? |
|
@agronholm I add it in this commit 0dd9f3e |
|
I see. This is why the correct tense of the verb is important! I will review your PR tomorrow. |
I'm happy to take a look. Which PRs do you mean? |
|
@YannickJadoul In both (This and #317) In file https://github.com/Czaki/wheel/blob/master/wheel/pep425tags.py#L112 If you have time to check if logic is proper and all most important cases are covered. @agronholm Sorry. I need to remember to check text before accept comment. |
There was a problem hiding this comment.
@Czaki If you wanted some comments on this, here you go. I don't know enough about these mac headers to say a lot about that part, and for the rest, these are mainly some suggestions and thoughts.
The only two things I'm sure of could be changed are some details in the two messages.
EDIT: If you prefer for me to do it, I can also just create a PR that fixes the typos?
| return (((x << 24) & 0xFF000000) | | ||
| ((x << 8) & 0x00FF0000) | | ||
| ((x >> 8) & 0x0000FF00) | | ||
| ((x >> 24) & 0x000000FF)) |
There was a problem hiding this comment.
Using struct might be shorter/easier? It's also closely related to what you're trying to do (fix endianess). (cfr. https://stackoverflow.com/a/27506692)
| and if apple introduce some changes both implementation will need to be updated. | ||
| """ | ||
|
|
||
| import ctypes |
There was a problem hiding this comment.
Might not be worth changing everything, but isn't there also the struct built-in library that makes readying binary data easy? (See https://stackoverflow.com/a/52035410 for a discussion on struct vs. ctypes.) So struct.unpack(...) might be slightly shorter to use. But again, probably not worth changing.
There was a problem hiding this comment.
I decide to use ctypes because I can use types like uint8. Struct library has only standard sizes (long, etc), not fixed sizes. And apple use fixed sizes in header.
There was a problem hiding this comment.
Hmm, OK. But it seems that these standard sizes can be enforced:
Note
By default, the result of packing a given C struct includes pad bytes in order to maintain proper alignment for the C types involved; similarly, alignment is taken into account when unpacking. This behavior is chosen so that the bytes of a packed struct correspond exactly to the layout in memory of the corresponding C struct. To handle platform-independent data formats or omit implicit pad bytes, use standard size and alignment instead of native size and alignment: see Byte Order, Size, and Alignment for details.
https://docs.python.org/3/library/struct.html (and even already on Python 2 https://docs.python.org/2/library/struct.html).
But having uint8 might indeed be cleaner. Just thought I'd mention it :-)
| problematic_files = "\n".join(problematic_files) | ||
| error_message = \ | ||
| "[WARNING] This wheel needs higher macosx version than {} " \ | ||
| "is set in MACOSX_DEPLOYMENT_TARGET variable. " \ |
There was a problem hiding this comment.
Forgot to remove this line, after adding the format, later?
There was a problem hiding this comment.
I do not understand. I use this text in format.
There was a problem hiding this comment.
Depending on if "MACOSX_DEPLOYMENT_TARGET" in os.environ, it can be that "is set in MACOSX_DEPLOYMENT_TARGET variable." will be twice in the string, since you format it later and it is already in the format string.
|
|
||
|
|
||
| def parse_version(version): | ||
| zz = version & 2**9-1 |
There was a problem hiding this comment.
Same here; máybe this is easier with struct?
There was a problem hiding this comment.
I try to do this with struct but it is not intuitive for me. You have some suggestion how such code should look?
There was a problem hiding this comment.
Yeah, OK, I forgot the thing was already converted into an int, and not bytes anymore.
This works, but it's not better, I think: struct.unpack('>HBB', struct.pack('>I', 0x000a0e01)). Not worth using if struct is not used anywhere else, I'd say :-)
There was a problem hiding this comment.
Still thinking, though, because the current code confuses me a bit. Is this cleaner? I don't know, up to you, and just close if you like your solution better:
def parse_version(version):
x = (version & 0xffff0000) >> 16
y = (version & 0x0000ff00) >> 8
z = (version & 0x000000ff)
return x, y, z
There was a problem hiding this comment.
It is much cleaner. Thanks.
5 participants
In this moment wheel package create python wheel with same platform tag like python was compiled. But wheel may contain libraries which needs higher macos version. In this patch I add code which scan all files added to wheel and if their suffix suggest that it is library (
*.soor*.dynlib) and then try to determine macos version by scan Mach-O header. This code is pure python and do not depends on external files. The struct in comments are copied from Apple heade files.To manually check library needs use
otoolon file. and look onLC_BUILD_VERSIONorLC_VERSION_MIN_MACOSXsection.To set macosx minimum version set environment variable
MACOSX_DEPLOYMENT_TARGETbefore compile. If compilation is launched withpython setup.py ...then the version from variable need to be higher or equal than version returned bydistutils.util.get_platform()travis ci fail is connected with python 2.7.3 and occurs also in original data. It is not connected with my changes.