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.

Sign up for GitHub

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

Jump to bottom

Update SDK #48908

Merged
merged 8 commits into from
Jun 24, 2023
Merged

Update SDK #48908

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
2f76362
Update SDK
RussKie Jun 20, 2023
8b17dee
Fix trimming annotation error
SteveSandersonMS Jun 22, 2023
47f5966
Supress RID resolution warning
SteveSandersonMS Jun 22, 2023
3755745
Update SDK further to get fix https://github.com/dotnet/runtime/pull/…
SteveSandersonMS Jun 22, 2023
56d8310
Attempted further workaround
SteveSandersonMS Jun 22, 2023
db226bb
Revert attempted workaround that didn't work
SteveSandersonMS Jun 22, 2023
9db37bd
Disable RID check in templates with SQLite
captainsafia Jun 23, 2023
719fc18
Try different location for RID workaround
halter73 Jun 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 0 additions & 7 deletions eng/Workarounds.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,11 +42,4 @@
<ErrorOnDuplicatePublishOutputFiles>false</ErrorOnDuplicatePublishOutputFiles>
</PropertyGroup>

<!-- Ignore warning about RID resolution rules changing in .NET 8 Preview 6 -->
<!-- Needed until https://github.com/ericsink/SQLitePCL.raw/issues/543 is fixed -->
<!-- See https://github.com/dotnet/aspnetcore/pull/48908#issuecomment-1601894643 -->
<ItemGroup>
<RuntimeHostConfigurationOption Include="System.Runtime.Loader.UseRidGraph" Value="true" />
</ItemGroup>

</Project>
7 changes: 7 additions & 0 deletions eng/tools/GenerateFiles/Directory.Build.props.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,11 @@
<!-- Temporarily hardcoded to true -->
<SuppressGenerateILCompilerExplicitPackageReferenceWarning>true</SuppressGenerateILCompilerExplicitPackageReferenceWarning>
</PropertyGroup>

<!-- Ignore warning about RID resolution rules changing in .NET 8 Preview 6 -->
<!-- Needed until https://github.com/ericsink/SQLitePCL.raw/issues/543 is fixed -->
<!-- See https://github.com/dotnet/aspnetcore/pull/48908#issuecomment-1601894643 -->
<ItemGroup>
Copy link
Member

Choose a reason for hiding this comment

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

How does this work for templates?

Copy link
Member

Choose a reason for hiding this comment

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

I thought it might work for the templates because of these lines:

aspnetcore/src/ProjectTemplates/TestInfrastructure/PrepareForTest.targets

Lines 85 to 88 in 9f4b8b4

<GenerateFileFromTemplate
TemplateFile="$(MSBuildThisFileDirectory)..\TestInfrastructure\Directory.Build.props.in"
Properties="RepoRoot=$(RepoRoot);ArtifactsBinDir=$(ArtifactsBinDir)"
OutputPath="$(TestTemplateCreationFolder)Directory.Build.props" />

aspnetcore/src/ProjectTemplates/TestInfrastructure/Directory.Build.props.in

Line 8 in 9f4b8b4

<Import Project="${ArtifactsBinDir}GenerateFiles\Directory.Build.props" />

But I was just trying thing, and I'm a bit surprised it worked too. @wtgodbe Is there a better way to do this? I saw that @mitchdenny updated this a couple months ago as part of #48014. Do we still need to hardcode SuppressGenerateILCompilerExplicitPackageReferenceWarning?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, this is the right way to do it, at least for in-repo stuff. Template tests will get the workaround, but template projects may run into issues in real world scenarios (since the workaround won't be injected into them). I don't know enough about this specific issue to know if it's something that would actually manifest.

