-
Notifications
You must be signed in to change notification settings - Fork 176
Fix issue with GetOptions resetting variable and account for KeepPkgCache #1132
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 8 commits
1f51052
633b202
d4ff66f
05451c0
b11d0f3
a2a15c0
0d890a8
0950d36
19e254d
658d53a
287c6db
907ee44
36cd379
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,19 @@ | ||||||
| ### Enhancements | ||||||
|
|
||||||
| * <news item> | ||||||
|
|
||||||
| ### Bug fixes | ||||||
|
|
||||||
| * EXE: Fixed an issue for silent installers where command-line argument `/KeepPkgCache` was ignored and `/NoRegistry` would reset the default value. | ||||||
|
||||||
| * EXE: Fixed an issue for silent installers where command-line argument `/KeepPkgCache` was ignored and `/NoRegistry` would reset the default value. | |
| * EXE: Fixed an issue for silent installers where command-line argument `/KeepPkgCache` was ignored and `/NoRegistry` would reset the default value. (#1132) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -34,6 +34,8 @@ | |||||
| if sys.platform == "darwin": | ||||||
| from constructor.osxpkg import calculate_install_dir | ||||||
| elif sys.platform.startswith("win"): | ||||||
| import winreg | ||||||
|
|
||||||
| import ntsecuritycon as con | ||||||
| import win32security | ||||||
|
|
||||||
|
|
@@ -60,6 +62,49 @@ | |||||
| KEEP_ARTIFACTS_PATH = None | ||||||
|
|
||||||
|
|
||||||
| def _is_program_installed(partial_name: str) -> bool: | ||||||
| """ | ||||||
| Checks if a program is listed in the Windows 'Installed apps' menu. | ||||||
| We search by looking for a partial name to avoid having to account for Python version and arch. | ||||||
| Returns True if a match is found, otherwise False. | ||||||
| """ | ||||||
|
|
||||||
| if not sys.platform.startswith("win"): | ||||||
| return False | ||||||
|
|
||||||
| # For its current purpose HKEY_CURRENT_USER is sufficient, | ||||||
| # but additional registry locations could be added later. | ||||||
| UNINSTALL_PATHS = [ | ||||||
| (winreg.HKEY_CURRENT_USER, r"SOFTWARE\Microsoft\Windows\CurrentVersion\Uninstall"), | ||||||
| ] | ||||||
|
|
||||||
| partial_name = partial_name.lower() | ||||||
|
|
||||||
| for hive, path in UNINSTALL_PATHS: | ||||||
| try: | ||||||
| reg_key = winreg.OpenKey(hive, path) | ||||||
| except FileNotFoundError: | ||||||
| continue | ||||||
|
|
||||||
| subkey_count = winreg.QueryInfoKey(reg_key)[0] | ||||||
|
|
||||||
| for i in range(subkey_count): | ||||||
| try: | ||||||
| subkey_name = winreg.EnumKey(reg_key, i) | ||||||
| subkey = winreg.OpenKey(reg_key, subkey_name) | ||||||
|
|
||||||
| display_name, _ = winreg.QueryValueEx(subkey, "DisplayName") | ||||||
|
|
||||||
| if partial_name in display_name.lower(): | ||||||
| return True | ||||||
|
|
||||||
| except (FileNotFoundError, OSError, TypeError): | ||||||
| # Some keys may lack DisplayName or have unexpected value types | ||||||
| continue | ||||||
|
|
||||||
| return False | ||||||
|
|
||||||
|
|
||||||
| def _execute( | ||||||
| cmd: Iterable[str], installer_input=None, check=True, timeout=420, **env_vars | ||||||
| ) -> subprocess.CompletedProcess: | ||||||
|
|
@@ -1038,8 +1083,6 @@ def test_initialization(tmp_path, request, monkeypatch, method): | |||||
| ) | ||||||
| if installer.suffix == ".exe": | ||||||
| try: | ||||||
| import winreg | ||||||
|
|
||||||
| paths = [] | ||||||
| for root, keyname in ( | ||||||
| (winreg.HKEY_CURRENT_USER, r"Environment"), | ||||||
|
|
@@ -1423,3 +1466,45 @@ def test_regressions(tmp_path, request): | |||||
| check_subprocess=True, | ||||||
| uninstall=True, | ||||||
| ) | ||||||
|
|
||||||
|
|
||||||
| @pytest.mark.parametrize("no_registry", (0, 1)) | ||||||
| @pytest.mark.skipif(not ON_CI, reason="CI only") | ||||||
| @pytest.mark.skipif(not sys.platform.startswith("win"), reason="Windows only") | ||||||
| def test_not_in_installed_menu_list_(tmp_path, request, no_registry): | ||||||
| """Verify the app is Installed Apps Menu (or not), based the CLI arg '/NoRegistry'. | ||||||
lrandersson marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||
| If NoRegistry=0, we expect to find the installer in the Menu, otherwise not. | ||||||
| """ | ||||||
| input_path = _example_path("extra_files") # 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): | ||||||
| _run_installer( | ||||||
| input_path, | ||||||
| installer, | ||||||
| install_dir, | ||||||
| request=request, | ||||||
| check_subprocess=True, | ||||||
| uninstall=False, | ||||||
| options=options, | ||||||
| ) | ||||||
|
|
||||||
| # Extract name and version from yaml-file | ||||||
| # in order to predict parts of the installer name | ||||||
| with open(input_path / "construct.yaml") as f: | ||||||
| contents = f.readlines() | ||||||
| name = "" | ||||||
| version = "" | ||||||
| for line in contents: | ||||||
| if "name: " in line: | ||||||
| name = line.split(":")[-1].strip() | ||||||
| if "version: " in line: | ||||||
| version = line.split(":")[-1].strip() | ||||||
| if name and version: | ||||||
| break | ||||||
| partial_name = f"{name} {version}" | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems brittle. Instead of manually parsing the constructor/constructor/nsis/main.nsi.tmpl Lines 1627 to 1628 in 7a837bd
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, what about using the variable
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think the installer uses the build number (it's part of the version string), but that could work, too.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reduced it to |
||||||
|
|
||||||
| is_in_installed_apps_menu = _is_program_installed(partial_name) | ||||||
| _run_uninstaller_exe(install_dir) | ||||||
| assert is_in_installed_apps_menu == (no_registry == 0), ( | ||||||
| f"Unable to find program {partial_name} in the 'Installed apps' menu" | ||||||
| ) | ||||||
Uh oh!
There was an error while loading. Please reload this page.