-
-
Notifications
You must be signed in to change notification settings - Fork 33
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
Switch to rattler-build; disable mixed case also on linux #92
base: main
Are you sure you want to change the base?
Conversation
…nda-forge-pinning 2024.09.23.02.42.53
…onda-forge-pinning 2024.12.15.06.34.57
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 ( |
@h-vetinari should we enable the mixed-case option for Linux, too? |
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.
@h-vetinari should we enable the mixed-case option for Linux, too?
IMO yes, see also @isuruf's comment here. AFAIU we could make
ncurses-feedstock/recipe/build.sh
Lines 12 to 14 in 16f1845
if [[ $target_platform =~ osx-.* ]]; then | |
export cf_cv_mixedcase=no | |
fi |
unconditional
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.
Aside from a minor point about the run-export, this LGTM!
Given the discussion caused by me being too fast on this point with #74, perhaps @mbargull or @isuruf (or someone else from @conda-forge/ncurses) want to review as well? I mostly took over maintainership here because no-one responded in #87 😅
requirements: | ||
run: | ||
- python |
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.
This adds a dependency (even though it's at test-time), so let's not do this. Better option is to use test_downstreams
equivalent in rattler-build.
(Note that the non-rattler-build test was wrong as it was testing the ncurses functionality of the python that conda-build was running on.)
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.
Why is it better? From rattler-build perspective it doesn't really matter. In the test we're explicitly importing ncurses
and I am not sure if Python downstream tests would fail the same way.
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 any bootstrapping perspective including migrations, it is better to not have cycles.
In the test we're explicitly importing ncurses and I am not sure if Python downstream tests would fail the same way.
We should add those tests to python if they don't exist.
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.
Ping
Co-authored-by: h-vetinari <[email protected]>
Friendly one month ping. I'd like to see this go in as @wolfv has done some great work here and it'll unblock me on a few feedstocks e.g. conda-forge/marvin-feedstock#2 (comment) It's got the thumbs up from @h-vetinari. @isuruf do you have issues of this going in? Is there anything outstanding from your review above? |
If you want the disable mixed case on Linux, I think that should be a new PR instead of having two things done in the same PR. Why is switching to rattler-build blocking? |
See #97 |
I mean the mixed case as that's likely what I was seeing in. Not the rattler-build per say. I have to admit I didn't know what ncurses was until I came across it in conda-forge/jack-feedstock#22 (comment) |
No description provided.