installer: bump NetFx version requirement from 4.5.1 -> 4.6.1 #329

y2kbugger · 2021-02-24T19:56:29Z

We already require Fx4.6.1, but the installer checks for Fx4.5.1

Signed-off-by: Zak Kohler git@y2kbugger.com

I used the lower of the two 4.6.1 checks as that is how 4.5.1 was done previously.

.NET Framework 4.6.1
On Windows 10 November Update systems: 394254
On all other Windows operating systems (including Windows 10): 394271

dscho · 2021-02-24T20:26:50Z

.NET Framework 4.6.1
On Windows 10 November Update systems: 394254
On all other Windows operating systems (including Windows 10): 394271

Where does that information come from?

Also, could you amend the commit message to start with "installer: bump NetFx version requirement from 4.5.1 -> 4.6.1", and then continue by specifying what requires v4.6.1, accompanied by a link to the evidence (e.g. to the code in GCM Core or GCM for Windows that shows how it won't work with older versions)?

dscho · 2021-02-24T20:28:12Z

Oh, sorry, I forgot to thank you for this valuable contribution!

I'm not the domain expert on this, so I'll leave it to @mjcheetham to give their 👍 before merging.

y2kbugger · 2021-02-24T21:21:44Z

Version detection spec comes from here:
https://docs.microsoft.com/en-us/dotnet/framework/migration-guide/how-to-determine-which-versions-are-installed

To be fair, @mjcheetham noticed the discrepancy, mentioned in an email when asking about another issue:

One interesting thing to note.. the Git for Windows installer is checking for .NET Framework 4.5.1, but actually it should be checking for 4.6.1 (which is what GCM Core requires on Windows) – this is a bug

dscho · 2021-02-24T21:47:42Z

Version detection spec comes from here:
https://docs.microsoft.com/en-us/dotnet/framework/migration-guide/how-to-determine-which-versions-are-installed

To be fair, @mjcheetham noticed the discrepancy, mentioned in an email when asking about another issue:

One interesting thing to note.. the Git for Windows installer is checking for .NET Framework 4.5.1, but actually it should be checking for 4.6.1 (which is what GCM Core requires on Windows) – this is a bug

Excellent! Could you add that information (together with a link to @mjcheetham's comment) into the commit message? Then we're golden.

installer/install.iss

y2kbugger · 2021-02-26T05:22:36Z

Since this change isn't trivial like the first, I want to build and test that it works as expected. I'm looking into options for this.

dscho · 2021-02-26T08:18:02Z

installer/install.iss

-            RdbGitCredentialManager[GCM_Core].Checked:=True;
-        end;
-        // Auto-upgrade GCM to GCM Core in version v2.29.0
-        if RdbGitCredentialManager[GCM_Classic].Checked and ((PreviousGitForWindowsVersion='') or IsUpgrade('2.29.0')) then begin


Why not keep this block here, but enhance the conditionals so that e.g. the auto-upgrade won't happen if RdbGitCredentialManager[GCM_Core].Enabled evaluated to True?

My reason is that denesting the conditionals reduces complexity/ improves maintainability. The tradeoff is that the upgrade/replay execute regardless, and are the repealed by the version checks. but I think that is better than having a combinatorially nested ifs, as that would involve duplicating at least some of the checks.

If you look at structure of final code (rather than just diff) it is:

Replay previous

Upgrade to Core

Version Guard for GCM Core

Version Guard for GCM Classic

Did notice one thing that might need changed (based on testing): if both guards get triggered (and unchecked and disabled), do we need to manually set [GCM_None].Checked:=True, or do radios enforce that once option is chosen.

My rationale: if you put the version guard before replying, you can automatically reject an invalid replay.

On that note, @y2kbugger, have you tested what happens if GCM_Core is checked during a replay, but then disabled because of the version check (using a bogus minimum version to force it to be disabled would be the easiest way to test that, I think)?

mjcheetham · 2021-03-02T10:12:01Z

I'm not the domain expert on this, so I'll leave it to @mjcheetham to give their 👍 before merging.

This is correct. GCM Core requires .NET Framework 4.6.1 be installed to run. GCM "classic" only requires 4.5.1.

Heads-up.. a future version (the next one?) of GCM Core will require .NET Framework 4.7.2.

Going even further in to the future, GCM Core will not require any version of .NET Framework (we will bundle/statically link the .NET Core CLR and libraries).

y2kbugger · 2021-03-09T14:58:53Z

My reason is that denesting the conditionals reduces complexity/ improves maintainability. The tradeoff is that the upgrade/replay execute regardless, and are the repealed by the version checks. but I think that is better than having a combinatorially nested ifs, as that would involve duplicating at least some of the checks.

If you look at structure of final code (rather than just diff) it is:

Replay previous

Upgrade to Core

Version Guard for GCM Core

Version Guard for GCM Classic

Did notice one thing that might need changed (based on testing): if both guards get triggered (and unchecked and disabled), do we need to manually set [GCM_None].Checked:=True, or do radios enforce that once option is chosen.

I think i made the required changes and unless my reasoning(or code) is either wrong or the other style is just preferred this is ready to be tested.

dscho · 2021-03-10T21:28:38Z

a future version (the next one?) of GCM Core will require .NET Framework 4.7.2.

@mjcheetham this happened with the latest release, correct?

mjcheetham · 2021-03-17T13:47:19Z

a future version (the next one?) of GCM Core will require .NET Framework 4.7.2.

@mjcheetham this happened with the latest release, correct?

Yes. The current latest GCM Core release on Windows targets .NET Framework 4.7.2.
https://github.com/microsoft/Git-Credential-Manager-Core/blob/3fc6791abf670dd08c0620e8a6c642f7104c128a/src/windows/Payload.Windows/Payload.Windows.csproj#L6

dscho · 2021-12-06T10:03:46Z

I guess now that we no longer ship the old Git Credential Manager for Windows, this PR would become substantially simpler?

dscho · 2022-04-29T13:15:00Z

I guess now that we no longer ship the old Git Credential Manager for Windows, this PR would become substantially simpler?

@mjcheetham I guess that was a question for you?

mjcheetham · 2022-04-29T14:37:22Z

I guess now that we no longer ship the old Git Credential Manager for Windows, this PR would become substantially simpler?

@mjcheetham I guess that was a question for you?

GCM "Core" still requires .NET Framework 4.7.2.

dscho · 2022-05-04T21:12:12Z

I guess now that we no longer ship the old Git Credential Manager for Windows, this PR would become substantially simpler?

@mjcheetham I guess that was a question for you?

GCM "Core" still requires .NET Framework 4.7.2.

Oh, so we have to change

build-extra/installer/install.iss

Lines 2290 to 2292 in 19c1773

    
               // Restore the settings chosen during a previous install, if .NET 4.5.1 
        
               // or later is available. 
        
               if DetectNetFxVersion()<378675 then begin

, but to what value?

rimrul · 2022-05-05T04:54:53Z

Oh, so we have to change

build-extra/installer/install.iss

Lines 2290 to 2292 in 19c1773

// Restore the settings chosen during a previous install, if .NET 4.5.1

// or later is available.

if DetectNetFxVersion()<378675 then begin

, but to what value?

Probably 461808. See https://docs.microsoft.com/en-us/dotnet/framework/migration-guide/how-to-determine-which-versions-are-installed#detect-net-framework-45-and-later-versions

When users cannot choose to use Git Credential Manager, they're frequently puzzled why it is disabled. Let'd give them a hint. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

We forgot to bump the requirement when Git Credential Manager started requiring a later version than 4.5.1. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

I updated the branch, the patches now look very different and need a new review

dscho · 2022-05-05T09:32:47Z

@rimrul could I bother you for a re-review?

mjcheetham

Technically looks good (correct version check etc), but I'd change the UI to specify "Framework".

installer/install.iss

Typically, when referring to `.NET`, one apparently refers to the Core CLR, not to the Desktop CLR we mean here. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

rimrul · 2022-05-05T14:57:36Z

On a side-note: The CI seems to struggle a lot with installing the build-installers flavor of the SDK. I think it might be related to the fact that when I try to download the SDK artifact manually, it reliably slows to a crawl after around 20MB. This doesn't seem to happen with the full-sdk artifact, though.

mjcheetham

LGTM!

dscho · 2022-05-05T17:36:02Z

The CI seems to struggle a lot with installing the build-installers flavor of the SDK

Right, this is actually tracked in git-for-windows/setup-git-for-windows-sdk#336... I just didn't have time yet to look into it more closely.

I think it might be related to the fact that when I try to download the SDK artifact manually, it reliably slows to a crawl after around 20MB.

Potentially.

I think we should look into using partial clones, and parallel checkout, anyway. We could even just initialize an empty repository, get the commit SHA as well as the contents of .sparse/ via the API, and then fetch the objects in a batch. It's a bit tricky to get this right so that the performance does not tank, but I think this might be better than going forward with these git-sdk-64-extra-artifacts job artifacts.

The installer [now verifies that .NET Framework 4.7.2 is available](#329) before offering Git Credential Manager (GCM) as an option (because it is required for GCM to work). Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

dscho changed the title ~~Fix Fx version requirement from 4.5.1 -> 4.6.1~~ installer: bump NetFx version requirement from 4.5.1 -> 4.6.1 Feb 24, 2021

y2kbugger force-pushed the patch-1 branch from 6dc7df6 to faafdfd Compare February 24, 2021 21:23

y2kbugger force-pushed the patch-1 branch from faafdfd to 8fa54c0 Compare February 24, 2021 22:43

rimrul previously requested changes Feb 25, 2021

View reviewed changes

installer/install.iss Outdated Show resolved Hide resolved

installer/install.iss Outdated Show resolved Hide resolved

y2kbugger force-pushed the patch-1 branch from f9aac39 to bb6fc07 Compare February 26, 2021 05:18

y2kbugger requested a review from rimrul February 26, 2021 05:19

dscho reviewed Feb 26, 2021

View reviewed changes

dscho added 2 commits May 5, 2022 11:22

installer: when the GCM option is disabled, explain why

4dff45b

When users cannot choose to use Git Credential Manager, they're frequently puzzled why it is disabled. Let'd give them a hint. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

installer: GCM requires .NET 4.7.2 or later

f407190

We forgot to bump the requirement when Git Credential Manager started requiring a later version than 4.5.1. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

dscho force-pushed the patch-1 branch from bb6fc07 to f407190 Compare May 5, 2022 09:29

dscho requested a review from mjcheetham May 5, 2022 09:31

mjcheetham requested changes May 5, 2022

View reviewed changes

installer/install.iss Outdated Show resolved Hide resolved

rimrul reviewed May 5, 2022

View reviewed changes

installer/install.iss Outdated Show resolved Hide resolved

installer: clarify that we mean .NET _Framework_ 4.7.2

5db650c

Typically, when referring to `.NET`, one apparently refers to the Core CLR, not to the Desktop CLR we mean here. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

mjcheetham approved these changes May 5, 2022

View reviewed changes

dscho merged commit 0dff628 into git-for-windows:main May 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

installer: bump NetFx version requirement from 4.5.1 -> 4.6.1 #329

installer: bump NetFx version requirement from 4.5.1 -> 4.6.1 #329

y2kbugger commented Feb 24, 2021

dscho commented Feb 24, 2021

dscho commented Feb 24, 2021

y2kbugger commented Feb 24, 2021

dscho commented Feb 24, 2021

y2kbugger commented Feb 26, 2021

dscho Feb 26, 2021

y2kbugger Mar 1, 2021 •

edited

Loading

dscho Mar 10, 2021

dscho Mar 17, 2021

mjcheetham commented Mar 2, 2021

y2kbugger commented Mar 9, 2021

dscho commented Mar 10, 2021

mjcheetham commented Mar 17, 2021

dscho commented Dec 6, 2021

dscho commented Apr 29, 2022

mjcheetham commented Apr 29, 2022

dscho commented May 4, 2022

rimrul commented May 5, 2022

dscho commented May 5, 2022

mjcheetham left a comment

rimrul commented May 5, 2022

mjcheetham left a comment

dscho commented May 5, 2022

installer: bump NetFx version requirement from 4.5.1 -> 4.6.1 #329

installer: bump NetFx version requirement from 4.5.1 -> 4.6.1 #329

Conversation

y2kbugger commented Feb 24, 2021

dscho commented Feb 24, 2021

dscho commented Feb 24, 2021

y2kbugger commented Feb 24, 2021

dscho commented Feb 24, 2021

y2kbugger commented Feb 26, 2021

dscho Feb 26, 2021

Choose a reason for hiding this comment

y2kbugger Mar 1, 2021 • edited Loading

Choose a reason for hiding this comment

dscho Mar 10, 2021

Choose a reason for hiding this comment

dscho Mar 17, 2021

Choose a reason for hiding this comment

mjcheetham commented Mar 2, 2021

y2kbugger commented Mar 9, 2021

dscho commented Mar 10, 2021

mjcheetham commented Mar 17, 2021

dscho commented Dec 6, 2021

dscho commented Apr 29, 2022

mjcheetham commented Apr 29, 2022

dscho commented May 4, 2022

rimrul commented May 5, 2022

dscho commented May 5, 2022

mjcheetham left a comment

Choose a reason for hiding this comment

rimrul commented May 5, 2022

mjcheetham left a comment

Choose a reason for hiding this comment

dscho commented May 5, 2022

y2kbugger Mar 1, 2021 •

edited

Loading