MSI: Update uninstall scripts by lrandersson · Pull Request #3 · lrandersson/constructor

lrandersson · 2026-02-10T15:34:05Z

Description

Note that this PR builds upon #2

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?

constructor/briefcase.py

tests/test_examples.py

lrandersson · 2026-02-17T13:59:22Z

tests/test_examples.py

+            original_exception=e,
+        )
+
+    # Sanity check the installation directory


This is not really needed but something I had use of while debugging last week. Let me know if I should remove it.

I think this can be removed to keep the code cleaner.

Forgot to respond to this one, removed f39a787

lrandersson · 2026-02-17T14:00:16Z

tests/test_examples.py

    input_path = _example_path("register_envs")  # The specific example we use here is not important
    options = ["/InstallationType=JustMe", f"/NoRegistry={no_registry}"]
    for installer, install_dir in create_installer(input_path, tmp_path):
+        if installer.suffix == ".msi":


This test is for the .exe flag /NoRegistry so it's not applicable for the .msi installers.

Why not just change the installer type to exe in the construct.yaml file then? Using pytest.skip will skip the whole test. At least, you should use continue.

We can't use continue because it will fail on the assert statements that follow after.
I wonder if we should move this test into a separate file, some background: The test is running the example register_envs, which was picked arbitrarily when creating the test because we just needed any example in order to test the /NoRegistry flag. Therefore changing the installer type is not feasible because we still need the installer type set to all for test_example_register_envs. We could also create a new example for exe only, but that doesn't seem to be the correct setup since the test is only intended for /NoRegistry.

