ceps icon indicating copy to clipboard operation
ceps copied to clipboard

CEP: Jinja functionality in v1 recipes

Open wolfv opened this issue 1 year ago • 16 comments

wolfv avatar Apr 12 '24 15:04 wolfv

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.

h-vetinari avatar Apr 12 '24 20:04 h-vetinari

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

wolfv avatar Apr 13 '24 08:04 wolfv

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.

h-vetinari avatar Apr 13 '24 23:04 h-vetinari

My points:

  • There are already two variables for compilers (c_compiler and c_compiler_version). Adding additional 2 variables (that are non-mandatory) is fine IMO, especially if it's well-documented.
  • It complicates things for beginners. I think recipes would continue to work without adding the stdlib (correct?). 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.
  • In most cases, it will always be added when there is a compiler.

I can imagine two behaviors:

  • 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.
  • We add additional arguments to the jinja function, so that compiler('c', stdlib='glibc').

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.

wolfv avatar Apr 14 '24 05:04 wolfv

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

h-vetinari avatar Apr 14 '24 08:04 h-vetinari

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

wolfv avatar May 20 '24 17:05 wolfv

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]

h-vetinari avatar May 20 '24 22:05 h-vetinari

I expanded the content a bit.

Some things that are left to decide:

  • 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
  • Should ${{ compiler('c') }} evaluate directly to gcc_linux-64 or should there be an intermediate representation (as is now)
  • For pin_subpackage it will be impossible to evaluate it directly (due to self-referential nature of the function).
  • Should we add lower_bound and upper_bound to the pin functions? And how should they work (in conjunction with min_pin and max_pin)?
  • The final name for the cmp function.

wolfv avatar May 21 '24 16:05 wolfv

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

schuylermartin45 avatar May 21 '24 17:05 schuylermartin45

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

jezdez avatar Jul 02 '24 13:07 jezdez

yes

baszalmstra avatar Jul 02 '24 13:07 baszalmstra

Please use the following form to vote:

@xhochy (Uwe Korn)

  • [x] yes
  • [ ] no
  • [ ] abstain

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

  • [ ] yes
  • [ ] no
  • [ ] abstain

@mariusvniekerk (Marius van Niekerk)

  • [x] yes
  • [ ] no
  • [ ] abstain

@goanpeca (Gonzalo Peña-Castellanos)

  • [x] yes
  • [ ] no
  • [ ] abstain

@chenghlee (Cheng H. Lee)

  • [ ] yes
  • [x] no
  • [ ] abstain

@ocefpaf (Filipe Fernandes)

  • [x] yes
  • [ ] no
  • [ ] abstain

@marcelotrevisani (Marcelo Duarte Trevisani)

  • [ ] yes
  • [ ] no
  • [ ] abstain

@msarahan (Michael Sarahan)

  • [x] yes
  • [ ] no
  • [ ] abstain

@mbargull (Marcel Bargull)

  • [ ] yes
  • [ ] no
  • [x] abstain

@jakirkham (John Kirkham)

  • [x] yes
  • [ ] no
  • [ ] abstain

@jezdez (Jannis Leidel)

  • [x] yes
  • [ ] no
  • [ ] abstain

@wolfv (Wolf Vollprecht)

  • [x] yes
  • [ ] no
  • [ ] abstain

@jaimergp (Jaime Rodríguez-Guerra)

  • [x] yes
  • [ ] no
  • [ ] abstain

@kkraus14 (Keith Kraus)

  • [x] yes
  • [ ] no
  • [ ] abstain

@baszalmstra (Bas Zalmstra)

  • [x] yes
  • [ ] no
  • [ ] abstain

wolfv avatar Jul 02 '24 13:07 wolfv

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

wolfv avatar Jul 16 '24 10:07 wolfv

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?

schuylermartin45 avatar Jul 16 '24 12:07 schuylermartin45

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

chenghlee avatar Jul 16 '24 16:07 chenghlee

The vote has been closed with the following result:

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

Yes votes (11 / 84.62%):

  • @xhochy (Uwe Korn)
  • @mariusvniekerk (Marius van Niekerk)
  • @goanpeca (Gonzalo Peña-Castellanos)
  • @ocefpaf (Filipe Fernandes)
  • @msarahan (Michael Sarahan)
  • @jakirkham (John Kirkham)
  • @jezdez (Jannis Leidel)
  • @wolfv (Wolf Vollprecht)
  • @jaimergp (Jaime Rodríguez-Guerra)
  • @kkraus14 (Keith Kraus)
  • @baszalmstra (Bas Zalmstra)

No votes (1 / 7.69%)):

  • @chenghlee (Cheng H. Lee)

Abstain votes (1 / 7.69%):

  • @mbargull (Marcel Bargull)

Not voted (2):

  • @cj-wright (Christopher J. 'CJ' Wright)
  • @marcelotrevisani (Marcelo Duarte Trevisani)

Invalid votes (0):

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

wolfv avatar Jul 22 '24 15:07 wolfv