conda / ceps

Conda Enhancement Proposals
Creative Commons Zero v1.0 Universal
19 stars 24 forks source link

CEP: Jinja functionality in v2 recipes #71

Open wolfv opened 3 months ago

h-vetinari commented 3 months ago

In https://github.com/conda/conda-build/issues/5053 you had said:

I do think that for the future (ie. rattler-build) we'll have an upcoming CEP to define this feature better.

Is this that CEP? In any case, I don't see {{ stdlib(...) }} in the diff, and I think it needs to be covered in some way, given that we're now rolling this out at scale.

wolfv commented 3 months ago

Yep, this is the one. Do you want to have it as a separate function or rolled into compiler? I prefer to roll it into compiler...

h-vetinari commented 3 months ago

Yep, this is the one. Do you want to have it as a separate function or rolled into compiler? I prefer to roll it into compiler...

I'd prefer a separate function, because it's less magic to have to understand (i.e. "why do I need to specify c_stdlib_version and how does that affect my build environment?" is much easier to answer if there is a {{ stdlib("c") }} under build:); also, the coupling of compiler and stdlib versions is entirely incidental (at least for C), so I'd like to not conflate them conceptually.

wolfv commented 3 months ago

My points:

I can imagine two behaviors:

Note that while you are correctly pointing out a dislike about magical behavior, we are printing a lot more information, clearly displayed, with rattler-build. So we can make it obvious where the stdlib comes from (we already point out that the clangxx_osx-arm64 package comes from the compiler, for example).

And lastly, I just think it's a matter of writing really good documentation.

h-vetinari commented 3 months ago

I think recipes would continue to work without adding the stdlib (correct?).

Define "work". When we increase the c_stdlib_version on linux and macos later this year, the artefacts produced by a recipe without {{ stdlib("c") }} would continue to work on modern systems, but would be broken on systems with a too-old C standard library, because it's through the meta-packages pulled in by stdlib that we set the correct run-exports in the artefact metadata.

It would only affect very old systems, but I'd still consider that broken.

So it's something they should add but don't have to add. For compiler it's clear - the recipe will not work without a compiler.

For me it's the opposite here - a compiled recipe needs (in 99.9% of cases) a C standard library just as much as it needs a compiler. Up until now this was just an ambient assumption built-in to our infrastructure, but I'd prefer people to learn this at the same time as the necessity of {{ compiler(...) }} - the two go hand in hand (as implied by having the same "c" argument, and the same way the keys are constructed), even though the relevant versions are completely independent from each other.

I can imagine two behaviors:

  • We add additional arguments to the jinja function, so that compiler('c', stdlib='glibc').

I really dislike this, because it would replace the nicely universal

    - {{ compiler('c') }}
    - {{ stdlib('c') }}

with a horrible mess of platform-specific selectors (or however the syntax for that ends up looking like in the new recipe format):

    - {{ compiler('c', stdlib='glibc') }}                     # [linux]
    - {{ compiler('c', stdlib='macosx_deployment_target') }}  # [osx]
    - {{ compiler('c', stdlib='vs') }}                        # [win]
  • We control the stdlib behavior from the variant_config. This means, if the variant config contains entries for c_stdlib / c_stdlib_version we use them from the compiler function.

I agree that this is technically feasible, and with good output and documentation would be a workable solution. When I say I prefer to keep {{ stdlib("c") }} explicit, this is not an all-or-nothing statement, but more a 70:30 kind of preference over the implicit handling through the compiler.

One reason that pushes this from a 60:40 (based on "less magic") to a 70:30 preference is that the C stdlib has a special role, in the sense that it can become necessary even without an explicit C compiler. In other words, we'd have to cover cases where a recipe with only {{ compiler("cxx") }} or {{ compiler("rust") }} also includes c_stdlib{,_version}.

wolfv commented 2 months ago

@h-vetinari since we don't have a CEP for stdlib it might be good to define it here.

What are the default values for the different operating systems?

h-vetinari commented 2 months ago

since we don't have a CEP for stdlib it might be good to define it here.

Sounds good!

What are the default values for the different operating systems?

I was under the impression that defaults here aren't necessarily a good thing? Perhaps even more than the compiler packages, the stdlib-metapackages don't have a "canonical" naming. Are you planning to set defaults for {{ compiler(...) }} as well? Looking at the diff here it doesn't seem like there are default being proposed?

