Added support for script_env_variables

[jlstevens]

Adds support for an extra_env_variables key in the construct.yaml e.g.:

extra_env_variables:
  - CUSTOM_VARIABLE_1=CUSTOM1
  - CUSTOM_VARIABLE_2=CUSTOM2

This makes these environment variables available to the pre_install and post_install scripts.

Motivation: This mechanism makes it much easier to template installers that use pre_install and post_install scripts, allowing those scripts to stay generic and unchanged while making the templated changes clear in the construct.yaml in an easy-to-read, declarative form.

Right now only Unix .sh installers are supported in this PR. I wanted to see whether to constructor maintainers are open to this addition before implementing support for other platforms.

The only open question I have right now is whether there is a use-case to allow custom overrides of the pre-defined variables (i.e. $INSTALLER_NAME, $INSTALLER_VER, $INSTALLER_PLAT). My instinct is that there is no good reason to allow this so this PR ensures that custom extra variable definitions cannot override these definitions.

• Add a file to the news directory (using the template) for the next release's release notes?
• Add / update necessary tests?
• Add / update outdated documentation?

[conda-bot]

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

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#822), and ping the bot to refresh the PR.

[jaimergp]

Thanks for bringing this up in such an accommodating way!

I am not opposed to the idea, but:

• It indeed needs to be ported to other platforms
• We need to have a well defined syntax and be clear about what's supported and what's not (e.g. referencing other variables, escaping characters, etc)
• We need to be very careful about spaces, quotes, special characters like $, which might have different rules in different platforms.
• The field name should be clear about only affecting the pre and post install scripts, not the resulting installation.

In general I wonder if this is better solved with an example that documents how to run a pre-processing script to template your pre and post scripts on your own, without direct support in constructor. Given the amount of edge cases that can arise, it's tricky to come up with a generalized approach that works in all cases.

[jlstevens]

I am not opposed to the idea

Glad to hear it!

It indeed needs to be ported to other platforms

Agreed. My last commit adds support to Windows.

We need to have a well defined syntax and be clear about what's supported and what's not (e.g. referencing other variables, escaping characters, etc). We need to be very careful about spaces, quotes, special characters like $, which might have different rules in different platforms.

Yes, we will need clear documentation. I think it would be fine to only support simple literals initially and apply strict validation of the construct.yaml.

The field name should be clear about only affecting the pre and post install scripts, not the resulting installation.

Got any suggestions? Would script_extra_env_variables be a better name?

In general I wonder if this is better solved with an example that documents how to run a pre-processing script to template your pre and post scripts on your own, without direct support in constructor. Given the amount of edge cases that can arise, it's tricky to come up with a generalized approach that works in all cases.

While I agree that would be useful for even more complex use cases, I think the idea of passing in additional environment variables into the scripts improves on a fairly simple and clearly defined interface, especially as constructor is already defining environment variables for consumption by these scripts. I think your suggestion is a good one but orthogonal to what is proposed in this PR.

[jlstevens]

@jaimergp Thanks for the review!

I am happy with all your suggestions. Would you like me to apply them?

[jlstevens]

I went ahead and applied all your suggestions.

If this seems good to you, I'll go ahead and start tackling the docs.

[jaimergp]

Got any suggestions? Would script_extra_env_variables be a better name?

Let's go with script_env_variables. I think extra is implied.

---

The tests are passing and sufficient, so yea let's go and document this nicely, emphasizing on the fact that only literals are supported and that escaping, if needed, it's on the user (e.g. spaces require quoting à la `var: '"value with spaces"'), while accounting for YAML itself. That might be a bit annoying though, so we can also document that we always quote the value (update needed in the code), so inner quotes need to be escaped. I think the latter is more reasonable (because spaces would be more common than quotes in a value, I hope).

So, questions to tackle in the docs, depending on what we do in the code.

On Unix, I'd say we should always quote the value because not doing it causes more trouble than it's worth. However:

• If we double quote the value, we are allowing runtime interpolation with $VAR and friends. This might be both desired and undesired (some people might want to have a variable that's like PRICE: "$1" but that means something else in bash, so they'd need PRICE: "\\$1".
• This makes me prefer single quoting, but that means no string interpolation. I think this is fine as long as it's documented. If people want more control, they can always manually reprocess their script before calling constructor.

WDYT?

[jlstevens]

Let's go with script_env_variables. I think extra is implied.

Agreed. Implemented in 7d43220

This makes me prefer single quoting, but that means no string interpolation. I think this is fine as long as it's documented.

I agree. Single quoting seems safer and I think will be appropriate for the vast majority of use-cases. Once you get your values into your pre-/post-install scripts you can do your string interpolation in there.

On Unix I guess this just means wrapping the value from the dictionary in ' which I can do easily enough. Unfortunately, I don't know anything about quoting on Windows (at least not offhand) so I would have look into it. Do you know what kind of quoting would work best for Windows?

[jaimergp]

Unfortunately, I don't know anything about quoting on Windows (at least not offhand) so I would have look into it. Do you know what kind of quoting would work best for Windows?

I think we are fine with the current code. The env var is being set directly via Windows APIs, not CMD. So we don't need to do anything else (we are already using str_esc). But, just to be safe, we should add a few ugly variable values to the test example. Add values with single quotes, double quotes, spaces, dollars, and percentages.

[jlstevens]

I've updated the scripts example with a more challenging string. You can use whatever characters you want now except for single quote themselves which you can escape using '\'' - I have documented this in CONSTRUCT.md.

Now I will test this out on Windows with the .bat files.

[jaimergp]

I have documented this in CONSTRUCT.md

In case you are doing this manually, the workflow is to edit construct.py and then run scripts/make_docs.py to sync all the generated copies.

[jlstevens]

Getting special characters to work in the Windows environment variable seems to be trickier:

• Spaces are handled fine.
• str_esc handles $ by turning it into $$
• Surprisingly # seems to be treated as a yaml comment and gets dropped (at least in my windows environment). I don't get why this isn't an issue on Unix. Edit: I got confused, Unix and Windows behave the same and # can't be used as it denotes a yaml comment which gets ignored. I guess this isn't surprising after all.
• Any quotes (single or double) truncate the string

So until I figure it out, my Windows test string is the same as the Unix one but without quotes which I haven't manage to handle.

I am not sure how much effort should be spent on allowing quotes on Windows: I would be tempted to simply document that they are not supported (or I could apply some validation in the code...but I can't for # and anything after it as I don't have access to the whole string!)

[dbast]

@conda-bot check

[jlstevens]

@jaimergp All checks are now green!

Ready for review.

[jlstevens]

Thanks for reviewing @dbast!