Clean up STJ references Fixes #39902 #40645

Forgind · 2024-05-02T20:26:44Z

First commit removes references to S.T.J that shouldn't be necessary because we have our own "WorkloadManifestReader.SystemTextJson" class.

The second commit forces all our package versions to 8.0.0.

I was a bit confused with this logic:
<UseSystemTextJson Condition="'$(TargetFramework)'!='netstandard2.0' And '$(TargetFramework)'!='net472'">True</UseSystemTextJson>

This seems to say that we should use System.Text.Json on core only

kasperk81 · 2024-05-03T16:56:06Z

src/Resolvers/Microsoft.DotNet.MSBuildSdkResolver/Microsoft.DotNet.MSBuildSdkResolver.csproj

@@ -91,7 +91,7 @@
  </ItemGroup>

  <ItemGroup Condition="'$(TargetFrameworkIdentifier)' == '.NETFramework'">
-    <PackageReference Include="System.Text.Json"  VersionOverride="8.0.0" />
+    <PackageReference Include="System.Text.Json"  VersionOverride="$(SystemTextJsonPackageVersion)" Condition="'$(TargetFramework)' == 'net472'"/>


duplicate condition (previous line covered it)

I'm just confused why things aren't working the way I'd expect, so I was adding some duplication in case I had missed something, and it didn't work as I'd thought.

marcpopMSFT · 2024-05-08T19:09:04Z

eng/Versions.props

@@ -78,7 +78,7 @@
    <SystemSecurityCryptographyXmlPackageVersion>8.0.0</SystemSecurityCryptographyXmlPackageVersion>
    <SystemSecurityPermissionsPackageVersion>8.0.0</SystemSecurityPermissionsPackageVersion>
    <SystemServiceProcessServiceControllerVersion>8.0.0</SystemServiceProcessServiceControllerVersion>
-    <SystemTextJsonPackageVersion>8.0.2</SystemTextJsonPackageVersion>
+    <SystemTextJsonPackageVersion>8.0.0</SystemTextJsonPackageVersion>


This property is used in directory.packages.props so what you're doing here is actually overriding the version used by transitive pinning (so overriding the version for all implicit .net core references). You'll need a separate property to use for pinning to 8.0.0 and let the d.p.props version float to current. Runtime does this too if you see here: https://github.com/dotnet/runtime/blob/main/eng/Versions.props#L137

FYI, I talked to Eric and I'll be prepping a change for 8.0.1xx and 8.0.3xx for how we handle STJ. We'll want that change in 8.0.4xx as well. It depends on a change in runtime so you won't be able to get this PR working in 4xx as it turns out. You should be able to retarget to main, make the above change, apply my changes, and get it sorted out. I'll talk to you offline as it's complicated as there are actually 3 different classes of STJ references, not 2 like I thought.

Following up on this, I believe there are three categories of project that we talked about:

The four task projects should have STJ references with no version override. Razor and static web assets should have a net472 check but build and publish should not. I recommend looking at the 8.0.3xx internal AzDo branches to confirm:
https://dev.azure.com/dnceng/internal/_git/dotnet-sdk?path=/src/WebSdk/Publish/Tasks/Microsoft.NET.Sdk.Publish.Tasks.csproj&version=GBinternal/release/8.0.3xx&_a=contents

The components we have that build for framework and run in VS should have a reference to STJ for netfx targeting and should have a VersionOverride to version 8.0.0. That's because in these cases, VS is providing STJ and we need to work with whichever version they ship.

No other .net core building projects should have a reference to STJ.

CC @ericstj to confirm. We discussed this in a 1:1 a few weeks ago but didn't write it down so writing it down here now.

This sounds correct. Minimize the places where you ship a copy if you can reference the version provided by your host process.

marcpopMSFT · 2024-07-01T20:02:24Z

src/BuiltInTools/dotnet-watch/dotnet-watch.csproj

