Add jinja templating, payload as tar, tests by lrandersson · Pull Request #2 · lrandersson/constructor

lrandersson · 2026-02-05T16:12:49Z

Description

This PR is a continuation of conda#1164, but split into a separate PR to hopefully simplify review, summarizing the changes:

Add jinja templating for post_install.bat and pre_uninstall.bat (although this file is currently empty).
Move write_pyproject_toml into class Payload
Add new class TemplateFile to better organize files that are templated using jinja.
Add unit tests

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?

lrandersson · 2026-02-05T16:18:25Z

constructor/briefcase.py

+
+        archive_path = dst / self.archive_name
+
+        with tarfile.open(archive_path, mode="w:gz", compresslevel=1) as tar:


I chose tar.gz here over tar.bz2, the reason is that MSI already compresses the contents, therefor the format of the compression does not do much of a difference in terms of disk space. However, .tar.gz is in general a little bit faster over .tar.bz2, so I have intentionally chose speed here over disk space.

Could you add some benchmarks to this?

constructor/briefcase/run_installation.bat

constructor/briefcase.py

lrandersson · 2026-02-05T16:20:23Z

constructor/template_file.py

+from .jinja import render_template
+
+
+@dataclass(frozen=True)


We can utilize this later also for the other installers.

This seems to overly abstract. The name property appears to be too tailor-made (which other format needs it besides MSI) and then we just have a source and destination path. With all my other comments, I'm not sure we need this class.

The render_template_files function could then just handle one file and deal with all the file handling operations (which I think is nice).

marcoesters

My initial thoughts on this. Overall, I think we could do with a little less abstraction here.

constructor/briefcase/run_installation.bat

marcoesters · 2026-02-05T22:28:57Z

constructor/briefcase/run_installation.bat

+
+rem Truncates the payload to 0 bytes
+type nul > "%PAYLOAD_TAR%"


Why not delete it? That zero-byte file doesn't need to be there until the uninstallation.

There should probably also be a comment about why this is needed.

marcoesters · 2026-02-05T22:30:29Z

constructor/briefcase.py

+
+        archive_path = dst / self.archive_name
+
+        with tarfile.open(archive_path, mode="w:gz", compresslevel=1) as tar:


Could you add some benchmarks to this?

marcoesters · 2026-02-06T17:12:40Z

tests/test_briefcase.py

+def test_payload_templates_are_rendered():
+    """Test that templates are rendered when the payload is prepared."""
+
+    def assert_no_jinja_markers(path: Path) -> None:


That function looks simple enough to be inlined.

marcoesters · 2026-02-06T17:21:14Z

constructor/briefcase.py

    root: Path | None = None
+    archive_name: str = "payload.tar.gz"
+    conda_exe_name: str = "_conda.exe"
+    rendered_templates: list[TemplateFile] | None = None


I don't know if we want to make that distinction. Rendered templates are like any other file we add to the installer payload.

marcoesters · 2026-02-06T17:23:09Z

constructor/briefcase.py

+
+        return archive_path
+
+    def _convert_into_archive(self, src: Path, dst: Path) -> Path:


That function could be folded into make_tar_gz or vice versa

marcoesters · 2026-02-06T17:35:28Z

constructor/briefcase.py

+        """Render all configured Jinja templates into the payload root directory.
+        The set of successfully rendered templates is recorded on the instance and returned to the caller.
+        """
+        root = self._ensure_root()


This is what I meant about either using __init__ to create the directory or not allowing a None value/non-existing directory. We have to manually add self._ensure_root() in all appropriate places.

marcoesters · 2026-02-06T17:36:31Z

constructor/briefcase.py

+            },
+        }
+        # Render the template files and add them to the necessary config field
+        rendered_templates = self.render_templates()


I think this should go into prepare like with any other payload preparation. Since we will always have pre_uninstall and pre_install scripts for our installers, we can add those hard-coded into the config object above.

marcoesters · 2026-02-06T17:37:53Z

constructor/template_file.py

@@ -0,0 +1,26 @@
+from collections.abc import Mapping


Let's merge the two template functions into one. There doesn't seem to be enough here to justify a separate module.

marcoesters · 2026-02-06T17:54:45Z

constructor/template_file.py

+from .jinja import render_template
+
+
+@dataclass(frozen=True)


This seems to overly abstract. The name property appears to be too tailor-made (which other format needs it besides MSI) and then we just have a source and destination path. With all my other comments, I'm not sure we need this class.

