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

support stdlib() jinja function #4996

Closed
wants to merge 4 commits into from
Closed

support stdlib() jinja function #4996

wants to merge 4 commits into from

Conversation

isuruf
Copy link
Contributor

@isuruf isuruf commented Sep 6, 2023

Description

Checklist - did you ...

  • Add a file to the news directory (using the template) for the next release's release notes?
  • Add / update necessary tests?
  • Add / update outdated documentation?

Fixes #4981

cc @h-vetinari, @chenghlee, @mbargull, @beckermr

@conda-bot conda-bot added the cla-signed [bot] added once the contributor has signed the CLA label Sep 6, 2023
Copy link
Contributor

@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.

Thanks a lot for the PR!

I don't know the conda-build code base, but left some comments for things that stood out to me.

conda_build/jinja_context.py Outdated Show resolved Hide resolved
Comment on lines +515 to +516
# fall back to native if language-key is not explicitly set in variant
package_prefix = config.variant.get(language_key, package_prefix)
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if the fallback makes sense for stdlib? If I read the code correctly, the fallback key we'd be looking for would just be c?

IOW, if I'm not overlooking something here, I think we could move move the if "compiler" branch in here rather than setting a dummy package_prefix for stdlib.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it would be c_linux-64. We can also use {{ compiler('clang') }} which expands to clang_linux-64 with this fallback. Don't see a reason we shouldn't allow it for stdlib.

conda_build/jinja_context.py Outdated Show resolved Hide resolved
@isuruf
Copy link
Contributor Author

isuruf commented Sep 10, 2023

@h-vetinari, can you help with writing a test and fixing the existing tests?

@h-vetinari
Copy link
Contributor

Do you want me to raise a PR against your branch or can you make me collaborator on your fork?

@isuruf
Copy link
Contributor Author

isuruf commented Sep 10, 2023

Can you create a new PR in this repo?

@h-vetinari h-vetinari mentioned this pull request Sep 10, 2023
3 tasks
@h-vetinari
Copy link
Contributor

Done: #4999

@jaimergp
Copy link
Contributor

jaimergp commented Nov 7, 2023

Superseded by #4999

@jaimergp jaimergp closed this Nov 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed [bot] added once the contributor has signed the CLA
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

New jinja-function for handling sysroot
4 participants