Skip to content

Conversation

@marcoesters
Copy link
Contributor

Description

#892 introduced a small regression by missing a few template variables for PKG installers. In fixing this regression, I decided to expand the Jinja templating as described in #901.

Specifically, this PR removes custom Python code with Jinja templating logic, which removed a lot of custom Python formatting functions on Windows and simplified the data structures.

Additionally, I patched a few testing issues that broke tests for local test executions and when using the defaults channel.

Closes #901

Checklist - did you ...

  • 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 conda-bot added the cla-signed [bot] added once the contributor has signed the CLA label Jan 6, 2025
@marcoesters marcoesters marked this pull request as ready for review January 7, 2025 03:36
@marcoesters marcoesters requested a review from a team as a code owner January 7, 2025 03:36
{{ SCRIPT_ENV_VARIABLES }}

{%- for key, val in SCRIPT_ENV_VARIABLES | items %}
System::Call 'kernel32::SetEnvironmentVariable(t,t)i("{{ key }}", {{ val }}).r0'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we need quotes for val too?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ok they are pre-escaped in Python. Let's rename this for clarity in the future then:

Suggested change
System::Call 'kernel32::SetEnvironmentVariable(t,t)i("{{ key }}", {{ val }}).r0'
System::Call 'kernel32::SetEnvironmentVariable(t,t)i("{{ key }}", {{ escaped_val }}).r0'

(and in the line above too)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was surprised, too, but we don't do it in the current version either: https://github.com/conda/constructor/blob/main/constructor/winexe.py#L97

@marcoesters marcoesters requested a review from jaimergp January 8, 2025 15:44
@marcoesters marcoesters merged commit 0c80f9b into conda:main Jan 10, 2025
16 checks passed
@marcoesters marcoesters deleted the jinja2-improvements branch March 20, 2025 22:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla-signed [bot] added once the contributor has signed the CLA

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

Leverage Jinja better

3 participants