Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BUG: MACOSX_SDK_VERSION not participating in merge logic #1927

Closed
Tracked by #2102
h-vetinari opened this issue May 12, 2024 · 7 comments · Fixed by #1928
Closed
Tracked by #2102

BUG: MACOSX_SDK_VERSION not participating in merge logic #1927

h-vetinari opened this issue May 12, 2024 · 7 comments · Fixed by #1928

Comments

@h-vetinari
Copy link
Member

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
Copy link
Member Author

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

@beckermr
Copy link
Member

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

@h-vetinari
Copy link
Member Author

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
Copy link
Member

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

@h-vetinari
Copy link
Member Author

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
Copy link
Member

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

@h-vetinari
Copy link
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants