MSI: Reorganize payload preparation by lrandersson · Pull Request #1164 · conda/constructor

lrandersson · 2026-02-04T15:55:31Z

Description

This PR proposes a class Payload to organize the payload preparation.
I still think more work can be done on this but I tried to keep the changes minimal to simplify the review.
I have also added a couple of unit tests to demonstrate what I think is the biggest gain of using this structure over the old one.

I think this class can be extracted later and also be used for other installers, since we can make it possible (if needed) to override the function that creates the payload layout.

For the review, it might be easier to also open the whole file via "View file" and not only browsing from the "Files changed" tab.

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:14:40Z

I'm tagging @marcoesters here because I assume you will review this out of the possible reviewers. I just wanted to inform you that there is an additional PR that depends on this one, that also is in need of review. lrandersson#2, this PR is on my own fork because I need to merge that PR into this one (PR 2 -> PR 1164).

marcoesters · 2026-02-05T21:46:39Z

constructor/briefcase.py

-def create(info, verbose=False):
-    if not IS_WINDOWS:
-        raise Exception(f"Invalid platform '{sys.platform}'. MSI installers require Windows.")
+@dataclass(frozen=True)


Why use a dataclass over NamedTuple?

I'd say that they are very similar but in my opinion this more comes down to how we use this. The class PayloadLayout represents one thing, and we can add methods to it as well. I tried to keep it minimal for presentation. For a tuple also positional meaning is primary, but for the frozen data class the fields are primary.

I'm suggesting a NamedTuple, not a tuple. A NamedTuple also goes by fields and is immutable. I don't see that we will need methods for a structure that just describes a layout.

I see, what would the gain be to use a NamedTuple over a dataclass in this situation? I agree that we still get the data organized in a similar fashion but they signal different intent, and my thought process is:

I'd use a NamedTuple when the thing is a tuple.

I'd use frozen dataclass when the thing represents a thing.

Overall, a NamedTuple is a smaller object. I use a NamedTuple when I just need a simple immutable data container. However, I'm still not fully convinced we need this (yet). See my comment on the perpare function below.

marcoesters · 2026-02-05T22:12:42Z

constructor/briefcase.py

+    def remove(self) -> None:
+        shutil.rmtree(self.root)


Should there be a guard in case the directory doesn't exist (or an ignore_errors=True)?

I thought about it, but, I think it would make the most sense that the one who implements the interface is the one who should care about potential errors. Silently catching/ignoring errors here could lead to unexpected behavior.

By the way I have now guards for this since Payload.root is now a cached property in lrandersson#2

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

constructor/briefcase.py

+    def _ensure_root(self) -> Path:
+        if self.root is None:
+            self.root = Path(tempfile.mkdtemp())
+        return self.root


This should be part of __init__ or we shouldn't allow None root. I don't see an advantage having a potentially "unbound" root directory.

That is an option, the reason why I have done as above is to avoid filesystem side effects upon class instantiation. The caller ideally only need to use this as:

payload = Payload(info) payload.prepare()

However, if we are fine with above the side effects, let me know and I will update it. It is a minor thing to change in terms of code.

marcoesters · 2026-02-05T22:23:02Z

constructor/briefcase.py

+        self._stage_dists(layout)
+        self._stage_conda(layout)
+        return layout


This part makes me think that PayloadLayout may be overkill. _stage_dists and _stage_conda only use one element of PayloadLayout and only root is used outside of that function, so the entire object doesn't need to be returned.

Why not combine _create_layout and prepare into one function and remove the PayloadLayout data class? This seems simpler for now.

If we find that we want to use this class later for other installers, we can create these interfaces later.

I see your point, but I think it'll be a bit more "clear" after reviewing the follow-up PR https://github.com/lrandersson/constructor/pull/2/changes where more code has been added.

I like to think about it this way, alternative 1 (which we are very close to in the PR I linked):

self.render_templates(layout) self.write_pyproject_toml(layout) self.archive(layout) self._stage_dists(layout) self._stage_conda(layout)

Compared to alternative 2:

external = root / "external" base = external / "base" pkgs = base / "pkgs" self.render_templates(root, external) self.write_pyproject_toml(root, external) self.archive(base, external) self._stage_dists(pkgs) self._stage_conda(external)

And if for some reason we need to add more directories as arguments to any of the functions, one does not need to change the function definitions, only the implementation of each function.
To me it doesn't seem overkill but for maintenance and ease of implementation, alternative 1 is much simpler. Please let me know your thoughts.

Alternative 1 already lists a few MSI-specific function calls, like preparing the pyproject.toml file (which is technically not part of the installer payload anyway). The SH format structures its payload differently (preconda, postconda), and EXE installers have temp_extra_files.

I am overall in favor of a more object-oriented way rather than the current scripting appraoch constructor have, and alternative 1 looks reasonable at first glance.

However, I am concerned about introducing premature abstraction now that we may have to undo later. I think for now, alternative 2 is good enough for MSI. When there is earnest effort in making constructor more OOP-based, we can take a holistic look at all payloads and find what an appropriate level of abstraction is.

I see your point. Do you mind if we take this action as the last thing after merging lrandersson#2 into this PR? That way we can also see a simple diff between the two layouts given the current implementation.
(It will also be easier for me to merge the second PR that way).

@marcoesters I've now added a commit that basically is the same functionality but without the class PayloadLayout for comparison: 6af0910, let me know what you think.

Ping @marcoesters this should be the last open thread of this PR

I can't say that I miss the PayloadLayout class, honestly.

marcoesters · 2026-02-05T22:25:24Z

constructor/briefcase.py

+        """The layout is created as:
+        root/
+        └── external/
+            └── base/
+                └── pkgs/
+        """


I think this is a good opportunity to elaborate a little more on why this directory structure, specifically the intermediary external directory is needed.

Added here c4b11e8, note that there is also a path to external in the pyproject.toml that is generated, but I didn't explicitly mention that in the docstring.

MSI: Update uninstall scripts

Add jinja templating, payload as tar, tests

marcoesters · 2026-03-03T16:48:35Z

constructor/briefcase/pre_uninstall.bat

+"%CONDA_EXE%" --log-file "%LOG%" constructor uninstall --prefix "%BASE_PATH%"
+if errorlevel 1 ( exit /b %errorlevel% )
+
+rem If we reached this far without any errors, remove any log-files.


Suggested change

rem If we reached this far without any errors, remove any log-files.

rem If we reached this far without any errors, remove any log files.

marcoesters · 2026-03-03T16:49:42Z

constructor/briefcase.py

