Skip to content
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

Migrate from .NET Framework on Windows #113

Open
mjcheetham opened this issue May 12, 2020 · 13 comments · May be fixed by #1419
Open

Migrate from .NET Framework on Windows #113

mjcheetham opened this issue May 12, 2020 · 13 comments · May be fixed by #1419
Assignees
Labels
auth:microsoft Specific to Microsoft AAD/MSA authentication enhancement New feature or request platform:windows Specific to the Windows platform
Milestone

Comments

@mjcheetham
Copy link
Collaborator

We are currently not actually using the .NET Core on Windows. We should actually use the .NET Core runtime!

The reason we are currently using .NET Framework instead is because MSAL does (didn't) support embedded browser on Windows when targeting .NET Core or .NET Standard. We can use the UseCustomWebUi extension point to restore this functionality.

@mjcheetham mjcheetham added enhancement New feature or request platform:windows Specific to the Windows platform auth:microsoft Specific to Microsoft AAD/MSA authentication labels May 12, 2020
@Regenhardt
Copy link

So your readme straight up lies saying "...built on .NET Core"? Could be "...built on .NET" or "...built on .NET Framework and soon .NET Core" instead.

@rimrul
Copy link

rimrul commented Jul 30, 2020

No. The macOS build is built on .NET Core. So the readme is merely inacurate.

@dennisameling
Copy link

dennisameling commented Feb 6, 2021

@mjcheetham would you accept a PR that migrates the Windows bits to .NET Core? Moving to .NET Core should also unlock building natively for Windows ARM64. We're working on a semi-native ARM64 Git for Windows and while using the x86 GCMC works for now (through emulation), having a native binary would be fantastic.

Is there a specific reason why you want to use an embedded browser rather than the user's default browser? This is the official recommendation:

We recommend that you use the platform default, and this is typically the system browser. The system browser is better at remembering the users that have logged in before. If you need to change this behavior, use WithUseEmbeddedWebView(bool)

Note to self: this might be a viable approach for an embedded Web UI

@mjcheetham
Copy link
Collaborator Author

Hi @dennisameling,

Is there a specific reason why you want to use an embedded browser rather than the user's default browser?

There are/were a few reasons:

  1. To maintain the existing behaviour from Git Credential Manager for Window (GCMW) which used an embedded browser pop-up via the old ADAL auth library
  2. Without an intermediate window (take a look at GitHub and Bitbucket's UI flows with OAuth) where we can tell the user we're about to start a new browser window, just launching a browser window for some random authentication isn't a great user experience.

[..] would you accept a PR that migrates the Windows bits to .NET Core?

We're not wedded to the .NET Framework as the target runtime on Windows but need to consider "moving the user's cheese" for the UX. 🧀 ⚠️

We'd also need to be mindful of the impact of shipping a copy the .NET Core runtime with GCM Core has on installer/zip file size, and how bundlers of GCMC would respond. Git for Windows's libexec\git-core directory would now contain a helluva lot of binaries for example!

We can look at using the single-file publishing model of course to reduce the number of binaries shipped, and trimming to reduce the size. .NET 5 would likely be the preferred target runtime to do this trimming and single-file publishing with, due to several improvements made in the latest release.

Note to self: this might be a viable approach for an embedded Web UI

As I mentioned in this issue "We can use the UseCustomWebUi extension point to restore this functionality." - it's just a case of having enough time to get around to do that 😉

Ideally MSAL.NET would expose the WinForms-based embedded browser when running on .NET Core on Windows, but that seems to be combined with/is waiting on their replacement of the old browser with "WebView2" (which itself has runtime distribution considerations).


So to answer your question, "yes" we would accept a PR to move to .NET Core on Windows, but we'd also need to ensure the following at the same time:

  • Single-file distribution model (& .NET 5?)
  • Assembly trimming (& .NET 5?)
  • Embedded browser support for Microsoft authentication, OR..
  • ..some GUI that indicates we're about to launch a browser for the user.

(Note: on macOS we've recently removed the embedded browser workaround/native helper to rely on the default browser flow, but on Windows we have to be more considerate of the experience change that would bring with the various consumers of GCM which includes Sourcetree and Visual Studio).

@vtbassmatt
Copy link
Contributor

I'm in favor of this change - being on two different and diverging runtimes isn't hurting us now but smells like the kind of tech debt which will hurt us later. Let's sequence this after a move to .NET 5, and we can spend the time between now and then figuring out how much 🧀 -moving we'll do to VS and Sourcetree.

@vtbassmatt
Copy link
Contributor

As nice as this would be, the tradeoffs still don't really make sense. The bloat of bringing along the runtime is unacceptable to our distribution partners in Git for Windows, and the Win7 story would get needlessly complicated.

We should reevaluate this when: 1) we can drop Win7 and 2) some .NET flavor can get us down to 3x our size (as opposed to the 10x it is in .NET 5).

@ldennington ldennington reopened this Feb 6, 2023
@ldennington ldennington added this to the Future milestone Feb 6, 2023
@mjcheetham mjcheetham changed the title Use .NET Core on Windows Migrate from .NET Framework on Windows Feb 7, 2023
@mjcheetham
Copy link
Collaborator Author

Update on dropping .NET Framework on Windows.. work in MSAL, .NET 6 and 7 means that we may be at a point where we can consider trying this again.

  • MSAL supports the embedded browser window on Windows when targeting net6.0-windows10.0.17763.0. We'd need to dual-target still, but at least net6.0-windowsX is a superset of net6.0 so we'd not run in to issues like today where language features or BCL APIs are missing in .NET Framework.

  • Improvements in app trimming in .NET 7 (and AOT in the upcoming .NET 8) means we should be able to keep our standalone installation small enough to be acceptable to Git for Windows.

There is still one aspect that means we may not be able to fully embrace single-file publishing and that's Windows 7 support. Msys2 (that Git for Windows depends on) has signaled that it will be dropping Windows 7 and 8.0 support: https://github.com/msys2/msys2.github.io/blob/source/web/news.md#2023-01-15---dropping-support-for-windows-7-and-80

@dscho
Copy link
Contributor

dscho commented Feb 8, 2023

Msys2 (that Git for Windows depends on) has signaled that it will be dropping Windows 7 and 8.0 support: https://github.com/msys2/msys2.github.io/blob/source/web/news.md#2023-01-15---dropping-support-for-windows-7-and-80

So has the Git for Windows project.

@mjcheetham mjcheetham modified the milestones: Future, Git 2.43 May 22, 2023
@mjcheetham
Copy link
Collaborator Author

Update on Windows 7 issues and single-file publishing.. the latest docs indicate that we can use single file publishing if we are using the .NET 6.0.3 runtime or later:

[IMPORTANT] To run a single file app on Windows 7, you must use .NET Runtime 6.0.3 or later.

https://learn.microsoft.com/en-us/dotnet/core/deploying/single-file/overview

@mjcheetham
Copy link
Collaborator Author

This work should now be entirely achievable with the latest .NET 8 release candidate, from my testing! 🎉

We can logically break this work down in to a few steps:

  1. Update Mac and Linux targets from .NET 7 to .NET 8 (in RC now, with GA due Nov 2023)

    • .NET 7 is not an LTS release, so we should be doing this anyway!
  2. Remove any .NET Framework-only code/projects

    • Drop WPF UI helpers; we've not been using them by default in Windows builds for a number of releases now.
  3. Retarget Windows builds to .NET 8 and remove conditionally compiled code

    • Drop all the #if NETFRAMEWORK code which is no longer required
  4. Prepare codebase for trimming

    • Enable source generation for System.Text.Json
    • Enable compiled binding for Avalonia UI components
  5. Enable trimming and single-file publishing

@rimrul
Copy link

rimrul commented Oct 6, 2023

Retarget Windows builds to .NET 8

That might be a slight issue. Cygwin has explicitly announced that they intend to keep supporting Windows 8.1 for the forseable future. AFAIK Msys2 and Git for Windows have similar plans. But .NET 7 dropped support for Windows 8.1.

@dscho
Copy link
Contributor

dscho commented Oct 17, 2023

Cygwin has explicitly announced that they intend to keep supporting Windows 8.1 for the forseable future.

Historically, we have been following their lead in Git for Windows. Because we had to. Like, when we still wanted to support Windows XP and Cygwin ended support for Windows XP, Git for Windows could not bear the torch to resurrect and maintain that support.

In this instance, I think it might actually be the other way around: Git for Windows could say that, you know, Windows 8.1 support ended on January 10, 2023. If you need to use Git on that Windows version, either build it yourself or use an older version of Git for Windows.

I'm not saying that this decision has been made, though. I'm just saying that it is an option, maybe the easiest option we have.

@rimrul
Copy link

rimrul commented Oct 17, 2023

We could also potentially disallow the Git Credential Manager option in the Git for Windows installer on Windows 8.1 and recommend a standalone install of the latest .NET Framework based version in that situation.

@mjcheetham mjcheetham self-assigned this Nov 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auth:microsoft Specific to Microsoft AAD/MSA authentication enhancement New feature or request platform:windows Specific to the Windows platform
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants