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

ENH OSX sysroot/sdk package #23777

Merged
merged 61 commits into from
Sep 2, 2023
Merged

ENH OSX sysroot/sdk package #23777

merged 61 commits into from
Sep 2, 2023

Conversation

beckermr
Copy link
Member

@beckermr beckermr commented Aug 23, 2023

Checklist

  • Title of this PR is meaningful: e.g. "Adding my_nifty_package", not "updated meta.yaml".
  • License file is packaged (see here for an example).
  • Source is from official source.
  • Package does not vendor other packages. (If a package uses the source of another package, they should be separate packages or the licenses of all packages need to be packaged).
  • If static libraries are linked in, the license of the static library is packaged.
  • Package does not ship static libraries. If static libraries are needed, follow CFEP-18.
  • Build number is 0.
  • A tarball (url) rather than a repo (e.g. git_url) is used in your recipe (see here for more details).
  • GitHub users listed in the maintainer section have posted a comment confirming they are willing to be listed there.
  • When in trouble, please check our knowledge base documentation before pinging a team.

TODO

  • swap role of CONDA_BUILD_SYSROOT and SDKROOT
  • only set conda-build items if in conda-build
  • move items from clang compiler activation to here
  • add sdk version 0 which uses the SDK on the system

@conda-forge-webservices
Copy link

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

I wanted to let you know that I linted all conda-recipes in your PR (recipes/osx-sysroot) and found some lint.

Here's what I've got...

For recipes/osx-sysroot:

  • The home item is expected in the about section.
  • The recipe must have some tests.

@beckermr beckermr changed the title WIP something like this WIP OSX sysroot/sdk package Aug 23, 2023
@beckermr
Copy link
Member Author

here is a start @conda-forge/core

there is lots more to do here I think

it may make sense to have this package set the deployment target env instead of it appearing elsewhere in the CBC.

@conda-forge-webservices
Copy link

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

I wanted to let you know that I linted all conda-recipes in your PR (recipes/osx-sysroot) and found some lint.

Here's what I've got...

For recipes/osx-sysroot:

  • The top level meta key tests is unexpected
  • The recipe must have some tests.

@conda-forge-webservices
Copy link

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

I wanted to let you know that I linted all conda-recipes in your PR (recipes/osx-sysroot) and found some lint.

Here's what I've got...

For recipes/osx-sysroot:

  • The top level meta keys are in an unexpected order. Expecting ['package', 'build', 'test', 'outputs', 'about', 'extra'].

@conda-forge-webservices
Copy link

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 (recipes/osx-sysroot) and found it was in an excellent condition.

recipes/osx-sysroot/meta.yaml Outdated Show resolved Hide resolved
recipes/osx-sysroot/meta.yaml Outdated Show resolved Hide resolved
recipes/osx-sysroot/conda_build_config.yaml Show resolved Hide resolved
recipes/osx-sysroot/meta.yaml Outdated Show resolved Hide resolved
recipes/osx-sysroot/conda_build_config.yaml Outdated Show resolved Hide resolved
Copy link
Member

@jakirkham jakirkham left a comment

Choose a reason for hiding this comment

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

Thanks Matt! 🙏

Added a couple suggestions. Though maybe these change if we go the zip_keys route (they may also need some tweaking anyway)

recipes/osx-sysroot/conda_build_config.yaml Outdated Show resolved Hide resolved
recipes/osx-sysroot/meta.yaml Outdated Show resolved Hide resolved
@conda-forge-webservices
Copy link

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

I wanted to let you know that I linted all conda-recipes in your PR (recipes/osx-sysroot) and found some lint.

Here's what I've got...

For recipes/osx-sysroot:

  • The recipe must have some tests.
  • The recipe must have a build/number section.
  • The outputs section contained an unexpected subsection name. tests is not a valid subsection name.

@conda-forge-webservices
Copy link

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

I wanted to let you know that I linted all conda-recipes in your PR (recipes/osx-sysroot) and found some lint.

Here's what I've got...

For recipes/osx-sysroot:

  • The recipe must have a build/number section.

@conda-forge-webservices
Copy link

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 (recipes/osx-sysroot) and found it was in an excellent condition.

@jakirkham
Copy link
Member

Thanks Matt! 🙏

This is coming along nicely 🙂

One other question to consider, do we want this to just be a metapackage or do we want it to contain things (like TAPI files or similar)?

IOW if a user tried to build a package using this recipe locally, what behavior should they expect?

@beckermr
Copy link
Member Author

Yep @jakirkham. This is my thought exactly. :) I think this package should handle all of the SDK stuff we do in conda-build-ci-setup right now, TAPI, etc. Having it in build should behave just like the linux one.

@beckermr
Copy link
Member Author

We'll want special hooks for cache downloads in ci across envs.

@jakirkham
Copy link
Member

We should double check which things we can package both from a legal and technical standpoint. Recall issues on both fronts with the SDKs for example. Though TAPI might be ok (still should check though)

Some past discussion in PR: #5382

@jakirkham
Copy link
Member

Should add am ok punting on that discussion for now. Just thinking about the questions users might ask when this package shows up and how we might respond to them. Similarly what we might like to add here in the future

@beckermr
Copy link
Member Author

Oh I don't intend to package the sdk. Just put the scripts that download it in this package. It will be downloaded by the user when they run the job.

@beckermr
Copy link
Member Author

OK let's see how this version works. Might fail on ci because CI uses 10.9 for SDK.

Copy link
Member

@h-vetinari h-vetinari left a comment

Choose a reason for hiding this comment

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

Cool, I think this looks much more manageable. 👍

recipes/osx-sysroot/activate-mdt.sh Outdated Show resolved Hide resolved
recipes/osx-sysroot/activate-mdt.sh Outdated Show resolved Hide resolved
recipes/osx-sysroot/meta.yaml Show resolved Hide resolved
@beckermr
Copy link
Member Author

Ok @h-vetinari this is ready for another look. I tested locally and squashed some dumb bugs. Works in zsh now too.

@h-vetinari
Copy link
Member

Ok @h-vetinari this is ready for another look.

IMO this looks very nice, but I think some other (core) feedback would still be good, and especially if existing criticism can now be considered addressed and fixed.

@beckermr
Copy link
Member Author

@conda-forge/core @isuruf any comments?

@beckermr
Copy link
Member Author

Any other comments here @conda-forge/core?

@jakirkham
Copy link
Member

Could you please share what the current intended usage would be in an example?

@beckermr
Copy link
Member Author

beckermr commented Aug 28, 2023

Sure!

This package goes into build like this:

requirements:
  build:
    # expands to sysroot_{{ target_platform}} on linux
    # expands to macosx_deployement_target_{{ target_platform}} on osx
    - {{ stdlib('c') }}  
    - {{ compiler('c') }}

Our default pinnings will have

c_stdlib:
  - sysroot                   # [linux]
  - macosx_deployment_target  # [osx and x86_64]
  - macosx_deployment_target  # [osx and arm64]
c_stdlib_version:
  - 2.17                      # [linux]
  - 10.13                     # [osx and x86_64]
  - 11.0                      # [osx and arm64]

The stdlib jinja2 function doesn't yet exist in conda-build.

@beckermr
Copy link
Member Author

Any other comments here @conda-forge/core?

Copy link
Member

@h-vetinari h-vetinari left a comment

Choose a reason for hiding this comment

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

LGTM with or without the warning for mismatched SDK / deployment target.

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

Successfully merging this pull request may close these issues.

6 participants