conda-forge / conda-smithy

The tool for managing conda-forge feedstocks.
https://conda-forge.org/
BSD 3-Clause "New" or "Revised" License
146 stars 169 forks source link

BUG: MACOSX_SDK_VERSION not participating in merge logic #1927

Closed h-vetinari closed 1 month ago

h-vetinari commented 1 month ago

I just noticed that is a recipe uses

c_stdlib_version:          # [osx and x86_64]
  - "10.12"                # [osx and x86_64]
MACOSX_SDK_VERSION:        # [osx and x86_64]
  - "10.12"                # [osx and x86_64]

the merge logic of smithy will not be applied to MACOSX_SDK_VERSION, which will end up being incorrectly populated as 10.12 (while MACOSX_DEPLOYMENT_TARGET ends up being 10.13 after the merge with the global pinning).

I guess we should also add a linter warning for c_stdlib_version / MACOSX_SDK_VERSION <10.13?

CC @beckermr

h-vetinari commented 1 month ago

There's about ~80 feedstocks that set a too-low SDK currently.

beckermr commented 1 month ago

I don't follow if this is a longer thing versus something we should change in smithy itself to fix the issue.

h-vetinari commented 1 month ago

IMO we should warn on obsolete c_stdlib_version and either fix the merge logic for MACOSX_SDK_VERSION (so that 10.12 still ends up as 10.13 in the variant configs), and/or error on it in smithy. That way feedstocks can remove these things at their own pace without being broken.

beckermr commented 1 month ago

But shouldn't people still be able to build against an older sdk if they want to?

h-vetinari commented 1 month ago

But shouldn't people still be able to build against an older sdk if they want to?

Using a newer SDK doesn't break anything, only the deployment target is really relevant, so pulling in SDK 10.13 should be harmless (if we go with sdk=max(sdk,dep_target)). However, anything using C++ is going to be broken by libcxx 17 requiring 10.13 (and failing on stdlib-header inclusion on older SDKs).

If we want to let people opt into stuff <10.13, we would need to disable all merge logic between c_stdlib & MACOSX_DEPLOYMENT_TARGET, which I think is going to do more harm than good (by breaking recipes pointlessly). It's also not necessary from my POV to support this - we've dropped support for <10.13, so I don't see the problem if feedstocks don't get to build against older versions anymore.

beckermr commented 1 month ago

I still don't follow completely but am happy to review a pr if needed.

h-vetinari commented 1 month ago

But shouldn't people still be able to build against an older sdk if they want to?

So I just tested this with #1928, and people can still opt into older deployment targets if they override both c_stdlib_version and MACOSX_DEPLOYMENT_TARGET to 10.9 (for example). The SDK will be populated to use the same value.