-      Explicit System.Text.Json package reference is required for source-build to pick up the live package.
-      Avoids picking up an old version via transitive dependency from Microsoft.Build or Microsoft.CodeAnalysis.Workspaces.MSBuild.
-    -->
-    <PackageReference Include="System.Text.Json" />


Probably want a review from @tmat but with CPM now enabled, this may not be required anymore like it was before.

Having an explicit package reference is no longer required, or removing it is no longer required?

Having the explicit reference is no longer required, I think. If I understand the original issue per the comment, a transitive version was picked up that didn't work with source build and adding the explicit reference up-versioned to one that was supported. CPM should do the version change as well.

marcpopMSFT · 2024-07-01T20:03:56Z

src/Tests/ArgumentsReflector/ArgumentsReflector.csproj

@@ -23,6 +23,7 @@
    <Using Remove="Microsoft.NET.TestFramework.Utilities" />
    <Using Remove="Xunit" />
    <Using Remove="Xunit.Abstractions" />
+    <PackageReference Include="System.Text.Json" VersionOverride="$(SystemTextJsonPackageVersion)" />


this is a test so not sure it'll need the version override

marcpopMSFT · 2024-07-01T20:07:50Z

...ers/Microsoft.NET.Sdk.WorkloadManifestReader/Microsoft.NET.Sdk.WorkloadManifestReader.csproj

@@ -25,7 +25,9 @@
  </ItemGroup>

  <ItemGroup>
-    <PackageReference Include="System.Text.Json" Condition="'$(UseSystemTextJson)'=='True'" />
+    <PackageReference Include="System.Text.Json"


This one will require more investigation. The conditional above that sets UseSystemTextJson is configured for core only which we don't need a reference for so I think we might be able to just remove it from here but you'd have to confirm the framework build doesn't use STJ (as I know things changed so this property may not be accurately named anymore)

marcpopMSFT · 2024-07-01T20:09:50Z

src/Cli/Microsoft.DotNet.Cli.Utils/Microsoft.DotNet.Cli.Utils.csproj

@@ -47,6 +47,7 @@
    <PackageReference Include="Microsoft.Build" ExcludeAssets="runtime" PrivateAssets="all" />
    <PackageReference Include="Microsoft.Build.Utilities.Core" ExcludeAssets="runtime" PrivateAssets="all" />
    <PackageReference Include="System.CommandLine" />
+    <PackageReference Include="System.Text.Json" VersionOverride="$(SystemTextJsonPackageVersion)" Condition="'$(TargetFramework)' == 'net472'"/>


where is SystemTextJsonPackageVersion set and what is it set to? I think it should be 8.0.0 but don't see it in this PR (also the name is really close to what darc will update so it may need a slight update to ensure darc doesn't get confused).

I'm starting to think the PR to create the specific package versions was never ported to main. I'll need to investigate.

My PR is now in which should enable you to update the minimum version name #41890

marcpopMSFT · 2024-07-11T21:46:06Z

src/Tests/ArgumentsReflector/ArgumentsReflector.csproj

@@ -23,6 +23,7 @@
    <Using Remove="Microsoft.NET.TestFramework.Utilities" />
    <Using Remove="Xunit" />
    <Using Remove="Xunit.Abstractions" />
+    <PackageReference Include="System.Text.Json" />


Do you know why this was needed?

marcpopMSFT · 2024-07-11T21:46:52Z

src/Cli/Microsoft.DotNet.Cli.Utils/Microsoft.DotNet.Cli.Utils.csproj

@@ -47,6 +47,7 @@
    <PackageReference Include="Microsoft.Build" ExcludeAssets="runtime" PrivateAssets="all" />
    <PackageReference Include="Microsoft.Build.Utilities.Core" ExcludeAssets="runtime" PrivateAssets="all" />
    <PackageReference Include="System.CommandLine" />
