properly convert env interactions into context

[AaronOpfer]

This input:

{% set a = environ["a"] %}
{% set b = environ.get("b", "0") %}

package:
  name: abc
  version: 0

on main emits this recipe that needs manual correction:

schema_version: 1

context:
  a: "\"env.get(\"a\")\""
  b: "\"environ | get(\"b\", \"0\")\""

package:
  name: abc
  version: 0

but after this PR, it will instead emit:

schema_version: 1

context:
  a: ${{env.get("a")}}
  b: ${{env.get("b", default="0")}}

package:
  name: abc
  version: 0

which does not need any further editing.

This patch feels a little hacky, and I would especially like to immortalize my test case. I'm happy to accept feedback on how to go about making this patchset less hacky and immortalize my test case.

Thanks for your time.

[conda-bot]

We require contributors to sign our Contributor License Agreement and we don't have one on file for @AaronOpfer.

In order for us to review and merge your code, please e-sign the Contributor License Agreement PDF. We then need to manually verify your signature, merge the PR (conda/infrastructure#1077), and ping the bot to refresh the PR.

[travishathaway]

@conda-bot check

[AaronOpfer]

CLA is sorted now. It looks like CI is blocked on that.

[schuylermartin45]

Apologies for the delay, I was OOO late last week for medical reasons.

I'll try to take a look at this today. I think this is the first outside contribution to the Python code for this project.

I'll ask for some patience as we work out process issues, but I am super excited to see a community contribution!

[schuylermartin45]

It looks like the code formatter, linter, and static analyzer checks failed.

Let me know if you need assistance setting that up locally. Any feedback on the README or other documentation is very welcome. It would be nice to know if there is room for improvement!

[AaronOpfer]

These are my notes in no particular order:

• The make dev fails on up-to-date tooling if the conda-recipe-manager environment doesn't already exist as the conda command exits non-zero. I had to create a token env first to satisfy the makefile.
• The makefile doesn't work on my ancient CentOS7 make version 3.82. I needed to source make from conda-forge before I could proceed.
• Various tooling seems to be straddling defaults and conda-forge which is a no-no.
• Attempting to use the assisted build inside my company's conda configuration was impossible because we renamed all of the channels to make it more clear which ones are first-party vs third-party, and for various other reasons. As such every command that selects a specific conda channel (-c conda-forge) didn't work for me.

I think I got local dev working regardless but it wasn't painless.

[schuylermartin45]

I wouldn't worry about this being "hacky". The V0 recipe format is a mess, and to some extent, this library pays the price to be inherently messy so others don't have to be :)

[AaronOpfer]

You touched on it in a few places in your review so I will address it here:

The main problem that caused me to introduce all of these startswith hacks is that variables that are going to be moved into context: appear to lose information about whether their values were originally strings or expressions; this is then compensated for using a regex that seems to catch the common jinja operators. This has unhappy implications, such as the fact that the difference between {% set var = "env.get(...)" %} and {% set var = env.get(...) %} (string vs expression) are lost. This PR adds a hacky assumption that nobody ever is interested in setting a string that starts with env.get.

Ideally, we would instead make the recipe's context values a Union[ConstantStr, Expression] such that we don't have to create these strange special cases where we bail out of escaping, since these are inherently fragile.

I am not sure how much appetite you have for this though.

[schuylermartin45]

• The make dev fails on up-to-date tooling if the conda-recipe-manager environment doesn't already exist as the conda command exits non-zero. I had to create a token env first to satisfy the makefile.
• The makefile doesn't work on my ancient CentOS7 make version 3.82. I needed to source make from conda-forge before I could proceed.

If you could give more details on which version(s) of make were having issues, I would appreciate it. I think that bmake (Berkeley variant) has had some compatibility issues we've seen in the past. We install a known working make to environment to try to avoid some of this, but there's obviously a bootstrapping issue on first setup. At some point we will probably add pixi support to streamline this as an alternative setup mechanism. There is an outstanding PR that Wolf and I haven't had time to circle back on: #11

I suppose one could argue that we should edit the Makefile to be compatible with all make variants/versions. But I've run into many headaches in the past trying to get conda environments to play nice with make. It is not a fun time.

• Various tooling seems to be straddling defaults and conda-forge which is a no-no.

That's a stop-gap for waiting on packages to be made available. IMO that shouldn't have to be an issue, or at least conda should throw a fit if that is unsupported behavior. I'll have to have a think if we should prefer conda-forge over defaults.

• Attempting to use the assisted build inside my company's conda configuration was impossible because we renamed all of the channels to make it more clear which ones are first-party vs third-party, and for various other reasons. As such every command that selects a specific conda channel (-c conda-forge) didn't work for me.

I think I got local dev working regardless but it wasn't painless.

That's upsetting to hear, but not surprising with being a trailblazer with this PR. I'm not super familiar with such configurations. I personally maintain the cond-forge release for CRM. I believe another team inside Anaconda has started packaging it for defaults, but it will not be updated on the same cadence.

Thanks for the feedback!

[schuylermartin45]

You touched on it in a few places in your review so I will address it here:

The main problem that caused me to introduce all of these startswith hacks is that variables that are going to be moved into context: appear to lose information about whether their values were originally strings or expressions; this is then compensated for using a regex that seems to catch the common jinja operators. This has unhappy implications, such as the fact that the difference between {% set var = "env.get(...)" %} and {% set var = env.get(...) %} (string vs expression) are lost. This PR adds a hacky assumption that nobody ever is interested in setting a string that starts with env.get.

Ideally, we would instead make the recipe's context values a Union[ConstantStr, Expression] such that we don't have to create these strange special cases where we bail out of escaping, since these are inherently fragile.

I am not sure how much appetite you have for this though.

If I had infinite time and money, we could. But realistically, I am very resource constrained and have a lot on my plate.

startswith is fine, I just wasn't sure if we should prefer a ^get.env regex so we could easily add more edge cases as regex options. I'm not going to block the PR over that point.

We should add at least one unit test though for regression detection. I know you've had issues running this locally, so let me know if you have time to add the tests yourself or if you'd prefer for me to throw it on this PR/make a follow-up. The file of interest should be test_recipe_parser_convert.py and we'll need a new input and output file that contains a get.env statement.

[AaronOpfer]

I added test cases and I resolved your comments by adding breadcrumbs and making all references to env.get( use the same variation (env.get(). Let me know if there is anything else needed.

[schuylermartin45]

As an aside: it looks like we reduced our error count in the rattler-build conversion integration testing by 4. Nice.

Thanks for the contribution!