Rename MACOS_DEPLOYMENT_TARGET back to MACOSX_DEPLOYMENT_TARGET #309

stefanv · 2023-02-15T00:35:03Z

This is the environment variable name used by Apple (see compat(5)). It is also supported by cibuildwheel, CMAKE, etc.

It was renamed in 9e84bd9

stefanv · 2023-02-15T00:36:02Z

There is one other similar occurrence, which is MACOS_BUILD_TARGET, which occurs only in tests/test_tags.py.

henryiii · 2023-02-15T04:28:00Z

Ideally, it would be nice to get this into a patch release, since it is a pretty bad regression that breaks the ability to make wheels for older macOS versions (unless you know to set MACOS_BUILD_TARGET as well as MACOSX_BUILD_TARGET :) ).

dnicolodi · 2023-02-15T10:45:59Z

Ups, this typo is mine. I remember cross checking the variable name with the one used by other tools and convincing myself that all was good. I also tested this and it seemed to work fine, for example here https://github.com/dnicolodi/python-siphash24/actions/runs/3553666516/jobs/5969311668 cibuildwheel does not complain. Anyhow, the fix is obviously correct.

The MACOS_BUILD_TARGET thing in the tests is broken: the test does not test what it looks like it tests (setting this environment variable has no effect on the computed wheel tags but the test is broken). It is probably a left over from when the wheel tags where computed differently that I forgot to clean up. I'm having a look at what the story is there.

dnicolodi · 2023-02-15T13:27:17Z

Nope, I added the broken MACOS_BUILD_TARGET test and I've looked everywhere, but it really seem to be something I made up. I think the test can be simply removed. Does anyone know how I could have come up with this environment variable name?

dnicolodi · 2023-02-15T13:28:21Z

@stefanv I don't find anything about environment variables in the comapt man page on my macOS system with Xcode 14.2. Where did you find it documented?

The X got lost in the big refactoring in mesonbuild#202.

The test tried to test the effect of setting the MACOS_BUILD_TARGET environment variable on the produced wheel tags, however there is bug for which the emitted tags are always compared to the tag for the current platform. The environment variable has no effect on the wheel tags. Remove the bogus test.

stefanv · 2023-02-15T17:47:03Z

@stefanv I don't find anything about environment variables in the comapt man page on my macOS system with Xcode 14.2. Where did you find it documented?

Thanks for reviewing, @dnicolodi!

Here's the relevant manpage (I don't own a mac, so can't say exactly where this was captured):

https://www.manpagez.com/man/5/compat/

Czaki · 2023-02-15T17:57:27Z

The MACOS_BUILD_TARGET thing in the tests is broken

It was broken, because meson, in contrast to the wheel, does not check the platform tag in binary files, but only read values from the environment variable.

jarrodmillman · 2023-02-15T18:36:14Z

Ideally, it would be nice to get this into a patch release, since it is a pretty bad regression that breaks the ability to make wheels for older macOS versions (unless you know to set MACOS_BUILD_TARGET as well as MACOSX_BUILD_TARGET :) ).

@henryiii @FFY00 A 0.13.0rc1 release would be great, since we are already using 0.13.0rc0 for scikit-image.

dnicolodi · 2023-02-15T19:10:38Z

The MACOS_BUILD_TARGET thing in the tests is broken

It was broken, because meson, in contrast to the wheel, does not check the platform tag in binary files, but only read values from the environment variable.

Would you like to elaborate? Meson is not responsible for generating Python wheels. Also, which piece of software react to the MACOS_BUILD_TARGET environment variable? I haven't found any. meson-python only checks the MACOSX_DEPLOYMENT_TARGET to produce the wheel tags as it trusts the fact that if the environment variable is set, the toolchain has done the right thing and build a binary compatible with the specified macOS version. I don't see where MACOS_BUILD_TARGET enters in this process.

dnicolodi · 2023-02-15T19:19:49Z

Here's the relevant manpage (I don't own a mac, so can't say exactly where this was captured):

https://www.manpagez.com/man/5/compat/

Thanks. The linked man page is the same as the one on my system. I just overlooked the mention of the MACOSX_DEPLOYMENT_TARGET environment variable in it.

Czaki · 2023-02-15T19:41:22Z

The test that was removed in this PR and does not fails even if a typo in the variable because there is no validation if files are created correctly.