+    <PackageReference Include="System.Text.Json" VersionOverride="$(SystemTextJsonBasePackageVersion)" Condition="'$(TargetFramework)' == 'net472'"/>


why was this needed?

kasperk81 · 2024-07-18T23:14:58Z

...ers/Microsoft.NET.Sdk.WorkloadManifestReader/Microsoft.NET.Sdk.WorkloadManifestReader.csproj

    <!-- Netfx version builds against the lowest version of System.Text.Json that's guaranteed to be shipped with MSBuild in VS -->
    <PackageReference Include="Microsoft.Bcl.AsyncInterfaces" VersionOverride="8.0.0" />


not needed?

I was confused in looking at the 8.0.4xx-targeted version, as this wasn't needed then...but that's because it was using Newtonsoft.Json instead. Apparently it was fully switched to STJ, so now it became a problem. Thanks for catching that 🙂

kasperk81 · 2024-07-18T23:15:21Z

src/StaticWebAssetsSdk/Tasks/Microsoft.NET.Sdk.StaticWebAssets.Tasks.csproj

  <ItemGroup Condition="'$(TargetFrameworkIdentifier)' == '.NETFramework'">
    <PackageReference Include="System.Reflection.Metadata" VersionOverride="$(SystemReflectionMetadataVersion)" />
-    <PackageReference Include="System.Text.Json" />
+    <PackageReference Include="System.Text.Json"  Condition="'$(TargetFramework)'=='net472'" />


ViktorHofer · 2024-07-19T09:30:39Z

src/Microsoft.DotNet.TemplateLocator/Microsoft.DotNet.TemplateLocator.csproj

@@ -25,7 +25,7 @@
      <BuildOutputInPackage Include="@(ReferenceCopyLocalPaths-&gt;WithMetadataValue('ReferenceSourceTarget', 'ProjectReference'))" />
    </ItemGroup>
  </Target>
-
+  


Suggested change

ViktorHofer · 2024-07-19T09:31:34Z

eng/Versions.props

@@ -157,6 +157,7 @@
    <SystemSecurityPermissionsPackageVersion>9.0.0-preview.7.24366.18</SystemSecurityPermissionsPackageVersion>
    <SystemTextEncodingCodePagesPackageVersion>9.0.0-preview.7.24366.18</SystemTextEncodingCodePagesPackageVersion>
    <SystemTextJsonPackageVersion>9.0.0-preview.7.24366.18</SystemTextJsonPackageVersion>
+    <SystemTextJsonBasePackageVersion>8.0.0</SystemTextJsonBasePackageVersion>


What does "Base" stand for here? You might want to add a comment.

ViktorHofer · 2024-07-22T11:39:50Z

Consider using Toolset instead of Base: https://github.com/dotnet/runtime/blob/7c3286fd62a617a26439d751e3c2ce80fd6599f5/eng/Versions.props#L135-L139

eng/Versions.props

rainersigwald · 2024-07-22T14:54:49Z

eng/Versions.props

+    <!-- This is a minimum version for various projects to work. It's used for netfx-targeted components that run in Visual Studio
+         because in those cases, Visual Studio is providing System.Text.Json, and we should work with whichever version it ships. -->
+    <SystemTextJsonBasePackageVersion>8.0.4</SystemTextJsonBasePackageVersion>


Bumping the netfx reference to 8.0.4 will fail to satisfy this constraint for VS 17.11. That's bad, right?

8.0.3 should be fine, right? I would assume 8.0.3 is better than 8.0.0.

dotnet-issue-labeler bot added Area-Infrastructure untriaged Request triage from a team member labels May 2, 2024

Forgind changed the title ~~Clean up STJ references~~ Clean up STJ references Fixes #39902 May 2, 2024

kasperk81 reviewed May 3, 2024

View reviewed changes

marcpopMSFT reviewed May 8, 2024

View reviewed changes

build-analysis bot mentioned this pull request Jun 28, 2024

