search

matthew-brett / delocate

Find and copy needed dynamic libraries into python wheels
BSD 2-Clause "Simplified" License
262 stars 59 forks source link

DelocationError instead of automatically updating MacOS version of wheel #211

Closed HexDecimal closed 3 months ago

HexDecimal commented 3 months ago

The MacOS comparability code added in #198 might be too strict. I had expected Delocate to derive the version automatically from its dependencies unless an explicit target version is given. In this case, MACOSX_DEPLOYMENT_TARGET is used as an explicit version when it's set and some tool has set it upstream when I didn't expect it, so it's easy to get an error such as this:

delocate-wheel --require-archs x86_64 -w /private/var/folders/24/8k48jl6d249_n_qfxwsl6xvm0000gn/T/cibw-run-ddamarri/pp38-macosx_x86_64/repaired_wheel -v /private/var/folders/24/8k48jl6d249_n_qfxwsl6xvm0000gn/T/cibw-run-ddamarri/pp38-macosx_x86_64/built_wheel/tcod-16.2.3.dev21+g62074a7-pp38-pypy38_pp73-macosx_10_9_x86_64.whl
  INFO:delocate.delocating:Copying library /Users/runner/work/python-tcod/python-tcod/dependencies/SDL2-2.28.1/SDL2.framework/Versions/A/SDL2 to tcod/.dylibs/SDL2
  INFO:delocate.delocating:Modifying install name in tcod/_libtcod.pypy38-pp73-darwin.so from @rpath/SDL2.framework/Versions/A/SDL2 to @loader_path/.dylibs/SDL2
  Fixing: /private/var/folders/24/8k48jl6d249_n_qfxwsl6xvm0000gn/T/cibw-run-ddamarri/pp38-macosx_x86_64/built_wheel/tcod-16.2.3.dev21+g62074a7-pp38-pypy38_pp73-macosx_10_9_x86_64.whl
  Traceback (most recent call last):
    File "/private/var/folders/24/8k48jl6d249_n_qfxwsl6xvm0000gn/T/cibw-run-ddamarri/pp38-macosx_x86_64/build/venv/bin/delocate-wheel", line 8, in <module>
      sys.exit(main())
    File "/private/var/folders/24/8k48jl6d249_n_qfxwsl6xvm0000gn/T/cibw-run-ddamarri/pp38-macosx_x86_64/build/venv/lib/pypy3.8/site-packages/delocate/cmd/delocate_wheel.py", line 110, in main
      copied = delocate_wheel(
    File "/private/var/folders/24/8k48jl6d249_n_qfxwsl6xvm0000gn/T/cibw-run-ddamarri/pp38-macosx_x86_64/build/venv/lib/pypy3.8/site-packages/delocate/delocating.py", line 1004, in delocate_wheel
      out_wheel_fixed = _check_and_update_wheel_name(
    File "/private/var/folders/24/8k48jl6d249_n_qfxwsl6xvm0000gn/T/cibw-run-ddamarri/pp38-macosx_x86_64/build/venv/lib/pypy3.8/site-packages/delocate/delocating.py", line 839, in _check_and_update_wheel_name
      raise DelocationError(
  delocate.libsana.DelocationError: Library dependencies do not satisfy target MacOS version 10.9:
  /private/var/folders/24/8k48jl6d249_n_qfxwsl6xvm0000gn/T/tmpahver5b_/wheel/tcod/.dylibs/SDL2 has a minimum target of 10.11

I was not expecting MACOSX_DEPLOYMENT_TARGET to behave this way since I'm unfamiliar with the variable. I had hoped that #198 would automatically handle this case by renaming the wheel, but I had to set MACOSX_DEPLOYMENT_TARGET=10.11 to resolve this.

To workaround this issue either set MACOSX_DEPLOYMENT_TARGET to a higher version or use the --require-target-macos-version <version> flag.

I could make a PR to fix this but I need to know how others feel about the MACOSX_DEPLOYMENT_TARGET variable. Maybe I only need to add instructions on how to configure the target version to this error message. Or maybe the envrioment variable should be one that's less likely to be set by upstream tools.

matthew-brett commented 3 months ago

Sorry for my lack of reflection - but is the question here about which workaround we should suggest to the user, of the two options a) setting e.g. export MACOSX_DEPLOYMENT_TARGET=10.11 or b) adding a delocate command line flag e.g. --require-target-macos-version 10.11?

HexDecimal commented 3 months ago

@matthew-brett My question is if both of those two workarounds should be added to the error message, or should Decloate ignore MACOSX_DEPLOYMENT_TARGET when it exists to automatically adjust the MacOS version unless --require-target-macos-version is explicitly given.

I'm leaning towards ignoring MACOSX_DEPLOYMENT_TARGET since the current behavior is breaking older scripts, but the workarounds are easy to add once they're explained. I don't know how much people value MACOSX_DEPLOYMENT_TARGET but it seemed important enough to the person who made the PR. Still, this current behavior defies what I asked for in the PR since MACOSX_DEPLOYMENT_TARGET is more pervasive than I initially thought. I did not want Delocate to fail with a version check unless --require-target-macos-version was being used.

So I need clarification on how important MACOSX_DEPLOYMENT_TARGET is for MacOS developers.

matthew-brett commented 3 months ago

I can't give any definitive opinion - but my guess is that MACOSX_DEPLOYMENT_TARGET is important. You probably saw, but it's very widely used on Github, and I bet it's mainly used in CI - and to specify the minimum macOS on which the binary should work. If we ignore it, then the user specifies MACOSX_DEPLOYMENT_TARGET=10.9, but in fact gets a binary that crashes on 10.9 - I bet that will often be a problem.

bigcat88 commented 3 months ago

So I need clarification on how important MACOSX_DEPLOYMENT_TARGET is for MacOS developers.

very very large amount of packages for macOS depends on it. Almost any package that includes C/C++ module is using it.

HexDecimal commented 3 months ago

I've decided to keep the current behavior and improve the error message.