In any case, here are the specs that we're using in conda-forge. They won't change substantially anymore, barring unforeseen events.

c_stdlib:
  - sysroot                    # [linux]
  - macosx_deployment_target   # [osx]
  - vs                         # [win]
m2w64_c_stdlib:                # [win]
  - m2w64-toolchain            # [win]              <-- might change to m2w64-sysroot in the future
c_stdlib_version:              # [unix]
  - 2.17                       # [linux]            <-- per July 2024
  - 10.13                      # [osx and x86_64]
  - 11.0                       # [osx and arm64]
wolfv commented 2 months ago

I expanded the content a bit.

Some things that are left to decide:

schuylermartin45 commented 2 months ago
  • Limit the filters from Jinja to a default subset?
  • Should we add a split(str) filter (useful for version numbers, so that one can do "${{ version | split(".") | slice(0, 2) | join(".") }} or something along those lines. We could also create a more specialized function/filter for version string manipulation of this sort.
  • Should env work more like Python env

    • env["FOO"] instead of env.get("FOO")
    • env.get("FOO", "default") instead of env.get_default("FOO", "default")
  • Or should it be more like Github Actions

    • env.FOO instead of env.get("FOO")
    • env.FOO or "default" for default values

I can't speak as much for the needs of package builders, but I will say I think any allowed JINJA syntax needs to minimized. It adds a significant level of complication in the development of automated tooling (namely package editing). But if we must have them, we should limit the syntax and make the rules of using the features very clear. The more variety available, the more developers will use the tools in "clever" ways, the more breakages we will have when trying to automatically edit recipes.

jezdez commented 2 weeks ago

@conda/steering-council

This vote falls under the "Enhancement Proposal Approval" policy of the conda governance policy, please vote and/or comment on this proposal at your earliest convenience.

It needs 60% of the Steering Council to vote yes to pass.

To vote, please leave yes, no or abstain as comments below.

If you have questions concerning the proposal, you may also leave a comment or code review.

This vote will end on 2024-07-16, End of Day, Anywhere on Earth (AoE). This is an extended voting period due to summer holiday time in the Northern Hemisphere.

baszalmstra commented 2 weeks ago

yes

wolfv commented 2 weeks ago

Please use the following form to vote:

@xhochy (Uwe Korn)

@cj-wright (Christopher J. 'CJ' Wright)

@mariusvniekerk (Marius van Niekerk)

@goanpeca (Gonzalo Peña-Castellanos)

@chenghlee (Cheng H. Lee)

@ocefpaf (Filipe Fernandes)

@marcelotrevisani (Marcelo Duarte Trevisani)

@msarahan (Michael Sarahan)

@mbargull (Marcel Bargull)

@jakirkham (John Kirkham)

@jezdez (Jannis Leidel)

@wolfv (Wolf Vollprecht)

@jaimergp (Jaime Rodríguez-Guerra)

@kkraus14 (Keith Kraus)

@baszalmstra (Bas Zalmstra)

wolfv commented 6 days ago

@marcelotrevisani @jakirkham @mbargull @CJ-Wright last chance to vote - you can also choose abstain which would still help with the quorum.

schuylermartin45 commented 6 days ago

NP on the title of this PR: Since this is schema_version: 1 I've been referring to this as the V1 recipe format and the pre-CEP-13 format as V0. Do we want to keep consistent with that nomenclature?

I'm also not sure if it is worth codifying somewhere to prevent confusion. Maybe it is also worth it to add a section to the top of the CEP that specifies which schema_version a CEP belongs to, to prevent future confusion?

chenghlee commented 5 days ago

For the record: voted "no" because while I like the ideas presented in this CEP, I don't think the CEP as written is ready to be adopted as a specification. IMO, various unanswered questions/comments need to be addressed before adoption (e.g., top-down evaluation of the context object).

wolfv commented 50 minutes ago

The vote has been closed with the following result:

Total voters: 15 (valid: 13 = 86.67%)

Yes votes (11 / 84.62%):

No votes (1 / 7.69%)):

Abstain votes (1 / 7.69%):

Not voted (2):

Invalid votes (0):

We reached quorum, and enough YES votes for this CEP to be accepted. 🎉