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

Upgrade to net6.0 on Windows #154

Merged
merged 11 commits into from
Oct 21, 2022
Merged

Upgrade to net6.0 on Windows #154

merged 11 commits into from
Oct 21, 2022

Conversation

kyle-rader
Copy link
Contributor

@kyle-rader kyle-rader commented Sep 29, 2022

Problem

net5.0 has now passed end of life. Our Mac and Linux builds are already on net6.0 but the Windows build has remained on net5.0-windows10.xxx to maintain current WAM support.

Problem 2

MSAL.NET has not yet added a net6-windows target and it will check this target at runtime and fail if you aren't using that version.

Workaround

We can target a new windows specific TFM (Target Framework Moniker) to build our Windows version of AzureAuth with net6 but manually use use the net5.0-windows10.xxx version of the MSAL.NET dll (Microsoft.Identity.Client.dll).

This works by updating our PackageReference to generate the special PkgPACKAGE_NAME varaibles (which we use heavily in Office) and then create a separate reference tag for MSAL where we point explicitly to the DLL in the package we want to use for that namespace.

Trimming and Testing

To help fully test I added publish scripts for both mac and windows (these work on both arm and intel macs + windows).

This is helpful because they build in release mode, publish, and trim - just like our CI. So the output in ./dist is the actual binary, (unsigned) we would be shipping. This helped uncover new trimming errors, and lead to adding 2 AssemblyRoots to ensure those Dlls are not trimmed away. This is only needed for the Windows build.

Testing

  • Windows: Build and Test
    • Clear & Auth
      • IWA
      • Broker
      • Web
    • Auth silently with AT refresh (Web mode)
    • Both on and Off VPN (IWA fails off VPN as expected)
  • Mac: Build and Test (Both ARM arm64 and Intel `x86_64)
    • Tests
    • refresh auth with web mode silently
    • clear
    • auth in web mode
    • auth in device code mode

@kyle-rader kyle-rader changed the title [Draft] Attempt using net6 with MSAL 4.4.7.1 [Draft] Attempt using net6 with MSAL 4.47.1 Sep 29, 2022
@kyle-rader kyle-rader marked this pull request as ready for review September 29, 2022 18:52
@kyle-rader kyle-rader requested a review from a team as a code owner September 29, 2022 18:52
@kyle-rader kyle-rader changed the title [Draft] Attempt using net6 with MSAL 4.47.1 Upgrade to net6.0-windows10.0.19041.0 with MSAL 4.47.1 Sep 29, 2022
@goagain
Copy link
Contributor

goagain commented Sep 29, 2022

Seems like changing csproj file doesn't trigger dotnet tests. Should we add it?

@kyle-rader kyle-rader marked this pull request as draft September 29, 2022 19:10
@kyle-rader
Copy link
Contributor Author

kyle-rader commented Sep 29, 2022

IWA and Broker still work, but Web mode is now broken. @bgavrilMS is there a way to explicitly do system web browser and not embedded web browser?

EDIT: This is due to Assembly trimming. I'll have to add some trimming hints.

@kyle-rader
Copy link
Contributor Author

Seems like changing csproj file doesn't trigger dotnet tests. Should we add it?

@goagain we do already have src/** in the trigger paths, it may not have run since I am in Draft mode?

@kyle-rader kyle-rader added the enhancement New feature or request label Sep 29, 2022
@kyle-rader kyle-rader self-assigned this Sep 29, 2022
@pmaytak
Copy link

pmaytak commented Sep 29, 2022

IWA and Broker still work, but Web mode is now broken. @bgavrilMS is there a way to explicitly do system web browser and not embedded web browser?

EDIT: This is due to Assembly trimming. I'll have to add some trimming hints.

@kyle-rader To use OS browser add WithUseEmbeddedWebView(false) to AcquireTokenInteractive call.

More info here and here.

reillysiemens
reillysiemens previously approved these changes Sep 30, 2022
@kyle-rader kyle-rader changed the title Upgrade to net6.0-windows10.0.19041.0 with MSAL 4.47.1 Upgrade to net6.0 and MSAL 4.47.1 Sep 30, 2022
bin/mac_publish.sh Outdated Show resolved Hide resolved
bin/win_publish.cmd Outdated Show resolved Hide resolved
@kyle-rader kyle-rader changed the title Upgrade to net6.0 and MSAL 4.47.1 Upgrade to net6.0 on Windows Oct 21, 2022
@kyle-rader kyle-rader marked this pull request as ready for review October 21, 2022 19:09
Copy link
Contributor

@goagain goagain left a comment

Choose a reason for hiding this comment

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

Tested all modes on Windows and Mac.

Copy link
Contributor

@Haard30 Haard30 left a comment

Choose a reason for hiding this comment

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

Tested all modes on Windows

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants