-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Fix netfx build for S.S.C.Xml nuget package #78665
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
Conversation
|
Tagging subscribers to this area: @dotnet/area-system-security, @vcsjones Issue DetailsFixes #78652. Manually verified that the net462/ref.dll and net462/lib.dll were missing the typefowards before, and they're present after.
|
adamsitnik
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change LGTM, but there is one more project file that has the same bug:
runtime/src/libraries/System.Threading.AccessControl/ref/System.Threading.AccessControl.csproj
Line 9 in dd0ec94
| <Compile Include="System.Threading.AccessControl.netframework.cs" Condition="'$(TargetFrameworkIdentifier)' == 'NETFramework'" /> |
@bartonjs would you mind fixing it in this PR as well?
|
@ViktorHofer is there a way to be notified via a build failure when there's a typo like this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing this, @bartonjs
|
APICompat doesn't catch this because the type forwards don't exist on netstandard2.0. Baseline validation that compares ref/net462 or lib/net462 from the previous package with the live built one could theoretically catch this. That said, we don't have such a feature in ApiCompat today. I'm a bit surprised that our .NET Framework tests didn't catch this. It looks like the tests don't bind against the package asset and instead directly on the .NET Framework runtime library. That alone is a terrible indicator that we might not be testing our actually .NET Framework shipping assets and that would raise the question why we then even run those tests. |
For cryptoxml, to ensure that any changes we do take are still compatible with netfx, because it's effectively a serialization library and needs that level of fidelity. But, yes, it would be better if the tests (or APICompat/etc) could guard us from this, too. |
|
/backport to release/7.0 |
|
Started backporting to release/7.0: https://github.com/dotnet/runtime/actions/runs/3527528268 |
Fixes #78652.
Manually verified that the net462/ref.dll and net462/lib.dll were missing the typefowards before, and they're present after.