add MACOSX_DEPLOYMENT_TARGET to same zip as c_stdlib_version #5669

h-vetinari · 2024-03-23T23:14:42Z

As discussed in conda-forge/conda-smithy#1889

conda-forge-webservices · 2024-03-23T23:14:47Z

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.

mbargull

We should simplify this and not impose CUDA-related restrictions to non-CUDA configs.

beckermr · 2024-03-24T19:27:19Z

I don't follow your request @mbargull. Can you be a bit more explicit about what you mean?

FWIW, I think the cuda restrictions on the compilers that were here (and might still be) should go away once we move the min cuda version high enough. I forget the details. Others probably recall them.

beckermr · 2024-03-24T19:28:15Z

Also I suspect the issue you are objecting too should be in another PR, not this one. We're just trying to get stdlib running and don't want to do a bunch of other stuff at the same time.

mbargull · 2024-03-24T19:35:27Z

I don't follow your request @mbargull. Can you be a bit more explicit about what you mean?

I'm confused; GitHub swallowed my code comment/suggestion... let me retype this. Then things should be clearer.

mbargull · 2024-03-24T19:36:43Z

recipe/conda_build_config.yaml

@@ -165,6 +165,7 @@ zip_keys:
    - cxx_compiler_version      # [unix]
    - fortran_compiler_version  # [unix]
    - c_stdlib_version          # [unix]
+    - MACOSX_DEPLOYMENT_TARGET  # [osx]


There's no need to put the c_stdlib_version (and related keys) into this zip key.
This tight coupling is only needed for the CUDA stuff.
We should do something like

+ - # [osx] + - c_stdlib_version # [osx] + - MACOSX_DEPLOYMENT_TARGET # [osx] +# For CUDA, c_stdlib_version/cdt_name is zipped below with the compilers. + - # [linux and os.environ.get("CF_CUDA_ENABLED", "False") != "True"] + - c_stdlib_version # [linux and os.environ.get("CF_CUDA_ENABLED", "False") != "True"] + - cdt_name # [linux and os.environ.get("CF_CUDA_ENABLED", "False") != "True"] - # [unix] - c_compiler_version # [unix] - cxx_compiler_version # [unix] - fortran_compiler_version # [unix] - - c_stdlib_version # [unix] - - MACOSX_DEPLOYMENT_TARGET # [osx] + - c_stdlib_version # [linux and os.environ.get("CF_CUDA_ENABLED", "False") == "True"] - cdt_name # [linux and os.environ.get("CF_CUDA_ENABLED", "False") == "True"] - cuda_compiler # [linux and os.environ.get("CF_CUDA_ENABLED", "False") == "True"] - cuda_compiler_version # [linux and os.environ.get("CF_CUDA_ENABLED", "False") == "True"] - docker_image # [linux and os.environ.get("CF_CUDA_ENABLED", "False") == "True" and os.environ.get("BUILD_PLATFORM", "").startswith("linux-")]

or

+# For CUDA, c_stdlib_version/cdt_name is zipped below with the compilers. + - # [osx or (linux and os.environ.get("CF_CUDA_ENABLED", "False") != "True")] + - c_stdlib_version # [osx or (linux and os.environ.get("CF_CUDA_ENABLED", "False") != "True")] + - MACOSX_DEPLOYMENT_TARGET # [osx] + - cdt_name # [linux and os.environ.get("CF_CUDA_ENABLED", "False") != "True"] - # [unix] - c_compiler_version # [unix] - cxx_compiler_version # [unix] - fortran_compiler_version # [unix] - - c_stdlib_version # [unix] - - MACOSX_DEPLOYMENT_TARGET # [osx] + - c_stdlib_version # [linux and os.environ.get("CF_CUDA_ENABLED", "False") == "True"] - cdt_name # [linux and os.environ.get("CF_CUDA_ENABLED", "False") == "True"] - cuda_compiler # [linux and os.environ.get("CF_CUDA_ENABLED", "False") == "True"] - cuda_compiler_version # [linux and os.environ.get("CF_CUDA_ENABLED", "False") == "True"] - docker_image # [linux and os.environ.get("CF_CUDA_ENABLED", "False") == "True" and os.environ.get("BUILD_PLATFORM", "").startswith("linux-")]

