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: correctly rerender CI configs with new stdlib jinja-function #1840

Closed
Tracked by #2102
h-vetinari opened this issue Feb 16, 2024 · 15 comments · Fixed by #1888
Closed
Tracked by #2102

ENH: correctly rerender CI configs with new stdlib jinja-function #1840

h-vetinari opened this issue Feb 16, 2024 · 15 comments · Fixed by #1888

Comments

@h-vetinari
Copy link
Member

h-vetinari commented Feb 16, 2024

This is in the context of regro/cf-scripts#2135 (see links therein).

I tried rendering a recipe with - {{ stdlib("c") }}, and unsurprisingly it failed. Adding the following patch

--- a/conda_smithy/utils.py
+++ b/conda_smithy/utils.py
@@ -93,6 +93,7 @@ def render_meta_yaml(text):
     env.globals.update(
         dict(
             compiler=lambda x: x + "_compiler_stub",
+            stdlib=lambda x: x + "_stdlib_stub",
             pin_subpackage=stub_subpackage_pin,
             pin_compatible=stub_compatible_pin,
             cdt=lambda *args, **kwargs: "cdt_stub",

eliminates the failure (edit: this turned into #1846), but while trying to write tests for this, I got stuck with understanding where in smithy the actual content of the CBC gets generated, and more importantly, where the values are taken from.

For example, I cannot find how gcc gets inserted as the c_compiler - it's neither specified in the code, nor injected by the fixture or the test, nor does it read the global pinning AFAICT.

I tried to follow what's happening in
https://github.com/conda-forge/conda-smithy/blob/main/conda_smithy/configure_feedstock.py and others, and have followed several (apparently) false leads already, so I thought I'd ask if someone can tell me where to look.

To be clear, what I want to ensure is that a line like - {{ stdlib("c") }} ends up inserting the correct c_stdlib{,_version}: ... into the generated ci-config files (pretty much exactly like we do for the compilers), c.f. also conda-forge/conda-forge-pinning-feedstock#5499.

CC @beckermr

@beckermr
Copy link
Member

I wonder if conda build has a default somewhere.

@h-vetinari
Copy link
Member Author

Sure, that's a probable answer where "gcc" comes from. But luckily we shouldn't need conda-build (or changes to it) here - we only need to pick up settings for c_stdlib{,_version} from a local (or global) conda_build_config.yaml, and populate the CI configs correctly.

So far, even if I extend the config_yaml fixture to contain the information from conda-forge/conda-forge-pinning-feedstock#5499, it doesn't get inserted into the CI yamls, and I don't know where I need to look to make that happen...

@beckermr
Copy link
Member

I'd check the configure feedstock code bits. There are explicit lists of variables there. I am betting this needs to be added to one of those lists.

@beckermr
Copy link
Member

Try line 359 in configure_feedstock.py

@h-vetinari
Copy link
Member Author

Try line 359 in configure_feedstock.py

Thanks for the hint! I had remembered something about always_keep_keys, but I didn't touch it because it didn't contain anything about the compilers, which was generally a good thing to look for, as compiler & stdlib work very similarly.

I tried adding c_stdlib{,_version} to always_keep_keys, and indeed they then get included into the config files, but their values are empty. I think the issue is that smithy(?) does not yet know that the presence of - {{ stdlib("c") }} means that it has to look for c_stdlib{,_version} keys. I'm not sure how that's done on the compiler side either, can't find relevant code for "compiler"/"stub"/etc.

@beckermr
Copy link
Member

That seems like a conda build issue, not a smithy one. Smithy uses conda build to do basically all of the handling of these keys.

@beckermr
Copy link
Member

@beckermr
Copy link
Member

@beckermr
Copy link
Member

We may also need to add stdlib to the base default variant: https://github.com/conda/conda-build/blob/f60b5fc40f502398530e4c143118e7fbaaaa5bef/conda_build/variants.py#L119

@beckermr
Copy link
Member

cc @mbargull for comments since he knows conda-build very well.

@h-vetinari
Copy link
Member Author

Thanks a lot for the pointers. I was hoping we could avoid touching conda-build again, but that was hopelessly optimistic it seems. I'll try to look at things more closely and come up with a PR.

@beckermr
Copy link
Member

It may be possible to adjust / wrap the conda-build internals with functions in smithy, but yeah, we may need to go back to conda-builds.

@mbargull
Copy link
Member

The compiler(<lang>) -> <lang>_compiler -> <package>_<target-platform> is bolted on in a few places.
You probably just missed one (likely in find_used_variables_in_text which @beckermr mentioned if that one hasn't been adjusted) and tests covering output of variant selection used in smithy.

@mbargull
Copy link
Member

Presumably fixed with conda/conda-build#5195 (if CI confirms the added test passes).
(Since {{ stdlib(...) }} is an as of yet not used feature, this might not make it into a 24.1.x patch release but should be fixed for conda-build=24.3 which I guess will be the next release some time next month.)

@h-vetinari
Copy link
Member Author

(Since {{ stdlib(...) }} is an as of yet not used feature, this might not make it into a 24.1.x patch release but should be fixed for conda-build=24.3 which I guess will be the next release some time next month.)

I'm aware of that of course, though the "not yet used feature" argument goes both ways - there's no risk in backporting a fix.

In any case, if conda-build decides not to backport, I'll try to make the case for a conda-forge-specific backport on the feedstock. There's large body of work backed up on this (e.g. to unblock the macOS situation, resp. fulfill our announcement to move to cos7 in the summer), so I think there would be good arguments also conda-forge-internally. And if even that fails, I'll wait for 24.3. ;-)

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

Successfully merging a pull request may close this issue.

3 participants