-
-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
take c_stdlib_version into account for MACOSX_DEPLOYMENT_TARGET / DEFAULT_LINUX_VERSION #26012
Conversation
Hi! This is the friendly automated conda-forge-linting service. I was trying to look for recipes to lint for you, but couldn't find any. |
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 ( Here's what I've got... For recipes/semsimian:
|
config = load(text, Loader=BaseLoader) | ||
|
||
if 'MACOSX_DEPLOYMENT_TARGET' in config: | ||
for version in config['MACOSX_DEPLOYMENT_TARGET']: | ||
version = tuple([int(x) for x in version.split('.')]) | ||
deployment_version = max(deployment_version, version) | ||
if 'c_stdlib_version' in config: | ||
for version in config['c_stdlib_version']: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this work if the values depend on the platform using selectors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question; I had assumed it would work like for MACOSX_DEPLOYMENT_TARGET
, which can already be split between x64 and arm64, but I guess we never build arm64 in staged-recipes, so that case is not relevant.
In practice, it'll work because 10.x > 2.y even if we get some glibc versions mixed in, but I admit that that's not very nice to rely on. And I guess it would break if there's only a linux c_stdlib_version
...
Not sure how much logic I should expend here w.r.t. filtering selectors - sounds like some simple regexes would be enough; WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I implemented something that handles most common selectors, except ones containing or
(like # [linux or win]
), but those really shouldn't occur for c_stdlib_version
/ MACOSX_DEPLOYMENT_TARGET
/ MACOSX_SDK_VERSION
, as that'd be completely wrong for at least one of the platforms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my POV this works now; there's 3 specs of c_stdlib_version
in CBC, but during the build we only take the ones for the current platform into account.
linux
+ echo 'Building all recipes'
Building all recipes
Found c_stdlib_version for linux: version=(2, 17)
Overriding DEFAULT_LINUX_VERSION to be cos7
osx
+ echo 'Building all recipes'
Building all recipes
Found c_stdlib_version for osx: version=(10, 12)
Overriding MACOSX_DEPLOYMENT_TARGET to be 10.12
I was also figuring out how to do the same for linux; I can correctly populate Another issue is that we're still on |
All the |
@beckermr, any comments here? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll miss edge cases but so be it.
Hi! This is the friendly automated conda-forge-linting service. I was trying to look for recipes to lint for you, but couldn't find any. |
Thanks! I'm putting this in despite the linter complaining there's no recipe. The actual recipe I used for testing (which hard-requires macOS 10.12) will go through #25929. |
For reference, this was the CI run before rebasing out the recipe. |
No description provided.