The render_template_files function could then just handle one file and deal with all the file handling operations (which I think is nice).

lrandersson · 2026-02-06T18:59:00Z

@marcoesters To answer above comments.
The requested benchmarking, using a dummy tar with 800 files of 32kB each, below are the results for different compression levels:

cl=1: 0.225s, size=209.1 KiB
cl=3: 0.215s, size=210.1 KiB
cl=6: 0.264s, size=116.9 KiB
cl=9: 0.285s, size=108.3 KiB

I just pushed a bunch of commits that did change a lot based on the review, I hope to summarize it:

Removed template_file.py, merged the rendering functions into one
Using cached properties now for both Payload.root and Payload.rendered_templates. Let me know what you think, this also removes the call to _ensure_root(). I think this looks pretty good and we don't get side-effects from class instantiation (as mentioned in the other PR).
Updated the scripts run_installation.bat and pre_uninstall.bat, we now call conda-standalone instead of `tar.
Folded/merged the make_tar_gz functionality.
Inlined the test utility function to verify we are free from unrendered jinja tokens.

marcoesters

Thank you! I think it's much closer to being ready now.

marcoesters · 2026-02-06T21:21:02Z

constructor/briefcase.py

+                    "post_install_script": str(self.rendered_templates["post_install_script"].dst),
+                    "pre_uninstall_script": str(self.rendered_templates["pre_uninstall_script"].dst),


Suggested change

"post_install_script": str(self.rendered_templates["post_install_script"].dst),

"pre_uninstall_script": str(self.rendered_templates["pre_uninstall_script"].dst),

"post_install_script": layout.root / "post_install.bat",

"pre_uninstall_script": layout.root / "pre_uninstall.bat",

We know where these files go and that location is not configurable, so we might as well hard-code them here. That also gives layout even more of a purpose and makes the TemplateParameter dataclass obsolete and you don't need to store the template files in the class (the fact that they are templates isn't that important, I think).

Applied here 3327439

marcoesters · 2026-02-06T21:23:22Z

constructor/briefcase.py

+    def make_tar_gz(self, src: Path, dst: Path) -> Path:
+        """Create a .tar.gz of the directory 'src'.


I think it would overall be good see a benchmark of tar.gz vs. tar.bz2 in terms of space and extraction speed, especially compared to the native MSI compression. While registry handling becomes much easier with one file, it would be good to say what the cost is.

Agree, will do that separately.

marcoesters · 2026-02-06T21:24:15Z

constructor/briefcase/run_installation.bat

-"%INSTDIR%\_conda.exe" install --offline --file "%BASE_PATH%\conda-meta\initial-state.explicit.txt" -yp "%BASE_PATH%"
+"%CONDA_EXE%" install --offline --file "%BASE_PATH%\conda-meta\initial-state.explicit.txt" -yp "%BASE_PATH%"
+
+rem Delete the payload to save disk space, a truncated placeholder of 0 bytes is recreated during uninstall


Suggested change

rem Delete the payload to save disk space, a truncated placeholder of 0 bytes is recreated during uninstall

rem Delete the payload to save disk space.

rem A truncated placeholder of 0 bytes is recreated during uninstall

rem because MSI expects the file to be there to clean the registry.

Applied here 3327439

lrandersson · 2026-02-06T21:33:42Z

@marcoesters Applied the fixes here 3327439

MSI: Update uninstall scripts

lrandersson · 2026-02-19T17:41:04Z

@marcoesters I think I've addressed all of your comments. Note that this doesn't go into main but into the next (final) branch.

marcoesters

Once the canary issue is solved, we should take a closer look at logging. Here are some other comments.

marcoesters · 2026-02-20T22:47:54Z

constructor/briefcase.py

+    # There might be other ways we want to enable `add_debug_logging`, but it has proven
+    # very useful at least for the CI environment.
+    add_debug_logging: bool = bool(os.environ.get("CI")) and os.environ.get("CI") != "0"


constructor has a --debug flag. We shouldn't enable debugging on default in the CI because many installers are built and executed in CI environments.

Wouldn't it be a tedious design if you have to run the tests once to see if there is an error, and then enable this flag and rerun the tests again to see what the underlying issue is? Especially considering that testing times are already very long. Note also that the "additional logging" is only visible to us upon a test failure.

We should then improve our error reporting to make sure that in general, the underlying issue is clear from the error message. If the information is important, it shouldn't just be part of the debugging output.

That I fully agree with but this comes back to if we want to log to file by default or not. Perhaps the variable name is not serving its purpose here but this flag enables all the file logging during pre/post uninstall/install.
This is what we discussed earlier about the issue of not having access to anything similar to tee, which is why I at least enforced file logging during the CI environment otherwise there is no way to tell what goes wrong from an error.

I'd propose we keep the implementation but rename the variable to log_to_file

@marcoesters

Always log to file 3c91055 (note that there is a small section remaining that relates now to add_debug but now the variable name is more in line with what it does: it enables "echo on" and prints some variables to the log.

Generalize log files and move them to INSTDIR, remove within pre_uninstall.bat if necessary 556705a

constructor/briefcase.py

marcoesters · 2026-02-20T22:56:59Z

constructor/briefcase.py

-        shutil.rmtree(self.root)
+    def make_archive(self, src: Path, dst: Path) -> Path:
+        """Create an archive of the directory 'src'.
+        The inputs 'src' and 'dst' must both be existing directories.


Why do they need to exist already?

The directory src must exist since that is what we are trying to archive, but I guess dst does not explicitly need to exist, but since it's a public method I requested that as a defensive measure to avoid confusing tarfile or shutil errors, since I thought current implementation will at least raise a clear error message:

if not dst.is_dir(): raise NotADirectoryError(dst)

I would generally not require that a destination directory exists. It seems like a fragile design.

26ebcb3 (changed inputs to input in docstring in later commit)

marcoesters · 2026-02-20T22:58:04Z

constructor/briefcase.py

+        templates = [
+            TemplateFile(
+                src=BRIEFCASE_DIR / "run_installation.bat",
+                dst=self.root / "run_installation.bat",
+            ),
+            TemplateFile(
+                src=BRIEFCASE_DIR / "pre_uninstall.bat",
+                dst=self.root / "pre_uninstall.bat",
+            ),
+        ]


I'm not sure we need that information since the file names are all hard-coded. I don't know that we need a separate dataclass for it either since it just contains source and destination.

"I'm not sure we need that information" - are you referring to the return format of the function or something else?

And I agree that it is optional, the purpose of the current implementation is to be able to verify easily during testing that the templates exist and to avoid hardcoding any file-names more than in this function. I like to think of it as having an ability to check from the payload object what templates we have rendered.

Ideally I'm not 100% with the current design, there are many ways to do this. I'd either actually use the return value somewhere meaningful perhaps in prepare or design this slightly differently. If you have any other suggestions please let me know.

Note also that write_pyproject_toml indirectly depends on this function since it is referencing the files created by render_templates()

write_pyproject_toml does indirectly depend on the function, but it doesn't use it. I think creating a separate data class just to store source and destination is a little heavy-handed. After all, you only need the locations of the rendered template files.

If you want to return something to facilitate testing, I suggest returning a dictionary with the key being pre-/post-install and the value being the file location.

8c8cc7b. I returned simply a list, I thought it might be the simplest because we just want to confirm that they have been rendered as expected.

marcoesters · 2026-02-23T21:52:50Z

constructor/briefcase/pre_uninstall.bat

+
+{%- if add_debug %}
+rem Get the name of the install directory
+for %%I in ("%INSTDIR%") do set "APPNAME=%%~nxI"


Has this been tested with %INSTDIR% that ends in a path with spaces?

Yes, all of the tests actually contain spaces because the install directory is set as "name versionnumber-buildnumber"

Add jinja templating, payload as tar, tests

109693f

lrandersson mentioned this pull request Feb 5, 2026

MSI: Reorganize payload preparation conda/constructor#1164

Merged

3 tasks

lrandersson commented Feb 5, 2026

View reviewed changes

constructor/briefcase/run_installation.bat Outdated Show resolved Hide resolved

lrandersson commented Feb 5, 2026

View reviewed changes

constructor/briefcase.py Show resolved Hide resolved

lrandersson commented Feb 5, 2026

View reviewed changes

Update briefcase.py

5e97190

marcoesters reviewed Feb 6, 2026

View reviewed changes

lrandersson added 5 commits February 6, 2026 13:07

Inline test utility function

2019d4f

Remove payload tar, update pre_uninstall.bat

b80642e

Use conda-standalone for extracting tar

8aebd7c

Merge archive functions, update root as cached property

e4bc141

Remove template_file.py improve handling of templates

313f7f2

lrandersson requested a review from marcoesters February 6, 2026 18:59

lrandersson added 2 commits February 6, 2026 14:17

Add missing .dst

e642b75

Remove compresslevel arg

839e290

marcoesters reviewed Feb 6, 2026

View reviewed changes

Review fixes

3327439

lrandersson requested a review from marcoesters February 6, 2026 21:33

lrandersson added 9 commits February 9, 2026 08:15

Fix typo in file name causing build errors

3df7843

Dynamically set archive type from file name

2566f73

Rename class function and update docstring

938e027

Update uninstallation scripts

b2d2025

Update pre_uninstall.bat

81a75b4

Add logging

3300636

Improve log handling for msi tests

a813e17

Add register_envs

d64e6ab

Fix syntax error with remove

0f4d68d

lrandersson added 2 commits February 16, 2026 13:51

Ensure .exe test not running for MSI

d64a807

Properly disable test for MSI

c3d51fd

lrandersson mentioned this pull request Feb 16, 2026

MSI: Update uninstall scripts #3

Merged

3 tasks

lrandersson and others added 5 commits February 17, 2026 08:56

Add more tests and another check errorlevel

d9fa4f8

Make logging more neat

1d40e95

Removed all use of 'sanity'

f39a787

Updated test for MSI (remove pytest.skip)

60f85bb

Merge pull request #3 from lrandersson/dev-ra-753

2ad0d23

MSI: Update uninstall scripts

lrandersson added 2 commits February 20, 2026 08:59

Update to use CLI for newer conda-standalone

8a32292

Fix missing quote and properly use --log-file

84b0136

marcoesters reviewed Feb 20, 2026

View reviewed changes

lrandersson added 2 commits February 23, 2026 12:27

Docstring formatting

b501aab

pre-commit fix

c5237a3

lrandersson requested a review from marcoesters February 23, 2026 17:38

lrandersson added 2 commits February 23, 2026 13:26

Hopefully fix issue with conda-standalone canary

2c40ad5

Fix typo in workflow

6c542cd

lrandersson mentioned this pull request Feb 23, 2026

Ensure latest conda-standalone is used for canary builds conda/constructor#1173

Merged

3 tasks

marcoesters reviewed Feb 23, 2026

View reviewed changes

lrandersson added 4 commits February 24, 2026 08:38

Automatically create 'dst'

26ebcb3

Remove TemplateFile

8c8cc7b

Fix docstring

621a545

FIx pre-commit

6312aa7

lrandersson requested a review from marcoesters February 24, 2026 13:54

lrandersson added 2 commits February 24, 2026 13:48

Always log to file

3c91055

Generalize install/uninstall logs and move into INSTDIR

556705a

marcoesters approved these changes Feb 25, 2026

View reviewed changes

lrandersson merged commit 68a527e into dev-ra-798 Feb 26, 2026
15 checks passed


		archive_path = dst / self.archive_name

		with tarfile.open(archive_path, mode="w:gz", compresslevel=1) as tar:


		rem Truncates the payload to 0 bytes
		type nul > "%PAYLOAD_TAR%"


		return archive_path

		def _convert_into_archive(self, src: Path, dst: Path) -> Path:

		"post_install_script": str(self.rendered_templates["post_install_script"].dst),
		"pre_uninstall_script": str(self.rendered_templates["pre_uninstall_script"].dst),

		def make_tar_gz(self, src: Path, dst: Path) -> Path:
		"""Create a .tar.gz of the directory 'src'.

-rem Delete the payload to save disk space, a truncated placeholder of 0 bytes is recreated during uninstall
+rem Delete the payload to save disk space.
+rem A truncated placeholder of 0 bytes is recreated during uninstall
+rem because MSI expects the file to be there to clean the registry.

Conversation

lrandersson commented Feb 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist - did you ...

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcoesters left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lrandersson commented Feb 6, 2026

Uh oh!

marcoesters left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lrandersson commented Feb 6, 2026

Uh oh!

lrandersson commented Feb 19, 2026

Uh oh!

marcoesters left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lrandersson Feb 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lrandersson Feb 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

lrandersson commented Feb 5, 2026 •

edited

Loading

lrandersson Feb 23, 2026 •

edited

Loading

lrandersson Feb 24, 2026 •

edited

Loading

lrandersson Feb 24, 2026 •

edited

Loading

lrandersson Feb 23, 2026 •

edited

Loading

lrandersson Feb 24, 2026 •

edited

Loading