@eerhardt do we still need the SuppressGenerateILCompilerExplicitPackageReferenceWarning workaround?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, this is the right way to do it, at least for in-repo stuff. Template tests will get the workaround, but template projects may run into issues in real world scenarios (since the workaround won't be injected into them). I don't know enough about this specific issue to know if it's something that would actually manifest.

Ah, that makes sense. Hopefully, this is resolved for preview7 and we don't have to issue a known issues warning. 🤞🏽

Copy link
Member

Choose a reason for hiding this comment

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

@eerhardt do we still need the SuppressGenerateILCompilerExplicitPackageReferenceWarning workaround?

I believe so. I don't know of any changes here that would cause us to no longer need the workaround. We are running our tests against a specific version of the runtime, and not the runtime that comes with the SDK. See dotnet/runtime#84372.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, that makes sense. Hopefully, this is resolved for preview7 and we don't have to issue a known issues warning. 🤞🏽

I doubt this will be fixed by preview7. Even if ericsink/SQLitePCL.raw#543 is resolved by then and uploaded to nuget, EF still needs to update its dependencies. Anyone using Microsoft.Data.Sqlite will likely see warnings until rc1 at the earliest unless they manually upgrade, so we should probably get ahead of the issues. @dotnet/efteam

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if it would be reasonable to add a mechanism to tell the SDK that it should not warn on some particular package or particular versions of packages - for example, the consumer has checked that they are not affected by the non-portable RID in that package, as would be the case here. @dsplaisted @vitek-karas @agocke - thoughts?

It would go kind of counter to making a hard push for packages to fix their RID usage, but it would provide an option between suppressing the warning entirely and switching to the old behaviour - neither of which we really want people to do.

Copy link
Member

Choose a reason for hiding this comment

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

Is there a way to suppress the warning by its warning code - I don't know if NoWarn works on SDK warnings. If that works, then I'd probably not add any new support and ask the affected people to suppress the warning code instead.

Basically we want packages to fix this problem, and there are not that many packages which are affected by this and consequently not that many teams/customers will see the warning either. I'd like to avoid adding production code into the SDK unless there's a strong reason for it - and this doesn't feel that strong - assuming the warning can be suppressed already.

Copy link
Member

Choose a reason for hiding this comment

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

<NoWarn>NETSDK1206</NoWarn> works.

Copy link
Member

Choose a reason for hiding this comment

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

Basically we want packages to fix this problem, and there are not that many packages which are affected by this and consequently not that many teams/customers will see the warning either.

That's fair. The main concern is that the affected package is brought in as a dependency under certain template configurations to there is a chance that users might run into the issue with this particular package at a higher rate than other packages.

<RuntimeHostConfigurationOption Include="System.Runtime.Loader.UseRidGraph" Value="true" />
</ItemGroup>
</Project>
5 changes: 0 additions & 5 deletions src/ProjectTemplates/Web.ProjectTemplates/RazorPagesWeb-CSharp.csproj.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,5 @@
<PackageReference Include="Microsoft.Identity.Web.DownstreamApi" Version="${MicrosoftIdentityWebDownstreamApiVersion}" Condition=" '$(IndividualB2CAuth)' == 'True' OR '$(OrganizationalAuth)' == 'True'" />
</ItemGroup>

<!-- Temporarily ignore RID-related warnings. -->
<ItemGroup>
<RuntimeHostConfigurationOption Include="System.Runtime.Loader.UseRidGraph" Value="true" />
</ItemGroup>

<!--#endif -->
</Project>
5 changes: 0 additions & 5 deletions src/ProjectTemplates/Web.ProjectTemplates/StarterWeb-CSharp.csproj.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,5 @@
<PackageReference Include="Microsoft.Identity.Web.DownstreamApi" Version="${MicrosoftIdentityWebDownstreamApiVersion}" Condition=" '$(IndividualB2CAuth)' == 'True' OR '$(OrganizationalAuth)' == 'True'" />
</ItemGroup>

<!-- Temporarily ignore RID-related warnings. -->
<ItemGroup>
<RuntimeHostConfigurationOption Include="System.Runtime.Loader.UseRidGraph" Value="true" />
</ItemGroup>

<!--#endif -->
</Project>