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 minimum macOS deployment target to 10.13 #5829

Merged
merged 2 commits into from
May 6, 2024

Conversation

h-vetinari
Copy link
Member

@h-vetinari h-vetinari commented May 1, 2024

This tiny change has been a long time coming, for a taste of the journey check out:

Closes conda-forge/conda-forge.github.io#1844

We've already announced the move last August, but needed a whole bunch of new functionality to make things work.

I proposed in the core call just now to do this separately for macOS (as we've promised to move to glibc 2.17 on linux in June, and this will probably be an even bigger change), so that we get to test it in real life, and also since all the prerequisites are there now.

In particular, feedstocks that haven't yet been touched by the stdlib-piggyback migrator (which will keep running; it adds the required {{ stdlib("c") }} to the recipes whenever a migrator or version bump comes by) will now get a linter warning to add it (as of conda-smithy 3.35), so that a warning-free feedstock will continue to produce correct artefacts. This was what had been agreed in the last core call, and people were fine to bump this in the core call just now as well.

Still, CC @conda-forge/core if anyone has input. Otherwise I'll merge this in ~48h.

@h-vetinari h-vetinari requested a review from a team as a code owner May 1, 2024 18:29
@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 May 1, 2024

too?

@h-vetinari
Copy link
Member Author

True thanks, I had forgotten that we're still setting this in the global pinning. I guess this could be phased out eventually, but shouldn't hold us up here.

Copy link
Member

@beckermr beckermr left a comment

Choose a reason for hiding this comment

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

Do we need to change the macos_machine variable too? I honestly do not know what that is.

@h-vetinari
Copy link
Member Author

Do we need to change the macos_machine variable too? I honestly do not know what that is.

macos_machine is only really used in a handful of feedstocks, and responsible for setting the target triple. I'm not sure where the 13.4.0 / 20.0.0 numbers come from, but AFAICT this does not need to be changed. Please correct me @isuruf / @xhochy if I'm off here.

@xhochy
Copy link
Member

xhochy commented May 2, 2024

This is is the Darwin Kernel version. We should update it. According to https://en.wikipedia.org/wiki/MacOS_High_Sierra#Release_history we should set it to 17.0.0.

@beckermr
Copy link
Member

beckermr commented May 2, 2024

Is this value anywhere else in our compiled packages such that changing it would create issues?

To clarify, I mean in the already built compiler packages or even downstream packages built by the compilers. Clearly we need to change the feedstocks @h-vetinari found.

@h-vetinari
Copy link
Member Author

This is is the Darwin Kernel version. We should update it.

Thanks!

According to https://en.wikipedia.org/wiki/MacOS_High_Sierra#Release_history we should set it to 17.0.0.

Shouldn't we then set it to 17.7.0? For 10.9, 13.4 we also used the latest available minor version. Though OTOH, for osx-arm, we did take 20.0.0... I guess since this is mostly a measure of the C stdlib evolution, the minor version doesn't really matter anyway.

Is this value anywhere else in our compiled packages such that changing it would create issues?

I think this value is pretty arbitrary in the sense that it's only the name of our internal target triple, and updating the version only really serves the accuracy of our triples (to a relatively pedantic degree; our __osx metadata wouldn't let the packages be installed on older versions anyway) - not sure if the question of leaving out the darwin version is somehow heretical, but I do have to wonder... (of course, we also encode the glibc version/epoch in our linux targets, same argument could be made there vis-à-vis __glibc)

Nothing else but our compiler infrastructure depends on it AFAICT (though a small number of non-compiler feedstocks hard-code the full path to clang instead of using $CC - those would need to be fixed).

@beckermr
Copy link
Member

beckermr commented May 2, 2024

Ok. I guess we'd have to patch all of the compiler feedstocks so that packages with the new target triple don't interact with packages using the old target triple. That's a pain but possible.

@h-vetinari
Copy link
Member Author

I guess we'd have to patch all of the compiler feedstocks so that packages with the new target triple don't interact with packages using the old target triple.