I do not play with meson, but when using another build system, it happens to have build polluted by the previous run (ex. make, cmake will not rebuild all files because the env variable is changed, only this that sources files were changed) or part of build process may be the usage of binary blob give by a vendor (like in this package https://github.com/tlambert03/nd2).

Both scenarios will lead to an incorrectly tagged wheel file. First may be minimalized by using CI, but still user could enable cache operations for speedup build.

dnicolodi · 2023-02-15T19:47:43Z

The test that was removed in this PR and does not fails even if a typo in the variable because there is no validation if files are created correctly.

This is not what was wrong with the test. The typo was in an environment variable name not touched by the removed test.

Czaki · 2023-02-15T19:48:42Z

tests/test_tags.py

-
-@pytest.mark.skipif(platform.system() != 'Darwin', reason='macOS specific test')
-def test_tag_macos_build_target(monkeypatch):
-    monkeypatch.setenv('MACOS_BUILD_TARGET', '12.0')


I see here that is touched

Sure, but who reads it? Which piece of software reads this environment variable? The environment variable expected by the toolchain is MACOSX_DEPLOYMENT_TARGET. I suspect that MACOS_BUILD_TARGET is something I made up.

Yes. but when this test will set variable to 10.9 and the tag calculation will check files then this test will fail, as the test will in both runs return 12.0 (as the wrong variable was set).

@Czaki you don't make any sense. I'm giving up.

Your assumption that after correctly setting the environment variable, the toolchain will produce the correct files is wrong. It will often be true, but not always.

Nad I only point the test that, if checking the platform tag will examine files, the test will find a bug in the variable name.

Except that the test is not compiling any code an there is no file to examine.

After reading the code I was pretty sure that it compile. I'm sorry for bothering you.

Change CI MACOSX_DEPLOYMENT_TARGET from 10.13 to 10.9 to allow for continued compatibility with earlier OS versions given that associated wheels are available which meet the dependency version requirements. The correct fix for building MacOSX wheels is here: mesonbuild/meson-python#309 We are using this temporary workaround: scikit-image#6757 SciPy 1.10.1 will also suport 10.9: scikit-image#6750

Change CI MACOSX_DEPLOYMENT_TARGET from 10.13 to 10.9 to allow for continued compatibility with earlier OS versions given that associated wheels are available which meet the dependency version requirements. The correct fix for building MacOSX wheels is here: mesonbuild/meson-python#309 We are using this temporary workaround: #6757 SciPy 1.10.1 will also suport 10.9: #6750

eli-schwartz approved these changes Feb 15, 2023

View reviewed changes

henryiii approved these changes Feb 15, 2023

View reviewed changes

BUG: fix typo in MACOSX_DEPLOYMENT_TARGET environment variable name

e0c3d39

The X got lost in the big refactoring in mesonbuild#202.

dnicolodi force-pushed the MACOSX_DEPLOYMENT_TARGET-restore branch from 7fc234c to f4cdce7 Compare February 15, 2023 16:32

dnicolodi force-pushed the MACOSX_DEPLOYMENT_TARGET-restore branch from f4cdce7 to af2432d Compare February 15, 2023 16:33

dnicolodi merged commit 657747a into mesonbuild:main Feb 15, 2023

Czaki reviewed Feb 15, 2023

View reviewed changes

andyfaff mentioned this pull request Feb 16, 2023

MAINT: bump OpenBLAS version, bump macOS image used in GHA scipy/scipy#17950

Merged

stefanv mentioned this pull request Feb 16, 2023

macosx x86_64 build does not respect MACOSX_DEPLOYMENT_TARGET pypa/cibuildwheel#1419

Closed

tylerjereddy mentioned this pull request Feb 16, 2023

BLD: make sure macosx_x86_64 10.9 tags are being made on maintenance/1.10.x scipy/scipy#17981

Merged

dnicolodi mentioned this pull request Feb 17, 2023

CI: add job to build wheels and upload them when a release is tagged #312

Closed

jarrodmillman mentioned this pull request Feb 17, 2023

CI Set MACOSX_DEPLOYMENT_TARGET=10.9 for Wheels scikit-image/scikit-image#6750

Merged

stefanv mentioned this pull request Mar 29, 2023

update scikit to 0.20.0 conda-forge/scikit-image-feedstock#100

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename MACOS_DEPLOYMENT_TARGET back to MACOSX_DEPLOYMENT_TARGET #309

Rename MACOS_DEPLOYMENT_TARGET back to MACOSX_DEPLOYMENT_TARGET #309

stefanv commented Feb 15, 2023

stefanv commented Feb 15, 2023

henryiii commented Feb 15, 2023

dnicolodi commented Feb 15, 2023

dnicolodi commented Feb 15, 2023

dnicolodi commented Feb 15, 2023

stefanv commented Feb 15, 2023

Czaki commented Feb 15, 2023

jarrodmillman commented Feb 15, 2023 •

edited

Loading

dnicolodi commented Feb 15, 2023

dnicolodi commented Feb 15, 2023

Czaki commented Feb 15, 2023

dnicolodi commented Feb 15, 2023

Czaki Feb 15, 2023

dnicolodi Feb 15, 2023

Czaki Feb 15, 2023

dnicolodi Feb 15, 2023

Czaki Feb 15, 2023

dnicolodi Feb 15, 2023

Czaki Feb 15, 2023

Rename MACOS_DEPLOYMENT_TARGET back to MACOSX_DEPLOYMENT_TARGET #309

Rename MACOS_DEPLOYMENT_TARGET back to MACOSX_DEPLOYMENT_TARGET #309

Conversation

stefanv commented Feb 15, 2023

stefanv commented Feb 15, 2023

henryiii commented Feb 15, 2023

dnicolodi commented Feb 15, 2023

dnicolodi commented Feb 15, 2023

dnicolodi commented Feb 15, 2023

stefanv commented Feb 15, 2023

Czaki commented Feb 15, 2023

jarrodmillman commented Feb 15, 2023 • edited Loading

dnicolodi commented Feb 15, 2023

dnicolodi commented Feb 15, 2023

Czaki commented Feb 15, 2023

dnicolodi commented Feb 15, 2023

Czaki Feb 15, 2023

Choose a reason for hiding this comment

dnicolodi Feb 15, 2023

Choose a reason for hiding this comment

Czaki Feb 15, 2023

Choose a reason for hiding this comment

dnicolodi Feb 15, 2023

Choose a reason for hiding this comment

Czaki Feb 15, 2023

Choose a reason for hiding this comment

dnicolodi Feb 15, 2023

Choose a reason for hiding this comment

Czaki Feb 15, 2023

Choose a reason for hiding this comment

jarrodmillman commented Feb 15, 2023 •

edited

Loading