add cep for definition of new keys & values in build format #56

wolfv · 2023-06-14T15:59:35Z

No description provided.

cep-20.2.md

marcelotrevisani · 2023-06-14T16:23:10Z

cep-20.2.md

+  license_url: url
+
+  # REMOVED:
+  # prelink_message:


this bit here was added for EULA like stuffs that needs confirmation from the user, I believe NVIDIA or/and intel folks use it? or at least they were keen to have it. I would be careful in removing this one :)

Yes I 100% agree. We have to keep this.

I suppose we could use prelink scripts? @jakirkham can comment for the nvidia stuff.

They aren't using it on conda-forge (I checked)

I am also keen on dropping the post-link, pre-link, and post-unlink keys in build.

The keys in the recipe. We would keep the files.

Prelink_message is mainly for eula purposes that the user needs to accept it, it is not related to the prelink and postlink script that are, as far I'm concerned, deprecated

prelink and postlink script that are, as far I'm concerned, deprecated

post-link and pre-unlink should only be deprecated if similar functionality is offered elsewhere.

The keys in the recipe. We would keep the files.

Personally, I'm not a big fan of the "some file relative to meta.yaml/recipe.yaml triggers some implicit behavior" thing.
(I'd even go as far as removing the implicit invocation of build.sh/bld.bat -- but that might upset people 🤷.)

Was there a resolution to this for packages that want / need to display some kind of EULA message or even want to require some kind of interaction from a user, i.e. accepting a EULA?

NVIDIA legal seems to be appeased currently, but from experience these decisions can change, and there will likely be uses outside of NVIDIA packages as well.

It feels like the "if a file named in this particular way is found then we do X" behaviour will stay, but the keys to change how that file is named will not... If we do so, I'd rather we do it like we treat activation scripts (their target location is the trigger, not the source one); but that doesn't work well for pre-link & post-unlink because the files will be gone.

carterbox · 2023-06-14T18:37:07Z

cep-20.2.md

+      script:
+        script: ${{ "build_mamba.sh" if unix else "build_mamba.bat" }}


Suggested change

script:

script: ${{ "build_mamba.sh" if unix else "build_mamba.bat" }}

script: ${{ "build_mamba.sh" if unix else "build_mamba.bat" }}

Was this a mistake?

Also, on a related note, I've always found it confusing that script: can either be a string that is the name of a file which is a script or it can be a list of strings which will be converted into a bash or bat script. What can this new format do to make this less confusing?

Yep, that was a mistake, thanks!

I see two ways of achieving this, either by having another level in the dict, e.g.

script: file: myfile.sh # or content: | ....

Or having an additional key, like script_file vs script or something like that.

Ok, maybe a third one would be something like: script: ${{ embed("build.sh") }} and write a jinja function that pulls in the contents of the file. This could be quite useful for other files as well (e.g. README, ...).

I have to say that I like the current syntax a lot but I get that it is confusing.

This could potentially allow choosing the interpreter to use:

script: shell: bash file: build.sh

e.g. https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idstepsshell

I have to say that I like the current syntax a lot but I get that it is confusing.

@beckermr Could you elaborate on why you like that script: is a keyword with two different meanings?

I like not having to remember the different names for the different things.

Actually my first thought when I saw script: ${{ "build_mamba.sh" if unix else "build_mamba.bat" }} was: "can't we have a default that abstracts away the different file extensions?"

Something like¹:

script: build_mamba # will use .sh on unix and .bat on win

If someone needs differently named scripts, they could always drop down to selectors, but it would make one more source of noise go away in the vast majority of cases.

Footnotes

if we need to disambiguate from script: for compatibility (is this relevant in a new format that's already breaking things?), we could name the key script_auto_ext: or something like that. ↩

Actually the logic for that could be (since build_mamba / build_mamba.sh / etc. are just strings): If $script + ".sh" exists (on unix; .bat on windows) execute it, if it doesn't exist, assume the extension is already in the script, i.e. execute $script.

cep-20.2.md

jaimergp · 2023-06-15T07:28:16Z

cep-20.2.md

+    preserve_egg_dir: bool (default false)
+
+    # only used as a hack to down-prioritize packages
+    track_features: [string]


Is it a good moment to rename this (even if we use track_features under the hood)?

Do we have an actual proposal for what to call the next thing to replace track features? Maybe that has to come first?

I would propose priority or downprioritize and then an integer (positive or negative)

jaimergp · 2023-06-15T07:30:16Z

cep-20.2.md

+
+```yaml
+# url pointing to the source tar.gz|zip|tar.bz2|...
+url: url


This can be a list of URLs too (mirrors of the same package with the same hash; used by R packages)

jaimergp · 2023-06-15T07:32:07Z

cep-20.2.md

+  # extra run dependencies
+  run: [MatchSpec]
+# extra files to add to the package for the test
+# Alternatively, use ${{ RECIPE_DIR }} or ${{ SOURCE_DIR }} and a single list?


I like this better, imo.

I have mixed feelings. I always found files and source_files ambiguous. Your proposed format would help. I also like your alternatives, but I feel like it could cause ambiguity. For example, if we allow ${{ RECIPE_DIR }}, does it mean that we can also do /absolute/path/to/file?

jaimergp · 2023-06-15T07:32:41Z

cep-20.2.md

+
+```yaml
+# list of imports to try
+imports: [string]


As we move to an language agnostic world, should this be renamed to python_imports for clarity?

Or we do

python: imports: - mypkg - mypkg.foo

With the potential to add more keys, e.g. pytest or others?

That could work, yes. I think pytest would be better handled as a command anyway, but maybe there are other Python specific checks to be made.

I'm thinking renaming imports to python is sufficient:

test: python: - mypkg - mypkg.foo - mypkg.foo:function rust: - mypkg - mypkg::foo - mypkg::foo::function

Anything more complex is probably better off in a script?

jaimergp · 2023-06-15T07:35:23Z

cep-20.2.md

+        - ${{ compiler('cxx') }}
+        - cmake
+        - ninja
+        - ${{ "python" if win }}


What's the returned value happens when there's no else clause and the if clause is not true? null?

Yes, a Yaml::Null value (that we filter out in these ConditionalList).

cep-20.2.md

Co-authored-by: jaimergp <jaimergp@users.noreply.github.com>

dhirschfeld · 2023-06-15T12:33:35Z

One thing that I would love to have is an env_vars section which could then be used to automatically create activate and deactivate scripts for all shells which would take care of backing up existing values and setting the new ones on activation and restoring the backed up versions on deactivation.

e.g.

env_vars:
  - DOTNET_ROOT=$CONDA_PREFIX/lib/dotnet
  - DOTNET_TOOLS=$CONDA_PREFIX/lib/dotnet/tools

Potentially a big enough change to deserve it's own CEP though 🤔

AntoinePrv · 2023-06-15T13:51:33Z

cep-20.2.md

+    # if it is only one element and ends with `.sh` or `.bat`
+    script: string | [string]


Does this means build.sh and bld.bat are not automatically detected and need to be explicitly mentioned?
I think that would be better to make it explicit

wolfv · 2023-06-15T13:55:13Z

@baszalmstra did some awesome work at making a pydantic model for the schema presented in this PR. What's awesome is that it allows us to get feedback and inline-help in the VSCode editor (check out this video) with all the red squiggly lines:

Screen.Recording.2023-06-15.at.15.36.55.mov

The repository for the pydantic file and the JSON schema is here: https://github.com/prefix-dev/recipe-format

If you start a new YAML file in VSCode and add the following line to the top, you should have it, too:

# yaml-language-server: $schema=https://raw.githubusercontent.com/prefix-dev/recipe-format/main/schema.json

We also changed a couple of definitions. Most notably the script_env is now:

- script_env:
    # list of environment variables to pass through to the script
    passthrough: [string]
    # dictionary of key/value of environment variables to set in the script
    env: {string: string}
    # list of secrets to pass through from the environment
    secrets: [string]

In the about section we renamed a couple of fields:

description is now either 
description:
  file: path # path to read description from, e.g. ${SRC_DIR}/README.md

 -- or --  
description: string

renamed:

home -> homepage
doc_url -> documentation
dev_url -> repository

And we split the definition of multi-output recipes into a "complex recipe".
Different rules apply to complex aka multi-output recipes:

toplevel package field can only have a version (that is used for all outputs, if they do not define their own version)
toplevel build field can only have a number that is used for all outputs if they do not define their own build-number
toplevel test is disallowed
toplevel about and source are still allowed, although we're not sure if we should keep the toplevel source or if that should be done via one of these cache outputs

AntoinePrv · 2023-06-15T13:59:02Z

Two more things I could think of:

Versioning the recipe format

We could have a metadata section but version is the only useful thing that comes to mind.

recipe:
  version: "1.0"

Parameters

Maybe that would be it's own CEP, but I think it could be valuable to pass values to the recipe from conda-build/boa/ratter-build.
Two use case that come to mind:

Pass a path to a directory to a ccache folder reused between builds (could also be done with forwarding an env variable)
Dispatch source between an url and a local directory so that feedstocks can be tested locally in project (mamba proves this is currenlty a mess).

carterbox · 2023-06-15T16:50:08Z

I agree with @AntoinePrv that a new format should be accompanied by a version metadata so that parsers can explicitly query which version of the format they are parsing.

dhirschfeld · 2023-06-15T23:31:02Z

I agree with @AntoinePrv that a new format should be accompanied by a version metadata

Perhaps named schema_version (or similar) as version is a bit overloaded.

wolfv · 2023-06-16T08:33:25Z

An alternative to having the schema_version in the file would be to encode the schema version in the filename (recipe.v1.yaml etc...). But I am fine with having an explicit schema version in the recipe (I think docker-compose does it, too).

jezdez · 2023-06-21T08:28:58Z

An alternative to having the schema_version in the file would be to encode the schema version in the filename (recipe.v1.yaml etc...). But I am fine with having an explicit schema version in the recipe (I think docker-compose does it, too).

I'm in favor of having it inside the file rather than the filename carry it

dhirschfeld

Not sure why I've got special status as a listed reviewer, but FWIW

xhochy · 2024-01-05T07:55:51Z

Yes

ocefpaf · 2024-01-05T09:17:11Z

Yes

wolfv · 2024-01-10T09:52:31Z

@CJ-Wright @goanpeca @chenghlee @marcelotrevisani @jakirkham @mbargull @kkraus14 @jaimergp this is a reminder to please vote with YES / NO / ABSTAIN on this CEP :)

mbargull · 2024-01-10T17:03:36Z

Could you add a comment on the removed options saying that replacements for them will be re-introduced later?

ping on this

mbargull

Yes

jaimergp

I have some comments before casting my vote (which will be yes).

It feels like the current writeup needs some proof-reading. I found some typos and out of date examples that should be sorted out before accepting the CEP.
Some ongoing conversations have not been resolved with a explicit decision. It's fine if those are not tackled in the present CEP, but at least a comment should be made mentioning that they will (or not) be worked on in the future.
The top-level script vs build/script explanation is not clear right now.
A couple sections could use some more details.
Some cosmetic changes.

I also have some minor comments that are a matter of taste and uncontroversial (e.g. path vs file). Feel free to ignore those.

cep-20.2.md

jaimergp · 2024-01-11T15:49:28Z

cep-20.2.md

+
+We propose a new recipe format that is heavily inspired by conda-build. The main
+change is a pure YAML format without arbitrary Jinja or comments with semantic
+meaning.


This probably needs something more specific to this particular part. Not a blocker though.

cep-20.2.md

jaimergp · 2024-01-11T16:46:23Z

cep-20.2.md

+  license_file: LICENSE
+  summary: A fast drop-in alternative to conda, using libsolv for dependency resolution
+  description: Just a package manager
+  repository: https://github.com/mamba-org/mamba


Suggested change

repository: https://github.com/mamba-org/mamba

development: https://github.com/mamba-org/mamba

jaimergp · 2024-01-11T16:48:54Z

cep-20.2.md

+  # pre-link: path
+```
+
+### Script section


Is this a top-level section or the "value type" for the script key in build?

Seems to be build/script based on the Markdown header depth, but yeah, I think that should be clarified.

Co-authored-by: jaimergp <jaimergp@users.noreply.github.com>

goanpeca · 2024-01-16T17:46:59Z

Yes 👍🏼

jaimergp · 2024-01-17T15:36:35Z

Yes.

wolfv · 2024-01-17T19:44:32Z

@CJ-Wright @chenghlee @marcelotrevisani @kkraus14

if you could please vote (even if you vote with abstain), that would be great!!

wolfv · 2024-01-17T19:45:36Z

Forgot @jakirkham

marcelotrevisani · 2024-01-17T19:57:38Z

Yes

chenghlee · 2024-01-17T20:53:57Z

cep-20.2.md

+The version can be explicitly set by adding a `schema_version` key to the recipe.
+
+```yaml
+# optional, since implicitly defaults to 1
+schema_version: 1 # integer
+```


In the old-format meta.yaml or the new-format recipe.yaml? If the latter, I'm inclined to make schema_version required, rather than optional, to avoid future "we can never update the default schema version" issues.

chenghlee · 2024-01-17T20:59:54Z

cep-20.2.md

+
+    # ignore all or specific files for prefix replacement
+    # was `ignore_prefix_files`
+    ignore: bool | [path] (defaults to false)


Would this be better as bool | [glob], rather than requiring explicit paths?

chenghlee · 2024-01-17T21:02:10Z

cep-20.2.md

+    # force the file type of the given files to be TEXT or BINARY
+    # for prefix replacement


For clarity, we should define what TEXT and BINARY mean in this context.

cep-20.2.md

chenghlee · 2024-01-17T21:06:59Z

cep-20.2.md

+  # The file to use as the script. Automatically adds `bat` or `sh` to the filename
+  # on Windows or UNIX respectively (if no file extension is given).


I'm not sure I like the idea of automatically appending a extension, especially on *nix where file extensions historically have had no particular semantics. I'm not sure why conda build should care if build-my-pkg is written in any particular language, especially if I've already specified interpreter. (E.g., if I've provided interpreter: "/usr/bin/env python" and specify file: foo, I don't want conda build trying to run python foo.sh.)

If you want to run a python file you should just specify the extension foo.py? I think it's probably ok to not support running files without an extension?

chenghlee · 2024-01-17T21:18:17Z

cep-20.2.md

+  # pre-link: path
+```
+
+### Script section


Seems to be build/script based on the Markdown header depth, but yeah, I think that should be clarified.

cep-20.2.md

chenghlee · 2024-01-17T21:23:42Z

cep-20.2.md

+#### Python test element
+
+The python test element renders a `test_import.py` file that contains the imports to test.
+It also automatically runs the `pip check` command to check for missing dependencies.


I'm also not keen on automatically running pip check, especially since optional features and dependencies are a thing. Need to dig out the specific package name, but we have run into issues where pip check failed because we (as packagers) explicitly decided to turn off a feature but had no way of informing pip of that change.

chenghlee · 2024-01-17T21:25:48Z

cep-20.2.md

+    requirements:
+      # same definition as top level
+
+    # same definitions as on top level, by default merged from outer recipe


cep-20.2.md

chenghlee · 2024-01-17T21:30:47Z

Left comments, but other than _maybe) making schema_version: N required, none of them would block a "yes" vote from me.

CJ-Wright · 2024-01-18T01:38:26Z

Yes
I am making this comment solely in my personal capacity and am not conveying any rights to any intellectual property of any third parties.

wolfv · 2024-01-19T12:30:27Z

Hi everyone! I have counted the votes:

12 yes
~~I am counting Cheng as 1 abstain~~ Update: Cheng is also a yes!
No no votes

That means 13 people have voted. The council consists of 15 people.

That means that: 86% have voted, and 100% have voted yes! This CEP is accepted! 🎉

chenghlee · 2024-01-19T16:31:04Z

@wolfv: you can count me as a "yes". 😄

add cep for definition of new keys & values in build format

e4c9809

beckermr reviewed Jun 14, 2023

View reviewed changes