This could also be solved in a more neat way using proper parametrization (related conda#1146)

The assertions won't fail because they are not inside the for-loop. The test seems to be written with the assumption that only exe installers are created for Windows. Using continue will create an MSI installer, but nothing will be done with it.

Yes that's correct. I don't see however how using continue and moving the assert statements into the for-loop makes the test-skip more explicit over the current approach. It is unfortunately already clunky as is since the installer needs to be created in order to identify what kind of installer type we are testing.

The difference is that currently, your tests are marked as skipped: https://github.com/lrandersson/constructor/actions/runs/22183340055/job/64150179614?pr=3#step:12:82

The loop will first create the exe installer (due to the sorting) and then get to the msi installer. So, the tests aren't even executed for the exe installer since they are outside the for-loop.

I agree that it is clunky. We don't even need to move the test into the for-loop because in the end as long as we ensure that the exe installer is the only installer that has been run. I think this is the only thing we need to add:

if installer.suffix != "exe": continue

I see, that's tedious. but it makes sense now when you say that.
I updated it here 60f85bb, I did indent the assert anyway because I tried the continue approach earlier last week and it will fail on the assert statement for the second parametrized test (because it will find the program in the 'Installed Apps Menu').

lrandersson · 2026-02-17T14:00:44Z

.github/workflows/main.yml

          CONSTRUCTOR_EXAMPLES_KEEP_ARTIFACTS: "${{ runner.temp }}/examples_artifacts"
          CONSTRUCTOR_SIGNTOOL_PATH: "C:/Program Files (x86)/Windows Kits/10/bin/10.0.26100.0/x86/signtool.exe"
-          CONSTRUCTOR_VERBOSE: 1
+          CONSTRUCTOR_VERBOSE: 0


This I'll change back to 1 later.

lrandersson · 2026-02-17T14:02:56Z

constructor/briefcase/pre_uninstall.bat

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


I'm not really happy with this solution of setting the file name here, since it is also explicitly set in test_examples.py, ideally it would be only defined in one place instead of two as it is right now (same also applies for postinstall.log). But it does suffice for now and helped tremendously while debugging last week, I also don't see any need to change this line in the near future since it just works.

Is there a way to access the log file for the MSI installer somehow?

Do you mean accessing it from the CI pipeline? Otherwise no, only by opening the file, I think it's a bit clunky, MSI has "support" for logging but it seems very generic since as you might have seen that we enable logging for MSI Installers via cmd.extend(["/L*V", str(log_path)]) - this doesn't include the logging for pre/post install/uninstall.

Yes, that's why I'm asking if there is any avenue that we can access log_path from within these scripts.

Ah, it could work but it's risky. We can end-up with encoding mismatch, MSI log is usually UTF-16. cmd.exe echo writes in the console codepage (effectively “ANSI”/OEM), so you’ll end up mixing encodings in one file.
We can also end-up with weird/corrupted log files from the MSI engine trying to write at the same time as the scripts.

A problem to solve for a different PR, but I would really like to see those logged in the same file. Maybe we need to extend the logger to detect the file encoding of an existing log file and change it accordingly? The documentation is a little sparse about what happens when encoding is None.

Is the risk for race-condition really there? Would an MSI installer even write to log while the script is running?

To answer this correctly, I think we need to do a lot of testing. I agree something for a different PR.

marcoesters · 2026-02-17T15:28:50Z

constructor/briefcase/pre_uninstall.bat

+"%CONDA_EXE%" --version >> "%LOG%" 2>&1
+{%- endif %}
+
+{%- set redir = ' >> "%LOG%" 2>&1' if add_debug else '' %}


conda-standalone has logging functionality you could use - it essentially works like the Tee-Object commandlet.

tests/test_examples.py

marcoesters · 2026-02-17T15:41:43Z

tests/test_examples.py

    input_path = _example_path("register_envs")  # The specific example we use here is not important
    options = ["/InstallationType=JustMe", f"/NoRegistry={no_registry}"]
    for installer, install_dir in create_installer(input_path, tmp_path):
+        if installer.suffix == ".msi":


Why not just change the installer type to exe in the construct.yaml file then? Using pytest.skip will skip the whole test. At least, you should use continue.

marcoesters · 2026-02-17T15:46:12Z

constructor/briefcase/pre_uninstall.bat

+{%- set redir = ' >> "%LOG%" 2>&1' if add_debug else '' %}
+{%- set dump_and_exit = 'type "%LOG%" & exit /b %errorlevel%' if add_debug else 'exit /b %errorlevel%' %}
+
+rem Sanity checks


Can we call these "consistency checks" instead of "sanity checks"?

I still see some instances

marcoesters · 2026-02-17T15:47:11Z

constructor/briefcase/pre_uninstall.bat

+if not exist "%CONDA_EXE%" (
+  {% if add_debug %}echo [ERROR] CONDA_EXE not found: "%CONDA_EXE%" >> "%LOG%" & type "%LOG%" & {% endif %}exit /b 10
+)


I think if we error out, we should always add a message. So, I'd skip the entire check if not debugging or always add the message.

Why don't we just always add to the log file? That answer may depend on how we deal with the MSI vs post-install logging inconsistency.

We cant add everything that is outputted from the scripts to the log file while also displaying the output "live" to the person who runs the installer, see my answer in #3 (comment) (the first bullet).

Or we might be able to achieve "always write to file" (but not the same file for the MSI engine + pre/post uninstall/install), with the following approach:

<all calls to conda-standalone utilize --log-file> // because it seems like its writing to stdout and well as the file? I've only tried so far with conda run python -c 'raise Exception(...)'

All bat-file "echo ..." needs to be written twice, once without redirection and one with redirection

The workaround I used for NSIS was to write to a step-log file and then type to screen and append to the main log. I think logging will need its own PR though - it's a bigger challenge than anticipated.

marcoesters · 2026-02-17T15:47:39Z

constructor/briefcase/pre_uninstall.bat

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


Is there a way to access the log file for the MSI installer somehow?

marcoesters · 2026-02-17T15:48:56Z

constructor/briefcase/pre_uninstall.bat

+{%- endif %}
+
+{%- set redir = ' >> "%LOG%" 2>&1' if add_debug else '' %}
+{%- set dump_and_exit = 'type "%LOG%" & exit /b %errorlevel%' if add_debug else 'exit /b %errorlevel%' %}


I think we should always create a log if possible. The log level should just determine what goes into the log. We do have to make sure it gets deleted in the end though.

@marcoesters thanks for your first set of review comments. I thought I'd respond here first because the current design is due to some limitations I ran into last week, and I think we need to discuss this before I make any additional changes.

I agree that we should always log to a file, unfortunately it doesn't seem possible to do this without redirecting all visible output from the user (the person who runs the installer) - when logging to a file, all you see is an empty console that closes after a while. I looked into this last week and the only way to use tee (or similar) is through PowerShell, which makes us "stuck" with above. Therefore I'm a bit reluctant to log all output to a file by default for the average user. Any thoughts on this?

You mentioned; "conda-standalone has logging functionality you could use - it essentially works like the Tee-Object commandlet.". Will this also log exceptions that could occur when conda.exe is running? If yes this sounds like a good idea, the current approach will also make any stack traces from conda.exe visible when the tests are running in our CI environment. This is also how I discovered that we the canary builds use an older version of conda-standalone, because they caused stack traces that were not visible without redirecting.

This is how the logger is set up: https://github.com/conda/conda-standalone/blob/00deca6accc6fd0b9dae8346f125f1ad1d107524/src/entry_point.py#L134-L164

So, it should catch all stdout and stderr, but you could try by running conda.exe python -c 'raise RuntimeError("Error message")' --log-file <path to log file> and see what happens.

I just tried it and unfortunately it doesn't add unhandled exceptions to the log file.

As we found, that was an incorrect example, but it does work.

constructor/briefcase/run_installation.bat

lrandersson added 3 commits February 10, 2026 10:33

Update uninstallation scripts

b2d2025

Update pre_uninstall.bat

81a75b4

Add logging

3300636

lrandersson force-pushed the dev-ra-753 branch from 9ddb969 to 3300636 Compare February 16, 2026 13:25

lrandersson added 5 commits February 16, 2026 09:50

Improve log handling for msi tests

a813e17

Add register_envs

d64e6ab

Fix syntax error with remove

0f4d68d

Ensure .exe test not running for MSI

d64a807

Properly disable test for MSI

c3d51fd

lrandersson commented Feb 17, 2026

View reviewed changes

constructor/briefcase.py Show resolved Hide resolved

Add more tests and another check errorlevel

d9fa4f8

lrandersson commented Feb 17, 2026

View reviewed changes

tests/test_examples.py Show resolved Hide resolved

lrandersson commented Feb 17, 2026

View reviewed changes

tests/test_examples.py Show resolved Hide resolved

lrandersson commented Feb 17, 2026

View reviewed changes

lrandersson requested review from marcoesters February 17, 2026 14:00

lrandersson assigned lrandersson and unassigned lrandersson Feb 17, 2026

lrandersson commented Feb 17, 2026

View reviewed changes

marcoesters reviewed Feb 17, 2026

View reviewed changes

lrandersson added 3 commits February 18, 2026 09:38

Make logging more neat

1d40e95

Removed all use of 'sanity'

f39a787

Updated test for MSI (remove pytest.skip)

60f85bb

marcoesters approved these changes Feb 19, 2026

View reviewed changes

lrandersson merged commit 2ad0d23 into dev-ra-798-2 Feb 19, 2026
15 checks passed

Conversation

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

Uh oh!

Description

Checklist - did you ...

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lrandersson Feb 17, 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!

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 19, 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!

lrandersson commented Feb 10, 2026 •

edited

Loading

lrandersson Feb 17, 2026 •

edited

Loading

lrandersson Feb 19, 2026 •

edited

Loading