Skip to content

[WIP] installer: Update InnoSetup from 5.6.1 to 6.1.2#327

Closed
dennisameling wants to merge 2 commits into
git-for-windows:mainfrom
dennisameling:update-innosetup
Closed

[WIP] installer: Update InnoSetup from 5.6.1 to 6.1.2#327
dennisameling wants to merge 2 commits into
git-for-windows:mainfrom
dennisameling:update-innosetup

Conversation

@dennisameling
Copy link
Copy Markdown
Member

@dennisameling dennisameling commented Feb 6, 2021

#326 installs Git for Windows in the 32-bit Program Files folder (C:\Program Files (x86)), but InnoSetup 6 added support for installing in the 64-bit folder for Windows on ARM:

6.0: Support for 64-bit mode on Windows 10 on ARM.

While not a hard requirement, it would be nice to have future versions of Git for Windows on ARM64 install in the 64-bit app directory by default.

They also bumped the minimal Windows version to Vista, but that should be fine given the fact that Git for Windows also requires Vista or later.

Most important changes are listed at https://jrsoftware.org/isdl.php#stable, and the full changelog is listed at https://jrsoftware.org/files/is6-whatsnew.htm

Testing on Windows on ARM

After applying this PR and building the installer, Git will be installed to C:\Program Files\Git instead of C:\Program Files (x86)\Git on Windows on ARM:

image

One thing I noticed is that on Windows on ARM, both Git Credential Manager options are greyed out:

image

Comment thread installer/install.iss
WizardImageFile={#SourcePath}\git.bmp
WizardSmallImageFile={#SourcePath}\gitsmall.bmp
MinVersion=0,5.01sp3
MinVersion=6.0
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bumped the MinVersion to 6.0 (Vista)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess that means we could drop our own check wether we're running on vista or later.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It needs to stay there, otherwise InnoSetup will default to 6.1sp1:

Change in default behavior: Starting with Inno Setup 6.1 the [Setup] section directive MinVersion defaults to 6.1sp1, so by default Setup will not run on Windows Vista or on versions of Windows 7 and Windows Server 2008 R2 which have not been updated. Setting MinVersion to 6.0 to allow Setup to run on Windows Vista is supported but not recommended: Windows Vista doesn't support some of Setup's security measures against potential DLL preloading attacks so these have to be removed by the compiler if MinVersion is below 6.1 making your installer less secure on all versions of Windows.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, i meant since InnoSetups requirement and this MinVersion=6.0 check are in place, we don't need this check anymore:

GetWindowsVersionEx(Version);
if (Version.Major<6) then begin
if SuppressibleMsgBox('Git for Windows requires Windows Vista or later.'+#13+'Click "Yes" for more details.',mbError,MB_YESNO,IDNO)=IDYES then
ShellExec('open','https://gitforwindows.org/requirements.html','','',SW_SHOW,ewNoWait,ErrorCode);
Result:=False;
Exit;
end;

Comment thread installer/install.iss
Form.Caption:='Git Uninstall: Removing in-use files';
Form.ClientWidth:=ScaleX(500);
Form.ClientHeight:=ScaleY(256);
Form.Center;
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had to remove this due to the following error:

Launching Inno Setup compiler ...
Error on line 3369 in C:\git-sdk-32\usr\src\build-extra\installer\install.iss: Column 14:
Unknown identifier 'CENTER'

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heh, I did the same in dscho/build-extra@674f320...


# Download the most recent Inno Setup version.
installer="is-unicode.exe"
installer="is.exe"
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no longer an is-unicode.exe file. Details:

Change in default behavior: Starting with Inno Setup 6 there's only one version available: Unicode Inno Setup. Unicode Inno Setup has been available for 9 years but in case you have not yet updated to it: please see the Unicode Inno Setup topic in the help file for more information. Basically, unless you're using [Code] to make DLL calls with string parameters you shouldn't have to make any changes to your script.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did the same in dscho/build-extra@commit 114305f, and I also added //CURRENTUSER, no idea why, I forgot the details (probably because it's been over a year that I dabbled with the InnoSetup upgrade).

Signed-off-by: Dennis Ameling <dennis@dennisameling.com>
Signed-off-by: Dennis Ameling <dennis@dennisameling.com>
@dennisameling dennisameling changed the title installer: Update InnoSetup from 5.6.1 to 6.1.2 [WIP] installer: Update InnoSetup from 5.6.1 to 6.1.2 Feb 6, 2021
@rimrul
Copy link
Copy Markdown
Member

rimrul commented Feb 6, 2021

One thing I noticed is that on Windows on ARM, both Git Credential Manager options are greyed out:

Seems like we can't detect .NET Framework 4.5.1 or newer

function DetectNetFxVersion:Cardinal;
begin
// We are only interested in version v4.5.1 or later, therefore it
// is enough to only use the 4.5 method described in
// https://msdn.microsoft.com/en-us/library/hh925568
if not RegQueryDWordValue(HKEY_LOCAL_MACHINE,'SOFTWARE\Microsoft\NET Framework Setup\NDP\v4\Full','Release',Result) then
Result:=0;
end;

// Restore the settings chosen during a previous install, if .NET 4.5.1
// or later is available.
if DetectNetFxVersion()<378675 then begin
RdbGitCredentialManager[GCM_Classic].Checked:=False;
RdbGitCredentialManager[GCM_Classic].Enabled:=False;
RdbGitCredentialManager[GCM_Core].Checked:=False;
RdbGitCredentialManager[GCM_Core].Enabled:=False;
end else begin

@dennisameling
Copy link
Copy Markdown
Member Author

dennisameling commented Feb 6, 2021

@rimrul thanks for that info! I just checked and this is the reason it fails:

x64 machine
This machine has two registry keys, one for 32-bit and one for the 64-bit .NET framework:

32-bit: HKEY_LOCAL_MACHINE\SOFTWARE\WOW6432Node\Microsoft\NET Framework Setup\NDP\v4\Full
64-bit: HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\NET Framework Setup\NDP\v4\Full

ARM64 machine
This machine only has one registry key, because .NET Framework 4 isn't available natively for ARM64 (only .NET Core 5+ is) and 64-bit (x64) emulation is only available in prerelease versions of Windows 10:

32-bit: HKEY_LOCAL_MACHINE\SOFTWARE\WOW6432Node\Microsoft\NET Framework Setup\NDP\v4\Full
64-bit: MISSING

InnoSetup will attempt to access the 64-bit registry in 64-bit mode, so it makes sense that the value isn't found. .NET Framework is installed on ARM64 though, only the 32-bit version.

To keep things clean, it might be an option to separate this PR into two:

  1. Update InnoSetup to 6.1.2, not changing any other functionality
  2. Add 64-bit support for Windows ARM64

Otherwise it might become a bit challenging to review this PR. What do you think?

@dscho
Copy link
Copy Markdown
Member

dscho commented Feb 26, 2021

To keep things clean, it might be an option to separate this PR into two:

1. Update InnoSetup to 6.1.2, not changing any other functionality

2. Add 64-bit support for Windows ARM64

Otherwise it might become a bit challenging to review this PR.

I think that it's a fine plan to split this work up into two separate PRs.

@dennisameling
Copy link
Copy Markdown
Member Author

dennisameling commented Mar 2, 2021

Closing this PR as it's superseded by #327. Decided to just close this PR rather than force-push commits here, so that we'll keep the comment history here 👍🏼

@dennisameling dennisameling deleted the update-innosetup branch March 21, 2021 22:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants