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

increase default MACOSX SDK version to 10.13 #274

Closed
wants to merge 2 commits into from

Conversation

h-vetinari
Copy link
Member

I believe this is a next reasonable step for conda-forge/conda-forge.github.io#1844.

In particular, it would unblock various migrations for packages like abseil, where dependent feedstocks would fail already in the # include stage if the SDK is not up-to-date.

@h-vetinari h-vetinari requested a review from a team as a code owner September 4, 2023 05:07
@conda-forge-webservices
Copy link
Contributor

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

@isuruf
Copy link
Member

isuruf commented Sep 4, 2023

This change does not do anything. This changes MACOSX_DEPLOYMENT_TARGET's default when the feedstock does not have it. (for really old feedstocks that were not rerendered)

@h-vetinari
Copy link
Member Author

This change does not do anything.

In that case, could you help me make it do something, please?

AFAICT, in the absence of MACOSX_SDK_VERSION in the .ci_support/*.yaml files, this will now set the SDK version to 10.13 rather than 10.9; if that's not the case, I'm happy to fix it wherever necessary.

To be clear: I don't want to change the deployment target here, only ensure we're using the newer SDKs in CI, so that something like the abseil migration will not encounter wide-spread breakage.

@h-vetinari
Copy link
Member Author

This changes MACOSX_DEPLOYMENT_TARGET's default when the feedstock does not have it. (for really old feedstocks that were not rerendered)

Isn't that a rather theoretical concern? Feedstocks need to be rerendered regularly anyway, otherwise CI breaks (e.g. obsolete images not supported by github anymore, etc.)

@isuruf
Copy link
Member

isuruf commented Sep 4, 2023

Isn't that a rather theoretical concern?

No, what I'm saying is that this does not change anything other than old feedstocks that have not been rerendered.

AFAICT, in the absence of MACOSX_SDK_VERSION in the .ci_support/*.yaml files, this will now set the SDK version to 10.13 rather than 10.9; if that's not the case, I'm happy to fix it wherever necessary.

No, you are not. Please look at the code.

To be clear: I don't want to change the deployment target here, only ensure we're using the newer SDKs in CI, so that something like the abseil migration will not encounter wide-spread breakage.

Huh? You are not doing that here. You are changing MACOSX_DEPLOYMENT_TARGET's default value when it's not in ci_support files.

For that, you can change MACOSX_DEPLOYMENT_TARGET in the migration yaml for abseil.

@h-vetinari
Copy link
Member Author

AFAICT, in the absence of MACOSX_SDK_VERSION in the .ci_support/*.yaml files, this will now set the SDK version to 10.13 rather than 10.9; if that's not the case, I'm happy to fix it wherever necessary.

No, you are not. Please look at the code.

Well, in the absence of a MACOSX_SDK_VERSION key, it bases itself on the deployment target:

if [[ "${MACOSX_SDK_VERSION:-0}" == "0" ]]; then
export MACOSX_SDK_VERSION=$MACOSX_DEPLOYMENT_TARGET
fi

We could add some more branches of course, but it seems pretty straight forward to adapt it in only where the existing values are hard-coded. I tend to think the "old feedstocks" argument is not relevant, as unrerendered feedstocks would simply have broken CI due to conda-forge/conda-forge.github.io#1798, which got fixed by conda-forge/conda-smithy@13a4c6f (1 year ago). In contrast, MACOSX_DEPLOYMENT_TARGET has been part of the global pinning since the beginning (~6 years ago).

So AFAICT there's no way this would affect old feedstocks (either their CI on osx is broken, or they get rerendered to something functionally equivalent), aside from the fact that - based on the announcement - it would be OK for newly build packages to require >=10.13.

@isuruf
Copy link
Member

isuruf commented Sep 4, 2023

I tend to think the "old feedstocks" argument is not relevant,

I'm not saying changing this for old feedstocks is bad. All I'm saying is that this change will apply only to really really old feedstocks.

Can you explain how this would change the SDK and not the deployment target in the context of some feedstock say the numpy-feedstock?

@h-vetinari
Copy link
Member Author

I'm not saying changing this for old feedstocks is bad. All I'm saying is that this change will apply only to really really old feedstocks.

OK, I see now that the pinning-induced deployment target will always override what the code here is doing (and that I'm changing). I wanted to do minimal changes, that was my mistake. I guess we need to refactor this a bit. Thanks for the patience.

For that, you can change MACOSX_DEPLOYMENT_TARGET in the migration yaml for abseil.

I hadn't thought of including these values in the migrator, as it doesn't solve our metadata issues (e.g. produced artefacts don't get __osx>=10.13). Or am I overlooking something there as well (given that the idea of run-exports on the compilers did not receive much enthusiasm)?

@isuruf
Copy link
Member

isuruf commented Sep 4, 2023

I hadn't thought of including these values in the migrator, as it doesn't solve our metadata issues (e.g. produced artefacts don't get __osx>=10.13).

Since abseil has __osx>=10.13 it will get it transitively at least. It's not great, but something we could do to unblock abseil.

given that the idea of run-exports on the compilers did not receive much enthusiasm)?

Because compiler and libcxx are two differernt things as I explained in the core meeting.

@h-vetinari
Copy link
Member Author

Since abseil has __osx>=10.13 it will get it transitively at least. It's not great, but something we could do to unblock abseil.

If you prefer that to just bumping the SDK version, then that's fine by me.

Because compiler and libcxx are two differernt things as I explained in the core meeting.

I understand the difference, and it's not about libcxx for me (although that's coming down the pipeline). I wanted to use the compiler as a proxy for "compiled package", and ensure that the artefacts we produce from here on out have the right >=10.13 requirement universally (until the more elegant {{ stdlib("c") }} is in place and rolled out).

Copy link
Member

@isuruf isuruf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even though setting a higher SDK and lower deployment target should work in theory if the developer is careful, it's often the case not, so -100 on this. Examples are python 3.8 which would use SDK features and will ignore the deployment target. Python 3.8 is EOL, but that's the only example I have on top of my mind.

export MACOSX_SDK_VERSION=$MACOSX_DEPLOYMENT_TARGET
# ensure minimal SDK of 10.13, while treatement of
# deployment targets <10.13 is being figured out
export MACOSX_SDK_VERSION="10.13"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would break when MACOSX_DEPLOYMENT_TARGET>10.13

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That branch is only taken if the value hasn't been populated.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What value?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MACOSX_SDK_VERSION. But I think I understand what you mean now. It would break setting MACOSX_DEPLOYMENT_TARGET>10.13 without also setting MACOSX_SDK_VERSION>10.13.

If we want to support the use-case of specifying the target without the sdk, I could rewrite this. I wasn't aware this was even possible and considered it a non-issue.

@isuruf
Copy link
Member

isuruf commented Sep 4, 2023

If you prefer that to just bumping the SDK version, then that's fine by me.

Yes, please

@isuruf
Copy link
Member

isuruf commented Sep 4, 2023

I wanted to use the compiler as a proxy for "compiled package", and ensure that the artefacts we produce from here on out have the right >=10.13 requirement universally (until the more elegant {{ stdlib("c") }} is in place and rolled out).

Let's do that properly with stdlib('c'). Please send a conda-build PR and we can discuss that at the core meeting with anaconda folks.

@h-vetinari
Copy link
Member Author

Let's do that properly with stdlib('c')

I'm fine to do it properly, but we IMO need a remediation to unblock things like abseil before we can get this put into place (which involves conda-build, boa, rattler, etc., much less rolling this out to all feedstocks).

Even though setting a higher SDK and lower deployment target should work in theory if the developer is careful, it's often the case not, so -100 on this.

Fair enough in principle, but what precisely would be the issue if we produce binaries that require 10.13 features? The run-export I'm advocating just adds a blanket run-requirement, so any potentially wrong package metadata (due to diverging sdk & target) would not lead to breakage on old systems.

@isuruf
Copy link
Member

isuruf commented Sep 4, 2023

but we IMO need a remediation to unblock things like abseil before we can get this put into place

That's why I mentioned adding it to the migrator.

Fair enough in principle, but what precisely would be the issue if we produce binaries that require 10.13 features? The run-export I'm advocating just adds a blanket run-requirement, so any potentially wrong package metadata (due to diverging sdk & target) would not lead to breakage on old systems.

Then we might as well bump the MACOSX_DEPLOYMENT_TARGET. Then why bother with MACOSX_SDK_VERSION?

@h-vetinari
Copy link
Member Author

Then we might as well bump the MACOSX_DEPLOYMENT_TARGET. Then why bother with MACOSX_SDK_VERSION?

I'm happy to do that. My impression was just that a bumped MACOSX_DEPLOYMENT_TARGET does not yet lead to correct package metadata (i.e. that excludes it being installed on <10.13), hence the run-export.

Touching the SDK version was to unblock the situation while giving more time to figure out the deployment target side.

That's why I mentioned adding it to the migrator.

Happy to do that too.

@isuruf
Copy link
Member

isuruf commented Sep 4, 2023

You want to change MACOSX_SDK_VERSION while MACOSX_DEPLOYMENT_TARGET is being figured out and to have correct metadata. However as I mentioned, bumping MACOSX_SDK_VERSION might inadvertently bump MACOSX_DEPLOYMENT_TARGET in some packages, and you are okay with packages having wrong metadata because of MACOSX_SDK_VERSION bump. How does that make sense at all?

@h-vetinari
Copy link
Member Author

h-vetinari commented Sep 4, 2023

How does that make sense at all?

If we bump the SDK and add a __osx>=10.13 run-export, it would unblock things, while not creating broken artefacts (because any wrong metadata due to sdk/target mismatch below 10.13 would be guarded by the run-export).

Once we have {{ stdlib("c") }} in place, we could remove the run-export again and use the proper infrastructure for the deployment target.

@isuruf
Copy link
Member

isuruf commented Sep 4, 2023

If we bump the SDK and add a __osx>=10.13 run-export, it would unblock things, while not creating broken artefacts (because any wrong metadata due to sdk/target mismatch below 10.13 would be guarded by the run-export).

Can you explain how that is different from changing MACOSX_DEPLOYMENT_TARGET and adding a __osx>=10.13 run-export?

@h-vetinari
Copy link
Member Author

Can you explain how that is different from changing MACOSX_DEPLOYMENT_TARGET and adding a __osx>=10.13 run-export?

It's not (as I learned through this PR; I thought we'd always have to set the SDK). I was just answering your question how things fit together, but as I said I'm completely fine with bumping MACOSX_DEPLOYMENT_TARGET in the migrator (even more so if we can also add the run-export; I think it would be very beneficial for the solver to avoid that __osx>=10.13 requirement only comes in transitively).

@h-vetinari
Copy link
Member Author

Closing as obsolete.

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 this pull request may close these issues.

2 participants