kaikramer / keystore-explorer

KeyStore Explorer is a free GUI replacement for the Java command-line utilities keytool and jarsigner.
https://keystore-explorer.org/
GNU General Public License v3.0
1.7k stars 275 forks source link

Windows installer recommendations #250

Closed Bill-Stewart closed 2 years ago

Bill-Stewart commented 3 years ago

I have some recommendations for the Windows installer:

  1. Use HKLM\SOFTWARE\Classes (or HKCU\Software\Classes if installing only for current user) instead of HKCR - see https://docs.microsoft.com/en-us/windows/win32/sysinfo/hkey-classes-root-key
  2. Add kse.exe to Software\Microsoft\Windows\CurrentVersion\App Paths subkey - see https://docs.microsoft.com/en-us/windows/win32/shell/app-registration#using-the-app-paths-subkey
  3. Register kse.exe as an application - see https://docs.microsoft.com/en-us/windows/win32/shell/app-registration#using-the-applications-subkey
  4. Create a ProgId for selected file types - see https://docs.microsoft.com/en-us/windows/win32/shell/fa-progids
  5. Add values to OpenWithProgIds subkey for selected file types - see https://docs.microsoft.com/en-us/windows/win32/shell/fa-file-types#setting-optional-subkeys-and-file-type-extension-attributes

In regards to HKLM vs HKCU, I would recommend taking a look at using Inno Setup as it provides a very simple abstraction and installer paradigm for installing per-computer vs. per-user (basically, HKA means "HKLM if installing per-computer, or HKCU if installing per-user").

kaikramer commented 3 years ago

Thanks for the recommendations. There is still a lot to do for the next KSE release and I don't want to delay it even more. Do these recommendations fix any issues or are they just best practices?

Bill-Stewart commented 3 years ago

KeyStoreExplorer_Setup_Windows.zip The main issues I see is are:

  1. The current installer writes to HKCR which (according to Microsoft, see link in item 1 above) should be treated as read-only. This can be problematic in cases where the installer runs in a different user context (e.g., elevation) because the default write location might be different than expected.

  2. The current installer can overwrite one or more existing associations for keystore file types when selected (and if you uninstall KSE the previous association is lost). The new guidelines (use OpenWithProgIds instead) prevents this and also (on Windows 10) adds KSE to the "Set defaults by app" dialog.

I already wrote an updated Inno Setup installer (attached) that handles all of the above, supports per-user vs. per-computer installs, etc. The updated installer uses a single checkbox to register the app as a file handler for keystore file types and provides a simple mechanism for adding additional types as well as some other niceties such as localization. You are free to use it if you like.

kaikramer commented 3 years ago

I am currently considering to do the switch to Inno Setup, but I have to integrate it in the build process and the content of the ZIP file uses a completely different approach, which makes it difficult for me to adapt them. Maybe you can guide me here a bit...

With NSIS there was a single script file template that contained a few place holders that were replaced by Gradle with the proper values (and then NSIS was executed with the resulting script):

task nsis(dependsOn: [prepareExe, copyDependencies]) {
    doLast {
        def nsisScript = "kse.nsi"
        mkdir distDir
        copy {
            from("nsis/${nsisScript}.template")
            rename("${nsisScript}.template", nsisScript)
            filter(ReplaceTokens, beginToken: '%', endToken: '%', tokens: [
                KSE_NAME: appName,
                KSE_VERSION: appVersion,
                KSE_SIMPLE_VERSION: appSimpleVersion,
                KSE_APP_USER_MODEL_ID: appUserModelId,
                ICONS_DIR: iconsDir.toString(),
                LICENSES_DIR: licensesDir.toString(),
                JAR_DIR: libsDir.toString(),
                LIB_DIR: dependenciesDir.toString(),
                LAUNCH4J_DIR: launcherOutDir.toString(),
                DIST_DIR: distDir.toString()
            ])
            into("nsis")
        }
        exec {
            workingDir "$projectDir/nsis"
            commandLine "makensis", "/V2", nsisScript
        }
    }
}

Using a similar approach for Inno Setup seems possible by merging includes.iss and KeyStoreExplorer.iss into a single file and appinfo.ini would be obsoleted by the Gradle template mechanism then. Or are there any reasons to leave includes.iss and KeyStoreExplorer.iss as they are and create a template only for appinfo.ini?

Those three defines are never used in KeyStoreExplorer.iss:

#define AppMajorVersion ReadIni(AddBackslash(SourcePath) + "appinfo.ini", "Application", "Major", "0")
#define AppMinorVersion ReadIni(AddBackslash(SourcePath) + "appinfo.ini", "Application", "Minor", "0")
#define AppPatchVersion ReadIni(AddBackslash(SourcePath) + "appinfo.ini", "Application", "Patch", "0")

Are they required by Inno Setup or can I simply remove them?

How can I omit the license step? There have been complaints by users in the past about having to accept the license.

Bill-Stewart commented 3 years ago

Correct: The appinfo.ini file is merely a way for the Inno Setup preprocessor to generate an up-to-date installer script. Yes, you could incorporate includes.iss into KeyStoreExplorer.iss and then use your templating for appinfo.ini. In this way you have two files: appinfo.ini would get updated for each new release and KeyStoreExplorer.iss would stay the same (unless it has a bug or needs an update). To me this seems to be a "clean" approach.

The App...Version defines get used in the AppFullVersion define just below them. They are split into Major, Minor, and Patch to make reading easy/obvious from appinfo.ini.

If you want to omit the license step, then you should be able to just remove the LicenseFile parameter from the corresponding line in the [Languages] section:

[Languages]
Name: english; MessagesFile: "compiler:Default.isl,Messages-en.isl"; LicenseFile: "License-en.rtf"; InfoBeforeFile: "Readme-en.rtf"

That is, change to:

[Languages]
Name: english; MessagesFile: "compiler:Default.isl,Messages-en.isl"; InfoBeforeFile: "Readme-en.rtf"

In that case then you don't need the License-en.rtf file of course.

Bill-Stewart commented 3 years ago

Correction: It looks like the includes.iss file is separate because Messages-en.isl file also refers to includes.iss to retrieve the application name and version. (You probably noticed this already.)

kaikramer commented 3 years ago

I have pushed the changes for InnoSetup into master. I had to add AppUserModelID: ...; to the exe icon in order to make KSE pinnable to the Windows taskbar and a link to the license folder in the start menu. The old installer also created an "uninstall" item in the start menu, but I don't think that's very useful or necessary. There might be other differences that I have missed so far.

I haven't checked on the improvements you did regarding the recommendations mentioned in this ticket. I just trust you on that.

I am unsure about two minor things:

  1. It did seem weird to me that the installer uses the KSE application icon and I have commented it out.
  2. Is the readme actually useful in any way? I mean one would assume that the user already knows what KSE is before installing it. And the links to the home page and GitHub project are probably also known because that's where the user has downloaded the installer (and both links are in the help menu of KSE as well). grafik

What are your thoughts on these two points?

Bill-Stewart commented 3 years ago

LGTM. The installer icon can be whatever you want, and of course feel free to remove the readme if you feel it's superfluous.

kaikramer commented 2 years ago

Closing tickets in preparation for release of KSE 5.5.0