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

Use current STJ in HostModel and remove unnecessary audit suppressions #109852

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ViktorHofer
Copy link
Member

@ViktorHofer ViktorHofer commented Nov 15, 2024

Fixes #108262 and follow-up work from 31e28ad
Blocked on dotnet/sdk#44042

@ViktorHofer ViktorHofer added blocked Issue/PR is blocked on something - see comments area-Infrastructure-installer and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Nov 15, 2024
@ericstj
Copy link
Member

ericstj commented Nov 15, 2024

I support the goal of this PR, I tried to do it before, but had to roll it back because of #108261

Be sure that you test those scenarios so that we don't break again. IIRC we didn't see the crossgen problems on PR it was only afterward.

<PackageReference Include="System.Text.Json" Version="$(SystemTextJsonToolsetVersion)" ExcludeAssets="Runtime" PrivateAssets="All" />
</ItemGroup>

<!-- Suppress System.Text.Json/8.0.4 advisory as desktop msbuild doesn't yet provide binding redirects for the non-vulnerable version (8.0.5). -->
Copy link
Member

Choose a reason for hiding this comment

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

Is this no longer the case for the VS versions we are using?

Copy link
Member Author

Choose a reason for hiding this comment

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

Still the case but I decided to apply the NU1903 NoWarn directly on the STJ PackageReference above so that this won't pop up again with a future vulnerability report for STJ.

<!-- Suppress System.Text.Json/8.0.4 advisory as desktop msbuild doesn't yet provide binding redirects for the non-vulnerable version (8.0.5). -->
<ItemGroup>
<NuGetAuditSuppress Include="https://github.com/advisories/GHSA-8g4q-xg66-9fp4" />
<ProjectReference Include="$(LibrariesProjectRoot)System.Text.Json\src\System.Text.Json.csproj" SetConfiguration="Configuration=$(LibrariesConfiguration)" />
Copy link
Member

Choose a reason for hiding this comment

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

We do the SetConfiguration here:

<ProjectReference Include="@(LibrariesProjectReference)" SetConfiguration="Configuration=$(LibrariesConfiguration)"/>

@ViktorHofer
Copy link
Member Author

ViktorHofer commented Nov 15, 2024

I support the goal of this PR, I tried to do it before, but had to roll it back because of #108261

I saw the exact same error when testing with vstest locally. I posted a comment in dotnet/sdk#44042 as that's the underlying reason why this doesn't work. When testing with xunit.console.dll, things work fine.

Be sure that you test those scenarios so that we don't break again. IIRC we didn't see the crossgen problems on PR it was only afterward.

Can you please elaborate on the crossgen problems?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-Infrastructure-installer blocked Issue/PR is blocked on something - see comments
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HostModel does not reference the version of System.Text.Json that it runs on in the SDK
2 participants