[Bug] Microsoft.Identity.Client.Broker has package/targets authoring error for .NET Framework #4108
Closed
1 task done
Comments
mjcheetham
changed the title
ldennington
added a commit
to git-ecosystem/git-credential-manager
that referenced
this issue
May 2, 2023
Remove Nerdbank.GitVersioning dependency from project in favor of a new VERSION file, which will be updated ahead of each release. Versioning in this file will begin with the version we plan to use for the next release: 2.1.0.0. This change involves the addition of a new custom MSBuild task, which reads in the contents of the VERSION file, converts it to a Version object, and then sets the various version-related MSBuild properties with the correct value (some with the `Revision` component appended, others without). Note that there is a bug in MSAL [1] that causes build failures for projects without dependencies with this change. We add Newtonsoft.Json as a global dependency in Directory.Build.props to work around this problem until the fix is released. [1]: AzureAD/microsoft-authentication-library-for-dotnet#4108
ldennington
added a commit
to git-ecosystem/git-credential-manager
that referenced
this issue
May 2, 2023
With the [plan to migrate GCM to a formula for release via `Homebrew/homebrew-core`](#1102), it became clear that [Nerdbank.GitVersioning](https://github.com/dotnet/Nerdbank.GitVersioning) was no longer an option for versioning the project. This is because `Nerdbank.GitVersioning` requires history to calculate things like ['Git height'](https://github.com/dotnet/Nerdbank.GitVersioning#what-is-git-height), and the formula requires a tarball to build, which, of course, has no history. This change pivots GCM to the simpler strategy of using a version specified in a `VERSION` file at root. This version will be updated by maintainers prior to release - giving them more granular control of versioning, which in turn allows for versioning to be more predictable (i.e. maintainers will know what the version will be before publication of the release). The version specified in the file is the one slated for the next release: `2.1.0.0`. **Note:** This change [fails on Windows](https://github.com/ldennington/git-credential-manager/actions/runs/4824686907) due to a [bug in MSAL](AzureAD/microsoft-authentication-library-for-dotnet#4108) unless we ensure all projects have at least 1 dependency. We are working around this issue by adding Newtonsoft.Json as this dependency (since we were already shipping it anyway).
ldennington
added a commit
to ldennington/git-credential-manager
that referenced
this issue
May 31, 2023
Update the current Json.NET dependency to System.Text.Json. Note that this is required only for the following reasons: - .NET Framework needs the package [1] - We need a global dependency to work around [2] until the fix [3] is released. 1: https://learn.microsoft.com/en-us/dotnet/standard/serialization/system-text-json/overview#how-to-get-the-library 2: AzureAD/microsoft-authentication-library-for-dotnet#4108 3: AzureAD/microsoft-authentication-library-for-cpp#3522
ldennington
added a commit
to git-ecosystem/git-credential-manager
that referenced
this issue
Jun 6, 2023
# Overview Migrate GCM from JSON.NET to `System.Text.Json`. The first commit updates to .NET 7.0 to take advantage of certain features like `JsonRequired`. The second updates `Microsoft.Identity.Client` so that the workaround for [this issue](AzureAD/microsoft-authentication-library-for-dotnet#4108 ) is no longer necessary. The third updates the current global Json.NET dependency to `System.Text.Json` and ensures it is only used for .NET Framework. The fourth adds a custom converter to handle Bitbucket's use of the non-standard 'scopes' property to provide token endpoint results. The fifth through ninth commits update `Trace2Message`, the Bitbucket REST API, the GitHub REST API, the Azure DevOps API tests, and OAuth-specific code to use `System.Text.Json` instead of Json.NET for JSON serialization/deserialization. # Performance comparison Comparison runs were done using [`dotnet-trace`](https://learn.microsoft.com/en-us/dotnet/core/diagnostics/dotnet-trace) with the command `printf "protocol=https\nhost=github.com" | git-credential-manager get`. The machine used was a Mac with a 2.4 GHz 8-Core Intel Core i9 processor running Ventura 13.4. Times are in milliseconds. ## Individual Run Times | **Newtonsoft.json** | **System.Text.Json** | |---------------------|----------------------| | 364.08 | 304.92 | | 366.11 | 305.23 | | 377.31 | 313.09 | | 381.3 | 313.51 | | 373.25 | 297.71 | | 373.98 | 312.78 | | 370.84 | 317.86 | | 368.25 | 311.78 | | 369.79 | 307.69 | ## Run Summary | | **Newtonsoft.json** | **System.Text.Json** | |-------------|---------------------|----------------------| | **Average** | 371.66 | 309.40 | | **Median** | 370.84 | 311.78 | ## Analysis Per the above, the transition to `System.Text.Json` and .NET 7.0 decreased end-to-end execution times by about 17% on average.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Which version of MSAL.NET are you using?
4.52.0
Platform
.NET 4.7.2
What authentication flow has the issue?
Other?
Is this a new or existing app?
The app is in production, and I have upgraded to a new version of MSAL.
Repro
See minimal example here: https://github.com/mjcheetham/msal-broker-package-error
Expected behavior
dotnet buildis successful.Actual behavior
Build error:
The
Microsoft.Identity.Client.NativeInterop.targetsfile has an authoring error:The condition
'@(PackageReference)' == ''is trivially true on any projects that have zero package references (whetherPackageReferenceor not), and also reference, transitively,MS.ID.Client.Broker.Possible solution
Replace the erroneous
Conditionwith one that also permits projects without any dependencies.Additional context / logs / screenshots / links to code
See the minimal repro here: https://github.com/mjcheetham/msal-broker-package-error
cc @bgavrilMS @ldennington
The text was updated successfully, but these errors were encountered: