Absorb macos_libfile from pypa/wheel

[abravalheri]

Summary of changes

This implements the approach discussed in https://discuss.python.org/t/extraction-of-deprecated-wheel-macosx-libfile-into-its-own-package/87509/9

An attempt to keep the file's git history was made by following the procedure¹:

cat <<EOF > /tmp/replace.txt
regex:#(\d+)==>pypa/wheel#\1
EOF
git clone --no-tags  https://github.com/pypa/wheel.git
/tmp/export-macos_libfile
cd /tmp/export-macos_libfile
git filter-repo --replace-message /tmp/replace.txt --path
wheel/macosx_libfile.py --path src/wheel/macosx_libfile.py --path-rename
'src/wheel/macosx_libfile.py':'setuptools/command/_macosx_libfile.py'

cd ~/workspace/setuptools
git checkout -b absorb-macos_libfile
git remote add import-macos_libfile /tmp/export-macos_libfile
git fetch import-macos_libfile
git merge --allow-unrelated-histories import-macos_libfile/main

UPDATE: I have redone this using --revision 374c4026acc114a95c77f0529891613973be78c0 for git clone instead of simply getting the file from the main branch to improve the ability of navigating the commit history via git blame.

UPDATE: To include the test files this procedure was revisited in #4965 (comment).

My own authored commits are:

• 545bc41
• fcbc3cb
• 48c2889

Closes #4935

Pull Request Checklist

• Changes have tests
• News fragment added in newsfragments/.
  (See documentation for details)

Footnotes

1. Git history may be important in the future when trying to understand why some particular approach was implemented. ↩

[abravalheri]

Type checkers don't seem very happy about the new file. I am tempted to ignore it for now...

Update: "ignore" annotations and adjustments for type checkers in 48c2889

[abravalheri]

I have noticed that simply importing the file from pypa/wheel@main branch makes it difficult to use git blame to browse through the history of the file.

This is something undesired, as it is important to browse the file history when investigating the code base.

Therefore I re-made the PR using the code available before pypa/wheel#655, using the following commands:

rm -rf /tmp/replace.txt /tmp/export-macos_libfile
cat <<EOF > /tmp/replace.txt
regex:#(\d+)==>pypa/wheel#\1
EOF
git clone --no-tags --revision 374c4026acc114a95c77f0529891613973be78c0 https://github.com/pypa/wheel.git /tmp/export-macos_libfile
cd /tmp/export-macos_libfile
git filter-repo --replace-message /tmp/replace.txt --path wheel/macosx_libfile.py --path src/wheel/macosx_libfile.py --path-rename 'src/wheel/macosx_libfile.py':'setuptools/command/_macosx_libfile.py'
git checkout -b main

cd ~/workspace/setuptools
git checkout absorb-macos_libfile
git reset --hard upstream/main
git remote remove import-macos_libfile


git checkout -b absorb-macos_libfile
git remote add import-macos_libfile /tmp/export-macos_libfile
git fetch import-macos_libfile
git merge --allow-unrelated-histories import-macos_libfile/main

git cherry-pick 1ca55e4 c8625ec

[abravalheri]

Note about licensing: pypa/wheel also uses the MIT license, which is the same as the setuptools license.

So as far as I understand it should be OK.

I will add a licensing comment to the top of the file just in case: 6f2f7e5

[abravalheri]

Tests on PyPy and 3.14 seem to already be broken in main (#4916, pytest-dev/pluggy#573), so they are unrelated to this PR.

[abravalheri]

@dholth, @agronholm, do we have your permission to "transfer" the file from the wheel to the setuptools repo? Is the copyright information I added at the beginning of the file enough for this purpose?

Setuptools also has MIT license so the overall license text at the root of the repo is the same.

[agronholm]

I think what's needed is to retain some information on who committed the original code (@Czaki), as they are the actual copyright holder of at least the majority of the module code.

[abravalheri]

I think what's needed is to retain some information on who committed the original code (@Czaki), as they are the actual copyright holder of at least the majority of the module code.

I tried to keep the git history in this PR, so the information of the commits should be fairly similar to the same information that is currently available on pypa/wheel. git blame should also deliver more or less the same results so it is "traceable".

@Czaki, if you prefer I can modify the file in the following way:

- # This file was originally contributed to the pypa/wheel project under
- # the following license and copyright notice:
+ # This file was originally contributed by Grzegorz Bokota
+ # to pypa/wheel under the following license and copyright notice:
  #
  # MIT License
  #
  # Copyright (c) 2012 Daniel Holth <[email protected]> and contributors

Do you have any thoughts?

[Czaki]

It will be nice to be mentioned.

[abravalheri]

Thank you very much @Czaki, I have added a6dee59.

Please let us know if you would prefer the mention to be something different/in a different way. Also feel free to drop suggestions of modifications directly via GitHub review comments.

[agronholm]

It looks like you'll also need to adopt the test module to make coverage happy.

[jaraco]

Kudos and nice work retaining the commit history.

[Avasam]

Concerning type-checking issues.
It this file is intended to be vendored w/o much future modification. Then it could be excluded from the checks just like any other vendor folders. (in mypy.ini).

I don't wanna cause a change after approval, so just something to consider if it comes up again.

(I haven't looked at the actual type issue either)

[abravalheri]

It looks like you'll also need to adopt the test module to make coverage happy.

Good point! I forgot the test files. I have re-done the filter-repo process to include them

This is the list of commands I used

rm -rf /tmp/replace.txt /tmp/wheel_files.txt /tmp/export-macos_libfile

cat <<EOF > /tmp/replace.txt
regex:#(\d+)==>pypa/wheel#\1
EOF

cat <<EOF > /tmp/wheel_files.txt
wheel/macosx_libfile.py
wheel/macosx_libfile.py==>setuptools/command/_macosx_libfile.py
src/wheel/macosx_libfile.py
src/wheel/macosx_libfile.py==>setuptools/command/_macosx_libfile.py
tests/test_macosx_libfile.py
tests/test_macosx_libfile.py==>setuptools/tests/macosx_libfile/test_macosx_libfile.py
tests/testdata/macosx_minimal_system_version/
tests/testdata/macosx_minimal_system_version/==>setuptools/tests/macosx_libfile/testdata/macosx_minimal_system_version/
EOF

git clone --no-tags --revision 374c4026acc114a95c77f0529891613973be78c0 https://github.com/pypa/wheel.git /tmp/export-macos_libfile
cd /tmp/export-macos_libfile
git filter-repo --paths-from-file /tmp/wheel_files.txt --replace-message /tmp/replace.txt
git checkout -b main


cd ~/workspace/setuptools
git checkout absorb-macos_libfile
git reset --hard upstream/main
git remote remove import-macos_libfile

git checkout -b absorb-macos_libfile
git remote add import-macos_libfile /tmp/export-macos_libfile
git fetch import-macos_libfile
git merge --allow-unrelated-histories import-macos_libfile/main

git cherry-pick 'fcbc3cb69^..a6dee5922'

@jaraco, the tests in the pypa/wheel repository include a bunch of binary test files. I was a bit concerned about including these "pre-compiled" binary files in the final wheel distribution, so I added an extra exclude-package-data in 35a75e8. Please let me know if you have any thoughts or suggestions for that.

[abravalheri]

Concerning type-checking issues.
It this file is intended to be vendored w/o much future modification. Then it could be excluded from the checks just like any other vendor folders. (in mypy.ini).

Hi @Avasam, I think the typing problems (at least with mypy) are most related to the fact that the module dynamically define classes for interoperating with ctypes depending on the endianness and magic numbers in the binary headers... And type checkers are naturally limited when it comes to support dynamic definitions.

Hopefully we would like to not need much further modifications, but we can never know, in the end of the day we will have to maintain the file and apply bugfixes etc.

[jaraco]

@jaraco, the tests in the pypa/wheel repository include a bunch of binary test files. I was a bit concerned about including these "pre-compiled" binary files in the final wheel distribution, so I added an extra exclude-package-data in 35a75e8. Please let me know if you have any thoughts or suggestions for that.

Thanks for flagging that. I'm a little uneasy including those, especially because they present a growing maintenance burden - how are those files generated; how can they be refreshed?

I'm not opposed to including them as-is, but it might be a good opportunity to at least document (perhaps just in this PR) what it takes to build them and maybe open a TODO to build them on demand.

[abravalheri]

Thanks for flagging that. I'm a little uneasy including those, especially because they present a growing maintenance burden - how are those files generated; how can they be refreshed?

I'm not opposed to including them as-is, but it might be a good opportunity to at least document (perhaps just in this PR) what it takes to build them and maybe open a TODO to build them on demand.

That is a very good point. I don't think this procedure is documented in pypa/wheel (or am I wrong about that, @agronholm?).

I am not sure (and also not an macOS daily user), but if I had to guess:

• By reading Better discovery of macos system version to determine proper platform tag wheel#314 I have the impression that these examples were generated by setting up MACOSX_DEPLOYMENT_TARGET variable before and then compiling test_lib.c?

• By having a look on the clang manual pages / guides, I can see some relevant CLI options:

  -mmacos-version-min=<version>
  
      When building for macOS, specify the minimum version supported by your application.
  
  
  -arch <architecture>
  
      Specify the architecture to build for (Mac OS X specific).
  
  -target <architecture>
  
      Specify the architecture to build for (all platforms).
  

  -mmacos-version-min seems to match conceptually the comment in https://github.com/pypa/wheel/blob/0.45.1/tests/test_macosx_libfile.py#L15.

  -arch and -target may also have been involved because the .dylib file names include suffixes like fat, multiple_fat, 386, universal2.

Maybe we can put something together. This is a very first naïve attempt:

cd <...>/testdata/macosx_minimal_system_version

# -o, -mmacosx-version-min, -arch
targets=(
    "test_lib_10_6_fat.dylib 10.6.0 x86_64 i386"
    "test_lib_10_10_fat.dylib 10.10.0 x86_64 i386"
    "test_lib_10_14_fat.dylib 10.14.0 x86_64 i386"
    "test_lib_10_6.dylib 10.6.0 x86_64"
    "test_lib_10_10.dylib 10.10.0 x86_64"
    "test_lib_10_14.dylib 10.14.0 x86_64"
    "test_lib_10_6_386.dylib 10.6.0 i386"
    "test_lib_10_10_386.dylib 10.10.0 i386"
    "test_lib_10_14_386.dylib 10.14.0 i386"
    "test_lib_multiple_fat.dylib 10.14.0 x86_64 arm64"
    "test_lib_10_10_10.dylib 10.10.10 x86_64"
    "test_lib_11.dylib 11.0.0 x86_64"
    "test_lib_10_9_universal2.dylib 10.9.0 x86_64 arm64"
)

for target in "${targets[@]}"; do
    output=$(echo $target | cut -d ' ' -f 1)
    version=$(echo $target | cut -d ' ' -f 2)
    archs=$(echo $target | cut -d ' ' -f 3-)
    arch_flags=$(printf " -arch %s" $archs)
    
    echo "clang -dynamiclib $arch_flags -mmacos-version-min=$version -o $output test_lib.c"
done

This would result in something like:

clang -dynamiclib  -arch x86_64 -arch i386 -mmacos-version-min=10.6.0 -o test_lib_10_6_fat.dylib test_lib.c
clang -dynamiclib  -arch x86_64 -arch i386 -mmacos-version-min=10.10.0 -o test_lib_10_10_fat.dylib test_lib.c
clang -dynamiclib  -arch x86_64 -arch i386 -mmacos-version-min=10.14.0 -o test_lib_10_14_fat.dylib test_lib.c
clang -dynamiclib  -arch x86_64 -mmacos-version-min=10.6.0 -o test_lib_10_6.dylib test_lib.c
clang -dynamiclib  -arch x86_64 -mmacos-version-min=10.10.0 -o test_lib_10_10.dylib test_lib.c
clang -dynamiclib  -arch x86_64 -mmacos-version-min=10.14.0 -o test_lib_10_14.dylib test_lib.c
clang -dynamiclib  -arch i386 -mmacos-version-min=10.6.0 -o test_lib_10_6_386.dylib test_lib.c
clang -dynamiclib  -arch i386 -mmacos-version-min=10.10.0 -o test_lib_10_10_386.dylib test_lib.c
clang -dynamiclib  -arch i386 -mmacos-version-min=10.14.0 -o test_lib_10_14_386.dylib test_lib.c
clang -dynamiclib  -arch x86_64 -arch arm64 -mmacos-version-min=10.14.0 -o test_lib_multiple_fat.dylib test_lib.c
clang -dynamiclib  -arch x86_64 -mmacos-version-min=10.10.10 -o test_lib_10_10_10.dylib test_lib.c
clang -dynamiclib  -arch x86_64 -mmacos-version-min=11.0.0 -o test_lib_11.dylib test_lib.c
clang -dynamiclib  -arch x86_64 -arch arm64 -mmacos-version-min=10.9.0 -o test_lib_10_9_universal2.dylib test_lib.c

I don't know if that is correct, or would produce the intended results. Also it probably depends on the installed versions of the build tools.

@Czaki, it has been a long time you contributed these files, but do you remember what was the procedure that was used for compiling? As Jason pointed out, it would be good if we can document it.

[Czaki]

This one that I added was compiled using MACOSX_DEPLOYMENT_TARGET on intel macbook. But there are older ones also compiled by someone else.

Maybe we can put something together. This is a very first naïve attempt:

Apple had changed the header format, and last time when I tested it, setting low deployment target do not change the header format to the old one (based on my memories).

So generation data during test may not provide files with old header format so not test all code path.

[agronholm]

That is a very good point. I don't think this procedure is documented in pypa/wheel (or am I wrong about that, @agronholm?).

IIRC the reason there are wheel files in the test data is to prevent false negatives if a bug was introduced in wheel that somehow made it generate invalid wheel files. But this was just one way to do it; manually constructing a wheel using ZipFile and adding the relevant RECORD and other files is just as valid (albeit more tedious) an approach.

[abravalheri]

IIRC the reason there are wheel files in the test data is to prevent false negatives if a bug was introduced in wheel that somehow made it generate invalid wheel files. But this was just one way to do it; manually constructing a wheel using ZipFile and adding the relevant RECORD and other files is just as valid (albeit more tedious) an approach.

Thanks @agronholm. Yes I agree that we can construct .whl using ZipFile, but that is a bit different from my original concern. I was thinking more about how to compile the compiled .dylib files. So we don't have much room here, ideally we need to know which procedure to use to compile the .c file in the examples.

[abravalheri]

Thank you @Czaki. Was the procedure you used just setting MACOSX_DEPLOYMENT_TARGET and then running clang? Or is it more complex? I am trying to understand how did we end up with the variations like multiple_fat or fat... Are they obtained via the -arch flag?

---

Apple had changed the header format, and last time when I tested it, setting low deployment target do not change the header format to the old one (based on my memories).

So generation data during test may not provide files with old header format so not test all code path.

If that is the case, this mean that we cannot regenerate these binary files dynamically, @jaraco and we would need to accept them as "magic files" in the PR.
At the same time, maybe it also mean that in the future we may be able to remove certain code paths because they are no longer being produced¹.

Footnotes

1. Even if we still have old sdist in PyPI... The "most general" naming of the wheel files are only relevant when publishing wheels. When it comes to installing wheels to the local machine, we can use a more specific tag, and the installer should not complain as long as it is compatible with the local architecture. ↩

[Czaki]

Thank you @Czaki. Was the procedure you used just setting MACOSX_DEPLOYMENT_TARGET and then running clang? Or is it more complex? I am trying to understand how did we end up with the variations like multiple_fat or fat... Are they obtained via the -arch flag?

If I correctly remember, I compile separate files and the combine them using lipo

If that is the case, this mean that we cannot regenerate these binary files dynamically, @jaraco and we would need to accept them as "magic files" in the PR.
At the same time, maybe it also mean that in the future we may be able to remove certain code paths because they are no longer being produced1.

I will try to find time and check these files. Maybe at least part of them could be generated dynamically.