(Sorry, GitHub UI didn't allow me to put this into a proper suggestion since it touches things outside of this "hunk".)

I see. Let's make a new PR for that. We've had trouble with these zips and proper rendering. It'll be easier to test as two steps.

If you want.. That PR would then be a requisite for this PR then; i.e., we should not zip MACOSX_DEPLOYMENT_TARGET with the compiler versions in any case.

The macosx deployment target being set explicitly will be deprecated behavior after stdlib is done. The zipping here is to allow for a transition period as far as I understand it.

beckermr · 2024-03-24T19:37:26Z

Ah good but please move this to another pr. We are doing two unrelated things here. I'm happy to help with the cuda stuff elsewhere. It will be easier to debug issues if we work on one thing at a time.

beckermr · 2024-03-24T19:46:13Z

I don't follow why precisely the extra changes are needed but I'm happy to defer to @h-vetinari and @mbargull to make the call.

mbargull · 2024-03-24T19:47:51Z

See gh-5672.
After merging that PR, the change for this PR would be just to add

  -                             # [osx]
    - c_stdlib_version          # [osx]
    - MACOSX_DEPLOYMENT_TARGET  # [osx]

for macOS.

mbargull · 2024-03-24T19:54:58Z

I don't follow why precisely the extra changes are needed

We impose artificial restrictions if we zip sysroot_linux*/MACOSX_DEPLOYMENT_TARGET with the compiler versions.
Meaning, previously, we'd only need such coupling for CUDA-related parts -- and that need doesn't change whether we use explicit c_stdlib packages or not.
^-- That is the conceptual rationale.
In addition, by avoiding this large zip key for cases for which it is not needed, we can also avoid issues/constraints we may run into due to too tight coupling.

h-vetinari · 2024-03-24T22:03:07Z

In #5592, I had said that I want to avoid putting c_stdlib_version into the compiler-zip, but AFAIU it turned out to be necessary because we needed to zip it with cdt_name for the CUDA world. This was further iterated in #5607. If we're able to peel the stdlib out of that zip again, all the better, but AFAIU we need to make sure that CUDA builds opt into glibc 2.17.

h-vetinari · 2024-03-25T02:41:05Z

Closing this as obsolete. Thanks for your help @mbargull! 🙏

conda-forge-webservices · 2024-03-25T03:19:16Z

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

I was trying to look for recipes to lint for you, but it appears we have a merge conflict.
Please try to merge or rebase with the base branch to resolve this conflict.

Please ping the 'conda-forge/core' team (using the @ notation in a comment) if you believe this is a bug.

h-vetinari · 2024-03-25T03:19:36Z

After merging that PR, the change for this PR would be just to add [...]

Ah, I had missed that bit, sorry. Coming right up.

Suggested-By: Marcel Bargull <marcel.bargull@udo.edu>

conda-forge-webservices · 2024-03-25T03:23:44Z

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.

h-vetinari · 2024-03-25T04:36:13Z

This PR now breaks the current state of conda-forge/conda-smithy#1889, for reasons I don't fully understand

  File "[...]\dev\conda-forge\conda-smithy\conda_smithy\configure_feedstock.py", line 615, in _collapse_subpackage_variants
    used_key_values = conda_build.variants.list_of_dicts_to_dict_of_lists(
                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "E:\miniforge\envs\smithy-dev\Lib\site-packages\conda_build\variants.py", line 634, in list_of_dicts_to_dict_of_lists
    values = list(zip(*set(zip(*(squished[key] for key in group)))))
                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "E:\miniforge\envs\smithy-dev\Lib\site-packages\conda_build\variants.py", line 634, in <genexpr>
    values = list(zip(*set(zip(*(squished[key] for key in group)))))
                                 ~~~~~~~~^^^^^
KeyError: 'c_stdlib_version'

mbargull · 2024-03-25T08:43:44Z

Querying this: https://github.com/search?q=org%3Aconda-forge%20path%3A%2F(%5E%7C%5C%2F)conda_build_config%5C.yaml%24%2F%20MACOSX_DEPLOYMENT_TARGET&type=code
we do have cases like https://github.com/conda-forge/cctools-and-ld64-feedstock/blob/522ce9e43aae12b7601e40034fc5d757cb21fe10/recipe/conda_build_config.yaml#L30
which

has multiple entries for MACOSX_DEPLOYMENT_TARGET,
has MACOSX_DEPLOYMENT_TARGET as part of another zip key.

Both of which will break once we put MACOSX_DEPLOYMENT_TARGET in a zip key here.
We should address this before merging this.

h-vetinari · 2024-03-25T09:08:50Z

This PR now breaks the current state of conda-forge/conda-smithy#1889, for reasons I don't fully understand

This is fixed now as of the last commit in that PR.

Both of which will break once we put MACOSX_DEPLOYMENT_TARGET in a zip key here.
We should address this before merging this.

Extending your query a bit, there are very few cases that have multiple values of MACOSX_DEPLOYMENT_TARGET defined, and all but cctools are split between x86_64 and arm64. I'll happily fix the cctools one, it's a feedstock I maintain anyway, so this shouldn't be a blocker here.

mbargull · 2024-03-25T09:14:54Z

Extending your query a bit, there are very few cases that have multiple values of MACOSX_DEPLOYMENT_TARGET defined, and all but cctools are split between x86_64 and arm64. I'll happily fix the cctools one, it's a feedstock I maintain anyway, so this shouldn't be a blocker here.

Sounds good. Since you are a maintainer there, then I consider this as being "addressed" since "at least one member of all affected parties has been notified" ;).

Is this good to merge then? (I think yes; feel free to merge if you agree.)

h-vetinari · 2024-03-26T02:48:46Z

I'll happily fix the cctools one, it's a feedstock I maintain anyway, so this shouldn't be a blocker here.

So I tried doing this in conda-forge/cctools-and-ld64-feedstock#68, but I'm beginning to wonder now if the zip here makes sense at all, in the sense that it only zips singleton values, but we cannot anymore do something like cctools was doing, because now that c_stdlib_version / MACOSX_DEPLOYMENT_TARGET are in a zip, they cannot be added to other zips anymore.

So I wonder what we gain from this PR after all? Yes, it allows us to be sure that c_stdlib_version / MACOSX_DEPLOYMENT_TARGET are in sync in terms of zip-length, but that isn't even a requirement anymore for conda-forge/conda-smithy#1889 after the processing was moved to the variants themselves.

Overall I think it does more harm than good, and we should revert it...

beckermr · 2024-03-26T02:57:32Z

Go for it!

h-vetinari requested a review from a team as a code owner March 23, 2024 23:14

h-vetinari mentioned this pull request Mar 23, 2024

merge c_stdlib_version & MACOSX_DEPLOYMENT_TARGET on osx conda-forge/conda-smithy#1889

Merged

mbargull requested changes Mar 24, 2024

View reviewed changes

mbargull reviewed Mar 24, 2024

View reviewed changes

h-vetinari closed this Mar 25, 2024

h-vetinari reopened this Mar 25, 2024

zip MACOSX_DEPLOYMENT_TARGET and c_stdlib_version on osx

2d2094f

Suggested-By: Marcel Bargull <marcel.bargull@udo.edu>

h-vetinari force-pushed the osx branch from e5a0b6c to 2d2094f Compare March 25, 2024 03:23

beckermr approved these changes Mar 25, 2024

View reviewed changes

mbargull approved these changes Mar 25, 2024

View reviewed changes

h-vetinari merged commit f974928 into conda-forge:main Mar 25, 2024
4 checks passed

h-vetinari deleted the osx branch March 25, 2024 09:19

h-vetinari mentioned this pull request Mar 25, 2024

META: {{ stdlib("c") }} migration conda-forge/conda-forge.github.io#2102

Open

57 tasks

h-vetinari mentioned this pull request Mar 26, 2024

Revert "zip MACOSX_DEPLOYMENT_TARGET and c_stdlib_version on osx" #5685

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add MACOSX_DEPLOYMENT_TARGET to same zip as c_stdlib_version #5669

add MACOSX_DEPLOYMENT_TARGET to same zip as c_stdlib_version #5669

h-vetinari commented Mar 23, 2024

conda-forge-webservices bot commented Mar 23, 2024

mbargull left a comment

beckermr commented Mar 24, 2024

beckermr commented Mar 24, 2024

mbargull commented Mar 24, 2024

mbargull Mar 24, 2024

beckermr Mar 24, 2024

mbargull Mar 24, 2024

beckermr Mar 24, 2024 •

edited

Loading

beckermr commented Mar 24, 2024

beckermr commented Mar 24, 2024

mbargull commented Mar 24, 2024

mbargull commented Mar 24, 2024 •

edited

Loading

h-vetinari commented Mar 24, 2024

h-vetinari commented Mar 25, 2024

conda-forge-webservices bot commented Mar 25, 2024

h-vetinari commented Mar 25, 2024

conda-forge-webservices bot commented Mar 25, 2024

h-vetinari commented Mar 25, 2024

mbargull commented Mar 25, 2024

h-vetinari commented Mar 25, 2024

mbargull commented Mar 25, 2024

h-vetinari commented Mar 26, 2024

beckermr commented Mar 26, 2024

add MACOSX_DEPLOYMENT_TARGET to same zip as c_stdlib_version #5669

add MACOSX_DEPLOYMENT_TARGET to same zip as c_stdlib_version #5669

Conversation

h-vetinari commented Mar 23, 2024

conda-forge-webservices bot commented Mar 23, 2024

mbargull left a comment

Choose a reason for hiding this comment

beckermr commented Mar 24, 2024

beckermr commented Mar 24, 2024

mbargull commented Mar 24, 2024

mbargull Mar 24, 2024

Choose a reason for hiding this comment

beckermr Mar 24, 2024

Choose a reason for hiding this comment

mbargull Mar 24, 2024

Choose a reason for hiding this comment

beckermr Mar 24, 2024 • edited Loading

Choose a reason for hiding this comment

beckermr commented Mar 24, 2024

beckermr commented Mar 24, 2024

mbargull commented Mar 24, 2024

mbargull commented Mar 24, 2024 • edited Loading

h-vetinari commented Mar 24, 2024

h-vetinari commented Mar 25, 2024

conda-forge-webservices bot commented Mar 25, 2024

h-vetinari commented Mar 25, 2024

conda-forge-webservices bot commented Mar 25, 2024

h-vetinari commented Mar 25, 2024

mbargull commented Mar 25, 2024

h-vetinari commented Mar 25, 2024

mbargull commented Mar 25, 2024

h-vetinari commented Mar 26, 2024

beckermr commented Mar 26, 2024

beckermr Mar 24, 2024 •

edited

Loading

mbargull commented Mar 24, 2024 •

edited

Loading