-
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
Conversation
news/1132-fix-cli-args
Outdated
|
|
||
| ### Bug fixes | ||
|
|
||
| * EXE: Fixed an issue for silent installers where command-line argument `/KeepPkgCache` was ignored and `/NoRegistry` would reset the default value. |
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.
| * 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.
| version = line.split(":")[-1].strip() | ||
| if name and version: | ||
| break | ||
| partial_name = f"{name} {version}" |
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.
This seems brittle. Instead of manually parsing the construct.yaml file, I suggest using the UninstallString value and see if it starts with $INSTDIR\Uninstall-:
constructor/constructor/nsis/main.nsi.tmpl
Lines 1627 to 1628 in 7a837bd
| WriteRegStr SHCTX "${UNINSTREG}" "UninstallString" \ | |
| "$\"$INSTDIR\Uninstall-${NAME}.exe$\"" |
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.
Hmm, what about using the variable installer we get from create_installer? If I'm not mistaken that should be the path to the exe and it is named as <name>-<version>-<build number>-<platform>-<arch>.exe. If we split installer.name on - we have name from [0] and version from [1], let me know what you think.
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.
I don't think the installer uses the build number (it's part of the version string), but that could work, too.
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.
Reduced it to
installer_file_name_parts = Path(installer).name.split("-")
name = installer_file_name_parts[0]
version = installer_file_name_parts[1]
partial_name = f"{name} {version}"
Co-authored-by: Marco Esters <[email protected]>
Description
This PR introduces an issue detected with NSIS
GetOptionsresetting the value ofARGV_NoRegistryas well as properly accounted for the CLI argKeepPkgCache, looks like this was ignored before.Checklist - did you ...
newsdirectory (using the template) for the next release's release notes?