Darwin Build_Release failure on Helix tests - Tests timing out #40074

Open

marcpopMSFT reviewed Jul 1, 2024

View reviewed changes

Forgind marked this pull request as ready for review July 11, 2024 21:45

Forgind requested review from a team and vijayrkn as code owners July 11, 2024 21:45

marcpopMSFT reviewed Jul 11, 2024

View reviewed changes

marcpopMSFT approved these changes Jul 18, 2024

View reviewed changes

Reimplement STJ change

9763f0b

Forgind force-pushed the stj-version branch from 5fa0ce6 to 9763f0b Compare July 18, 2024 22:28

Forgind requested review from a team, joperezr, tmat, arkalyanms, AntonLapounov and sharwell as code owners July 18, 2024 22:28

kasperk81 reviewed Jul 18, 2024

View reviewed changes

build-analysis bot mentioned this pull request Jul 19, 2024

msbuild crashes with "MSB0001: Internal MSBuild Error: must be valid" dotnet/dnceng#3304

Open

3 tasks

ViktorHofer reviewed Jul 19, 2024

View reviewed changes

Forgind removed request for a team, radical, lewing, tmat, pavelsavara, sharwell, vijayrkn, maraf, joperezr, AntonLapounov and arkalyanms July 19, 2024 14:33

PR feedback

9fe4889

build-analysis bot mentioned this pull request Jul 20, 2024

Roslyn analyzer throws error AD0001 NullReferenceException dotnet/dnceng#3305

Open

3 tasks

Merge branch 'main' into stj-version

3cf8751

Forgind commented Jul 22, 2024

View reviewed changes

eng/Versions.props Outdated Show resolved Hide resolved

rainersigwald reviewed Jul 22, 2024

View reviewed changes

Forgind force-pushed the stj-version branch from d571396 to 3cf8751 Compare July 23, 2024 18:47

Forgind added 2 commits July 23, 2024 14:47

Merge branch 'main' of https://github.com/dotnet/sdk into stj-version

2d84bf5

change base to toolset

bf21493

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up STJ references Fixes #39902 #40645

Clean up STJ references Fixes #39902 #40645

Forgind commented May 2, 2024 •

edited

Loading

kasperk81 May 3, 2024

Forgind May 7, 2024

marcpopMSFT May 8, 2024

marcpopMSFT Jun 27, 2024

ericstj Jun 27, 2024

marcpopMSFT Jul 1, 2024

Forgind Jul 1, 2024

marcpopMSFT Jul 3, 2024

marcpopMSFT Jul 1, 2024

marcpopMSFT Jul 1, 2024

marcpopMSFT Jul 1, 2024

marcpopMSFT Jul 1, 2024

marcpopMSFT Jul 8, 2024

marcpopMSFT Jul 11, 2024

marcpopMSFT Jul 11, 2024

kasperk81 Jul 18, 2024

Forgind Jul 19, 2024

kasperk81 Jul 18, 2024

ViktorHofer Jul 19, 2024

ViktorHofer Jul 19, 2024

ViktorHofer commented Jul 22, 2024

rainersigwald Jul 22, 2024

ViktorHofer Jul 22, 2024 •

edited

Loading

		<!-- Netfx version builds against the lowest version of System.Text.Json that's guaranteed to be shipped with MSBuild in VS -->
		<PackageReference Include="Microsoft.Bcl.AsyncInterfaces" VersionOverride="8.0.0" />

Clean up STJ references Fixes #39902 #40645

Are you sure you want to change the base?

Clean up STJ references Fixes #39902 #40645

Conversation

Forgind commented May 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ViktorHofer commented Jul 22, 2024

Choose a reason for hiding this comment

ViktorHofer Jul 22, 2024 • edited Loading

Choose a reason for hiding this comment

Forgind commented May 2, 2024 •

edited

Loading

ViktorHofer Jul 22, 2024 •

edited

Loading