+    def remove(self, *, ignore_errors: bool = True) -> None:
+        """Remove the root of the payload.
+
+        This function requires some extra care due to the root being a cached property.


Suggested change

This function requires some extra care due to the root being a cached property.

This function requires some extra care due to the root directory being a cached property.

marcoesters · 2026-03-03T16:50:10Z

constructor/briefcase.py

+        try:
+            delattr(self, "root")
+        except Exception:
+            # delattr on a cached_property may raise on some versions / edge cases


For example? And why is passing the appropriate course of action?

There are a few ways to look at it:

If this fails, the temporary directory is already removed which is the primary goal.

The payload.remove() function here is intended to be called as the final step (currently inside create(...))
It is likely that we could skip the entire delattr shenanigan, but to me it looks more complete to avoid a reference to a directory that might not exist.
I was also thinking of writing Payload to be used as a context-manager to achieve automatic clean-up but I thought it might be a bit overkill right now.

marcoesters · 2026-03-03T17:01:09Z

constructor/briefcase/pre_uninstall.bat

+  {{ error_block('Failed to create "%PAYLOAD_TAR%"', '%errorlevel%') }}
+)
+
+"%CONDA_EXE%" --log-file "%LOG%" constructor uninstall --prefix "%BASE_PATH%"


Since this assumes the --log-file feature, I think we need to add a check to ensure that it exists. That information exists in info, we just need to raise when it's False

Great idea, see 1ed9fe0

* Reorganize payload into a class and add tests * Ensure new tests are Windows only * Add jinja templating, payload as tar, tests * Update docstring * Update briefcase.py * Inline test utility function * Remove payload tar, update pre_uninstall.bat * Use conda-standalone for extracting tar * Merge archive functions, update root as cached property * Remove template_file.py improve handling of templates * Add missing .dst * Remove compresslevel arg * Review fixes * Fix typo in file name causing build errors * Dynamically set archive type from file name * Rename class function and update docstring * Update uninstallation scripts * Update pre_uninstall.bat * Add logging * Improve log handling for msi tests * Add register_envs * Fix syntax error with remove * Ensure .exe test not running for MSI * Properly disable test for MSI * Add more tests and another check errorlevel * Make logging more neat * Removed all use of 'sanity' * Updated test for MSI (remove pytest.skip) * Update to use CLI for newer conda-standalone * Fix missing quote and properly use --log-file * Docstring formatting * pre-commit fix * Hopefully fix issue with conda-standalone canary * Fix typo in workflow * Automatically create 'dst' * Remove TemplateFile * Fix docstring * FIx pre-commit * Always log to file * Generalize install/uninstall logs and move into INSTDIR * pre-commit fix * Remove PayloadLayout * Fix remaining syntax error * Review fixes

Reorganize payload into a class and add tests

13c2c05

lrandersson requested a review from a team as a code owner February 4, 2026 15:55

lrandersson changed the base branch from main to briefcase-integration February 4, 2026 15:59

conda-bot added this to 🔎 Review Feb 4, 2026

github-project-automation bot moved this to 🆕 New in 🔎 Review Feb 4, 2026

conda-bot added the cla-signed [bot] added once the contributor has signed the CLA label Feb 4, 2026

lrandersson added 2 commits February 4, 2026 11:30

Ensure new tests are Windows only

8619a37

Add jinja templating, payload as tar, tests

109693f

lrandersson mentioned this pull request Feb 5, 2026

Add jinja templating, payload as tar, tests lrandersson/constructor#2

Merged

3 tasks

marcoesters reviewed Feb 5, 2026

View reviewed changes

lrandersson added 7 commits February 6, 2026 08:31

Update docstring

c4b11e8

Update briefcase.py

5e97190

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 19:02

lrandersson added 11 commits February 6, 2026 14:17

Add missing .dst

e642b75

Remove compresslevel arg

839e290

Review fixes

3327439

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

lrandersson and others added 21 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

Update to use CLI for newer conda-standalone

8a32292

Fix missing quote and properly use --log-file

84b0136

Docstring formatting

b501aab

pre-commit fix

c5237a3

Hopefully fix issue with conda-standalone canary

2c40ad5

Fix typo in workflow

6c542cd

Automatically create 'dst'

26ebcb3

Remove TemplateFile

8c8cc7b

Fix docstring

621a545

FIx pre-commit

6312aa7

Always log to file

3c91055

Generalize install/uninstall logs and move into INSTDIR

556705a

Merge pull request #2 from lrandersson/dev-ra-798-2

68a527e

Add jinja templating, payload as tar, tests

pre-commit fix

8a9be10

Remove PayloadLayout

6af0910

Fix remaining syntax error

56aa4a4

marcoesters reviewed Mar 3, 2026

View reviewed changes

Review fixes

1ed9fe0

lrandersson requested a review from marcoesters March 3, 2026 17:45

marcoesters approved these changes Mar 3, 2026

View reviewed changes

github-project-automation bot moved this from 🆕 New to ✅ Approved in 🔎 Review Mar 3, 2026

lrandersson merged commit 9f0b620 into conda:briefcase-integration Mar 4, 2026
33 of 34 checks passed

github-project-automation bot moved this from ✅ Approved to 🏁 Done in 🔎 Review Mar 4, 2026

	rem If we reached this far without any errors, remove any log-files.
	rem If we reached this far without any errors, remove any log files.

	This function requires some extra care due to the root being a cached property.
	This function requires some extra care due to the root directory being a cached property.

Conversation

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

Uh oh!

Description

Checklist - did you ...

Uh oh!

lrandersson commented Feb 5, 2026

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!

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 Feb 6, 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!

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!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lrandersson commented Feb 4, 2026 •

edited

Loading

lrandersson Feb 6, 2026 •

edited

Loading