I don't even think that's necessary? The packages remain compatible, in the sense that we're only moving to a newer C stdlib evolution state, but that wasn't a problem previously either (as evidenced by all the feedstocks bumping MACOSX_DEPLOYMENT_TARGET on x64 already, yet still "using" a darwin13.4.0 target triple).

That's why I'm tempted to remove the version - it serves no purpose (that I can see) now that we have __osx, it's incorrect if anyone does a feedstock-local override, and it's not actually an incompatible triple (in the usual sense) to move up the Darwin kernel version.

@beckermr
Copy link
Member

beckermr commented May 3, 2024

I suspect the triple might be hard coded in some of the paths.

@h-vetinari
Copy link
Member Author

I suspect the triple might be hard coded in some of the paths.

What kind of paths could that be? Shouldn't everything installed be in the PREFIX and undergo relocation by conda? Not saying this isn't possible, but I'm not sure what kind of scenario could cause that.

Would very much appreciate your input here @isuruf. 🙏

@beckermr
Copy link
Member

beckermr commented May 3, 2024

Oh sorry I mean in the names of compilers or other tools.

@h-vetinari
Copy link
Member Author

Oh sorry I mean in the names of compilers or other tools.

If you click the link under "hard-code" above, I did exactly that search in conda-forge, and there's not more than a handful (that aren't compiler-related).

@isuruf
Copy link
Member

isuruf commented May 3, 2024

I guess we'd have to patch all of the compiler feedstocks so that packages with the new target triple don't interact with packages using the old target triple.

None of the feedstocks that @h-vetinari linked uses the macos_machine value from the global pinning.

If you click the link under "hard-code" above, I did exactly that search in conda-forge, and there's not more than a handful (that aren't compiler-related).

Packages like mpich, perl harcode the compiler name (not the path) at build time. That's why we still have x86_64-conda-linux-gnu-cc -> x86_64-conda_cos6-linux-gnu-cc symlink in the gcc_impl* packages.

If there's a triplet that works without macos version, that'd be great, but autoconf has some checks for the darwin kernel for existence of dynamic_lookup option if I remember correctly.

@h-vetinari
Copy link
Member Author

None of the feedstocks that @h-vetinari linked uses the macos_machine value from the global pinning.

Note this was just a simple github search. Indeed it's mostly feedstocks that override this value themselves, but it's an illustration of where it's actually being used and not just part of the config.

Packages like mpich, perl harcode the compiler name (not the path) at build time. That's why we still have x86_64-conda-linux-gnu-cc -> x86_64-conda_cos6-linux-gnu-cc symlink in the gcc_impl* packages.

Ah OK, thanks.

If there's a triplet that works without macos version, that'd be great, but autoconf has some checks for the darwin kernel for existence of dynamic_lookup option if I remember correctly.

OK. Given all that, do you think we should:

  • keep 13.4.0 (least effort)
  • move to 17.0.0
  • move to 17.7.0
  • try removing darwin version and deal with autoconf issues separately

@isuruf
Copy link
Member

isuruf commented May 3, 2024

I would go with either 1 or 4.

@xhochy
Copy link
Member

xhochy commented May 3, 2024

Fine with both, but if we do 1, we should add a comment in the cbc.yaml that it is intentionally outdated.

@h-vetinari
Copy link
Member Author

h-vetinari commented May 3, 2024

Thanks for the inputs! I think we should do 1. now, and 4. potentially later. I'll add a comment.

@h-vetinari
Copy link
Member Author

Given the overall agreement here, I'm going to put this in. Thanks for the reviews/inputs!

@h-vetinari h-vetinari merged commit ef4ca4c into conda-forge:main May 6, 2024
4 checks passed
@h-vetinari h-vetinari deleted the 10.13 branch May 6, 2024 00:30
beenje added a commit to beenje/fisx-feedstock that referenced this pull request May 14, 2024
osx-64 build failed when merging into main because libcxx was updated from 16.x.x
to 17.x.x between the time of the MR and the merge.
libcxx 17.x.x requires MacOS >=10.13 (see conda-forge/libcxx-feedstock#131)

My understanding is that a re-render should fix it due to conda-forge/conda-forge-pinning-feedstock#5829
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.

DISCUSS: Raise MacOS minimum target from 10.9 to 10.13?
4 participants