-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[main] Update dependencies from dotnet/runtime #18292
[main] Update dependencies from dotnet/runtime #18292
Conversation
…0616.2 Microsoft.NETCore.Platforms , Microsoft.NETCore.App.Runtime.win-x64 , Microsoft.NETCore.DotNetHostResolver , Microsoft.NET.HostModel , Microsoft.Extensions.DependencyModel , Microsoft.NETCore.App.Host.win-x64 , Microsoft.NETCore.App.Ref , System.Reflection.MetadataLoadContext , System.Resources.Extensions , System.Security.Cryptography.ProtectedData , System.Text.Encoding.CodePages , System.CodeDom , VS.Redist.Common.NetCore.SharedFramework.x64.6.0 From Version 6.0.0-preview.6.21314.2 -> To Version 6.0.0-preview.7.21316.2
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
Notification for subscribed users from https://github.com/dotnet/runtime:@dnr-codeflow Action requested: Please take a look at this failing automated dependency-flow pull request's checks; failures may be related to changes which originated in your repo.
|
@dsplaisted @sfoslund who is domestic cat this week? (could we set up there is response needed in CLI:
|
@jeffhandley is there someone who can help the CLI team resolve these analyzer violations? to unblock codeflow. |
Hmm, is this a new check. The code's been around, but it looks like we probably need to add SupportOsPlatform attributes to this class |
@jeffhandley I would expect |
We recently updated the CA1416 analyzer with dotnet/roslyn-analyzers#5117 to improve scenarios related to platform-neutral assemblies. I can't think of anything in there that could have caused this though. It's also possible that some of the recent changes around netstandard vs. netcoreapp could have surfaced something new here. dotnet/runtime#53023 Looking... |
Yes, that change should not reveal any warning for these PR changes. T
The warnings look valid and any recent change should cause this. No idea why they showed up now.
Seems it worked for the APIs you added but needs SupportedOSPlatform("windows") in more places, probably add an assembly-level attribute for windows targeted build EDIT: I see they are failed in Ubuntu build too, so assembly-level attribute for windows targeted build seems would not help, probably need to add an annotation for each APIs referencing windows only APIs (or add platform check if they are reachable in cross-platform build) |
I'm starting to suspect this change as what triggered this: dotnet/runtime@663c40d (dotnet/runtime#54147) |
Indeed, dotnet/runtime#54147 is what caused this. In that PR, the following assemblies will now be flowing through as netcoreapp assemblies instead of netstandard, and the windows-specific platform annotations are now applying:
Those 2 assemblies are the sources of the APIs the analyzer warnings are related to. That means the CLI is calling into windows-specific APIs without guards, but that was previously undetected. As @buyaa-n mentioned, the annotations would have to be applied all the way out the call stack. I suggest wrapping the original 3 call sites flagged with @ViktorHofer -- I don't know if we recognized dotnet/runtime#54147 as being a breaking change because of it potentially introducing build warnings. If not, we should circle back and document it as such for the Preview release. |
OK, let me add the pragma. The |
Yes, these types of changes are part of the bigger effort of dead-ending packages we don't support anymore in .NET 6 for which we have a breaking change notice in the draft right now. That said, we have another 40-50 libraries in dotnet/runtime which don't target |
Locally I'm seeing some other errors around nullable that we'd need to fix too for this PR to pass. Should have it ready in a few |
@ViktorHofer is there a list of packages that's going out of support? Also, these packages won't be available in .NET 6 even though they may have shipped during an earlier preview, is that correct? |
@terrajobst is finishing-up his draft and we will publish it as a breaking change notice alongside Preview 5. Meanwhile, here's the list of dead-ended packages. These are all now inbox starting with .NET 6 Preview 6.
|
Awesome, my workload features depend on almost all of these :) |
@wli3 Any idea what the failure is? It seems to be a test case testing for a failure, but failing for some other reason. |
Hmm the path passed in seems wrong:
|
…0617.1 Microsoft.NETCore.Platforms , Microsoft.NETCore.App.Runtime.win-x64 , Microsoft.NETCore.DotNetHostResolver , Microsoft.NET.HostModel , Microsoft.Extensions.DependencyModel , Microsoft.NETCore.App.Host.win-x64 , Microsoft.NETCore.App.Ref , System.Reflection.MetadataLoadContext , System.Resources.Extensions , System.Security.Cryptography.ProtectedData , System.Text.Encoding.CodePages , System.CodeDom , VS.Redist.Common.NetCore.SharedFramework.x64.6.0 From Version 6.0.0-preview.6.21314.2 -> To Version 6.0.0-preview.7.21317.1
Yeah I saw that, but that's the point of the test. It looks like it's generating a different exception now. It previously expected FileNotFound, but getting InvalidPath instead. |
@adamsitnik could this be related to your recent IO changes? |
I opened issue for the exception type that @joeloff already worked around. |
@joeloff I think we may need some things ported to the p6 PR too, if you're not already on it. |
This pull request updates the following dependencies
From https://github.com/dotnet/runtime