Repository: pyinstaller/pyinstaller
Pull request: bootloader: fix splash screen shutdown issues
This PR starts by adding bunch of new tests for splash screen - in particular, its *shutdown*. The one test that we had up until now is, in fact, "interactive test", where we expect the application to be alive for at least the specified amount of time (10 seconds), before we *terminate* it. Would the frozen application shut down without crashing? Would the onefile build leak its temporary directory? [Those who know less sleep better...](https://github.com/rokm/pyinstaller/actions/runs/9384136834/job/25839289530).

We then instrument the bootloader's cleanup code with couple of additional debug messages in an attempt to pin-point the issues that the new tests flushed out.

Turns out that in splash screen cleanup code, we are unloading Tcl shared library before Tk one. Reverting the order seems to deal with crashes in the new tests. However, the `DllMain` function in Tk seems to contain provisions for Tcl being unloaded before Tk; although on the other hand, the surrounding comments imply that this is considered an abnormal situation... Either way, the correct order seems to improve the situation, so let's go with it. And keep an eye on CI to see if we have any sporadic failures of those tests.

The above seems to take care of all issues but one - now the onefile variant of new tests seems to be consistently failing on test-msys2 (mingw32, i686). The frozen application is failing to clean up its temporary directory, and with the recently-refactored bootloader, this now triggers an error on our CI (as per `PYINSTALLER_STRICT_UNPACK_MODE` environment variable).

Again, we add couple of debug messages to make debugging such problems easier, and we learn that `libgcc_s_dw2-1.dll` and `libwinpthread-1.dll` cannot be removed. Those are dependencies of `tcl86.dll`, so we are dealing with a variant of #7106 - loading Tcl and Tk DLLs for splash screen loads additional DLLs into the process, but when we unload Tk and Tcl, those additional DLLs are not unloaded, and thus they become un-removable.

So as a work-around, if we fail to remove the temporary directory, we now enumerate all loaded DLLs, look up their file name to identify the ones that originate from the application's temporary directory, and attempt to force-unload them by repeatedly calling `FreeLibrary` until the call fails. And then try removing the temporary directory again. As questionable and hacky as this sounds, it does seem to do the trick; both for msys2/mingw32 and for UPX-processed Tcl/Tk DLLs - so we can close #7106.


Problem statement:
Splash mode leaves VCRUNTIME140.dll in temporary directory
Using PyInstaller 5.4.1 (`--splash`, `--onefile`, and UPX) on Windows 11. After quitting the application a single file `VCRUNTIME140.DLL` is left in `$Env:TEMP`:
```
 _MEI88802/
└──  __splash/
    └──  VCRUNTIME140.dll*
```
This is obviously not a big issue since the file is only about 100 KB but it should be fixed so the whole `_MEI*` directory is deleted.

Hints from issue discussion:
Hmm, you're right, but I can reproduce this only with UPX enabled. And it seems to work as expected if I exempt tcl/tk DLLs from UPX processing (`--upx-exclude tcl86t.dll --upx-exclude tk86t.dll`).

So what's probably happening is that upx-processed tcl/tk DLLs fail to release their reference to vcruntime140.dll (or unload it) when we unload them during splash screen cleanup, so we fail to remove it due to it still being marked as in use. I'll see if we can work around this (for example by trying to manually unload vcruntime140.dll in the splash screen cleanup).

Commit message: tests: move splash screen test into separate file

Move the splash screen test into separate `test_splash.py` file.
This way, when we add new splash screen tests, they will be all
in the same place.

Rename the test into `test_splash_screen_running`.

Patch to review:
diff --git a/tests/functional/test_interactive.py b/tests/functional/test_interactive.py
index 4b41513f741..fc0f10c5193 100644
--- a/tests/functional/test_interactive.py
+++ b/tests/functional/test_interactive.py
@@ -17,7 +17,7 @@
 import pytest
 
 from PyInstaller.utils.tests import importorskip
-from PyInstaller.compat import is_win, is_darwin, is_musl
+from PyInstaller.compat import is_win
 
 _RUNTIME = 10  # In seconds.
 
@@ -29,21 +29,3 @@ def test_ipython(pyi_builder):
         from IPython import embed
         embed()
         """, runtime=_RUNTIME)
-
-
-# Splash screen is not supported on macOS due to incompatible design.
-@pytest.mark.skipif(is_darwin, reason="Splash screen is not supported on macOS.")
-@pytest.mark.parametrize("build_mode", ['onedir', 'onefile'])
-@pytest.mark.parametrize("with_tkinter", [False, True], ids=['notkinter', 'tkinter'])
-@pytest.mark.xfail(is_musl, reason="musl + tkinter is known to cause mysterious segfaults.")
-def test_pyi_splash(pyi_builder_spec, capfd, monkeypatch, build_mode, with_tkinter):
-    if build_mode == 'onefile':
-        monkeypatch.setenv('_TEST_SPLASH_BUILD_MODE', 'onefile')
-    if with_tkinter:
-        monkeypatch.setenv('_TEST_SPLASH_WITH_TKINTER', '1')
-
-    pyi_builder_spec.test_spec('spec_with_splash.spec', runtime=_RUNTIME)
-
-    out, err = capfd.readouterr()
-    assert 'SPLASH: splash screen started' in err, \
-        f"Cannot find log entry indicating start of splash screen in:\n{err}"
diff --git a/tests/functional/test_splash.py b/tests/functional/test_splash.py
new file mode 100644
index 00000000000..473961924e2
--- /dev/null
+++ b/tests/functional/test_splash.py
@@ -0,0 +1,42 @@
+#-----------------------------------------------------------------------------
+# Copyright (c) 2005-2023, PyInstaller Development Team.
+#
+# Distributed under the terms of the GNU General Public License (version 2
+# or later) with exception for distributing the bootloader.
+#
+# The full license is in the file COPYING.txt, distributed with this software.
+#
+# SPDX-License-Identifier: (GPL-2.0-or-later WITH Bootloader-exception)
+#-----------------------------------------------------------------------------
+"""
+Tests for splash screen.
+
+NOTE: splash screen is not supported on macOS due to incompatible design.
+"""
+
+import pytest
+
+from PyInstaller.compat import is_darwin, is_musl
+
+
+# Test that splash screen is successfully started. This is an "interactive"
+# test (see test_interactive.py), where we expect the program to run for
+# specified amount of time, before we terminate it.
+@pytest.mark.skipif(is_darwin, reason="Splash screen is not supported on macOS.")
+@pytest.mark.parametrize("build_mode", ['onedir', 'onefile'])
+@pytest.mark.parametrize("with_tkinter", [False, True], ids=['notkinter', 'tkinter'])
+@pytest.mark.xfail(is_musl, reason="musl + tkinter is known to cause mysterious segfaults.")
+def test_splash_screen_running(pyi_builder_spec, capfd, monkeypatch, build_mode, with_tkinter):
+    if build_mode == 'onefile':
+        monkeypatch.setenv('_TEST_SPLASH_BUILD_MODE', 'onefile')
+    if with_tkinter:
+        monkeypatch.setenv('_TEST_SPLASH_WITH_TKINTER', '1')
+
+    pyi_builder_spec.test_spec(
+        'spec_with_splash.spec',
+        runtime=10,  # Interactive test - terminate test program after 10 seconds.
+    )
+
+    out, err = capfd.readouterr()
+    assert 'SPLASH: splash screen started' in err, \
+        f"Cannot find log entry indicating start of splash screen in:\n{err}"


Code review comments:
diff_hunk: @@ -40,3 +42,55 @@ def test_splash_screen_running(pyi_builder_spec, capfd, monkeypatch, build_mode,
     out, err = capfd.readouterr()
     assert 'SPLASH: splash screen started' in err, \
         f"Cannot find log entry indicating start of splash screen in:\n{err}"
+
+
+# Check that splash-enabled program properly exits (e.g., there are no crashes in bootloader during shutdown).
+# This is not coveved by the `test_splash_screen_running`, because that one terminates the program.
+# There are three variants of this test:
+# - splash screen is kept running until the end
+# - user imports pyi_splash, which calls pyi_splash.close() via atexit()
+# - user imports pyi_splash and calls pyi_splash.close()
+@pytest.mark.skipif(is_darwin, reason="Splash screen is not supported on macOS.")
+@pytest.mark.xfail(is_musl, reason="musl + tkinter is known to cause mysterious segfaults.")
path: tests/functional/test_splash.py
text: @user1:
Could we scope these to the whole file? i.e. Global variable `pytestmark = [pytest.mark.skipif(is_darwin, ...), pytest.xfail(is_musl, ...)]`