[main] Update dependencies from dotnet/arcade #6711
Conversation
![dotnet-maestro[bot]](https://avatars.githubusercontent.com/in/16706?s=60&v=4){kind=small}
…720.12 Microsoft.DotNet.XUnitExtensions , Microsoft.DotNet.Arcade.Sdk From Version 6.0.0-beta.21365.11 -> To Version 6.0.0-beta.21370.12
…723.11 Microsoft.DotNet.XUnitExtensions , Microsoft.DotNet.Arcade.Sdk From Version 6.0.0-beta.21370.12 -> To Version 6.0.0-beta.21373.11
…726.4 Microsoft.DotNet.XUnitExtensions , Microsoft.DotNet.Arcade.Sdk From Version 6.0.0-beta.21365.11 -> To Version 6.0.0-beta.21376.4
The former isn't supported anymore, and the latter is wrong (at least in the running code—I didn't check the tests).
…44fe-8415-92626fffb0b0
…727.2 Microsoft.DotNet.XUnitExtensions , Microsoft.DotNet.Arcade.Sdk From Version 6.0.0-beta.21369.3 -> To Version 6.0.0-beta.21377.2
…728.2 Microsoft.DotNet.XUnitExtensions , Microsoft.DotNet.Arcade.Sdk From Version 6.0.0-beta.21369.3 -> To Version 6.0.0-beta.21378.2
|
@ladipro (Are you still kitten?) Are we waiting for anything before merging this? |
| @@ -632,6 +632,7 @@ internal static bool IsMaxPathLegacyWindows() | |||
| } | |||
| } | |||
|
|
|||
| #pragma warning disable CA1416 | |||
There was a problem hiding this comment.
Instead of doing this, can you follow the guidance on this warning? https://docs.microsoft.com/en-us/dotnet/core/compatibility/code-analysis/5.0/ca1416-platform-compatibility-analyzer#recommended-action
Specifically, I'd expect to see this call annotated with [SupportedOSPlatform("windows")], which will require annotating or conditionalizing up the chain.
There was a problem hiding this comment.
The guidance I followed is right below the guidance you suggested.
Also, [SupportedOSPlatform("windows")] was introduced in .net 6, as was OperatingSystem.IsWindows(), so I would have to ifdef around the attribute or IsWindows check. That's pretty terrible, so I don't think that's a good idea.
There was a problem hiding this comment.
At the very least this needs a comment.
There was a problem hiding this comment.
I can add one. It doesn't change anything, but I should correct myself: it was introduced in .net 5, not .net 6.
| @@ -160,7 +159,6 @@ private AssemblyNameExtension(SerializationInfo info, StreamingContext context) | |||
| HashAlgorithm = hashAlgorithm, | |||
| VersionCompatibility = versionCompatibility, | |||
| CodeBase = codeBase, | |||
| KeyPair = keyPair | |||
There was a problem hiding this comment.
I'm still unclear on whether it's ok to make this change in the .NET Framework version of MSBuild where this API is available and works. Can you elaborate on your reasoning there?
There was a problem hiding this comment.
AssemblyNameExtension is internal, so customers can't use it without making a custom MSBuild. We don't rely on the KeyPair part because otherwise, the .net 6 code wouldn't run. So we don't need KeyPair, and customers can't need KeyPair, so we should be able to remove it and stop serializing/deserializing it.
There was a problem hiding this comment.
We don't rely on the KeyPair part because otherwise, the .net 6 code wouldn't run. So we don't need KeyPair
This doesn't obviously follow for me. I get that we don't need it on .NET 6+. But it's never used/compared in .NET Framework scenarios?
There was a problem hiding this comment.
I won't say I took a deep look at every use, but I tried to briefly look at all our uses (outside tests) to see if they were #ifdef'd on platform, and the ones that were didn't seem to use StrongNameKeyPair. I was a little thrown by "GetPublicKeyToken" and wondered if that was a use, but it seems to still exist on .net 6 without throwing deprecated warnings, so I think it's something else. If you want me to, I can put the removal under an ifdef'd change wave or, if it makes you really uncomfortable, just ifdef it out, but then we may be serializing and deserializing something on .net framework for no reason.
There was a problem hiding this comment.
Ok, I think this is what I was looking for (from https://docs.microsoft.com/en-us/dotnet/api/system.reflection.assemblyname.keypair?view=net-5.0#remarks):
When the runtime loads an assembly, it does not set the KeyPair property. The getter for the property is only useful if the user set the property before using the AssemblyName object to create a dynamic assembly, and subsequently wants to retrieve the key pair.
Since we use this in RAR, which is not interested in dynamic assemblies, it will always be null, so dropping it is ok.
…729.2 Microsoft.DotNet.XUnitExtensions , Microsoft.DotNet.Arcade.Sdk From Version 6.0.0-beta.21369.3 -> To Version 6.0.0-beta.21379.2
| @@ -160,7 +159,6 @@ private AssemblyNameExtension(SerializationInfo info, StreamingContext context) | |||
| HashAlgorithm = hashAlgorithm, | |||
| VersionCompatibility = versionCompatibility, | |||
| CodeBase = codeBase, | |||
| KeyPair = keyPair | |||
There was a problem hiding this comment.
Ok, I think this is what I was looking for (from https://docs.microsoft.com/en-us/dotnet/api/system.reflection.assemblyname.keypair?view=net-5.0#remarks):
When the runtime loads an assembly, it does not set the KeyPair property. The getter for the property is only useful if the user set the property before using the AssemblyName object to create a dynamic assembly, and subsequently wants to retrieve the key pair.
Since we use this in RAR, which is not interested in dynamic assemblies, it will always be null, so dropping it is ok.
Forgind
added
the
merge-when-branch-open
This pull request updates the following dependencies
From https://github.com/dotnet/arcade