-
Notifications
You must be signed in to change notification settings - Fork 4.5k
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
Fix AOT warnings in Azure.Core #44136
Conversation
API change check APIView has identified API level changes in this PR and created following API reviews. Azure.Core.Experimental |
@jsquire
Will we consider to upgrade the .net framework version for |
sdk/core/Azure.Core/src/Shared/NextLinkOperationImplementation.cs
Outdated
Show resolved
Hide resolved
eng/Packages.Data.props
Outdated
@@ -99,10 +99,10 @@ | |||
<PackageReference Update="System.Security.Cryptography.ProtectedData" Version="4.7.0" /> | |||
<PackageReference Update="System.Threading.Channels" Version="4.7.1" /> | |||
<PackageReference Update="System.Threading.Tasks.Extensions" Version="4.5.4" /> | |||
<PackageReference Update="System.Text.Json" Version="4.7.2" /> | |||
<PackageReference Update="System.Text.Encodings.Web" Version="4.7.2" /> | |||
<PackageReference Update="System.Text.Json" Version="6.0.0" /> |
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 v6.x line are confirmed safe with Azure Functions and PowerShell.
eng/Packages.Data.props
Outdated
@@ -99,10 +99,10 @@ | |||
<PackageReference Update="System.Security.Cryptography.ProtectedData" Version="4.7.0" /> | |||
<PackageReference Update="System.Threading.Channels" Version="4.7.1" /> | |||
<PackageReference Update="System.Threading.Tasks.Extensions" Version="4.5.4" /> | |||
<PackageReference Update="System.Text.Json" Version="4.7.2" /> | |||
<PackageReference Update="System.Text.Encodings.Web" Version="4.7.2" /> | |||
<PackageReference Update="System.Text.Json" Version="6.0.0" /> |
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.
Let's move to the latest version in the 6.x line rather than 6.0.0 for all of these.
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.
Updated System.Text.Json
to 6.0.9
, 6.0.0
is the only stable version for the other two.
@@ -13,6 +13,7 @@ | |||
<ItemGroup> | |||
<PackageReference Include="Azure.Core" /> | |||
<PackageReference Include="Microsoft.CSharp" /> | |||
<PackageReference Include="System.Text.Json" /> |
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.
This is most likely needed in the short-term due to the package bump. Please add a comment here documenting that and open an issue to remove this once the next version of Azure.Core is released.
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.
Created #44165 for tracking
@@ -35,7 +35,7 @@ void IUtf8JsonSerializable.Write(Utf8JsonWriter writer) | |||
} | |||
DataFactoryLinkedServiceReference? store = default; | |||
DataFactoryElement<string>? secretName = default; | |||
Optional<DataFactoryElement<string>> secretVersion = default; | |||
DataFactoryElement<string>? secretVersion = default; |
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.
@JoshLove-msft: Would you offer thoughts on the data factory bits?
@@ -11,6 +11,7 @@ | |||
<PackageReference Include="NUnit3TestAdapter" /> | |||
<PackageReference Include="Microsoft.NET.Test.Sdk" /> | |||
<PackageReference Include="Moq" /> | |||
<PackageReference Include="System.Text.Json" /> |
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.
Same comment as for Experimental. Please track this via comment and issue, as this should be a short-term need.
@@ -122,7 +126,7 @@ internal class NextLinkOperationImplementation : IOperation | |||
finalStateVia = OperationFinalStateVia.Location; | |||
} | |||
|
|||
string headerSourceStr = GetContentFromRehydrationToken(lroDetails, "headerSource"); | |||
string headerSourceStr = GetContentFromRehydrationToken(lroDetails, "headerSource")!; |
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.
If we're sure that the outcome here is not null, why is GetContentFromRehydrationToken
marked as nullable?
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.
Because the nullability depends on the original input, for instance, lastKnownLocation
is nullable, we can still get null from RehydrationToken
, other fields are not nullable we can ensure they are not null here.
The Azure.Core team has been discussing/wanting to bump the T2 references to the 6.x line for the framework packages, including System.Text.Json and have done our due diligence to ensure that they won't cause problems with the Functions in-process environment nor PowerShell. This is something that we've agreed that we want to do, but we haven't yet had a forcing function to make it a priority. We'll need to do a very careful test pass, both CI and Live, for a large portion of the T2 packages across the data and management planes to ensure that the bump doesn't cause regressions. As long as we do that, we should be good with the version bumps.
No, we cannot change the target frameworks for the T1 package. That said, we shouldn't be bumping the T1 package references. There's a reason why we keep two distinct sets. Let's revert the T1 section back which will ensure that Batch stays stable. |
38fb07c
to
cec377b
Compare
After upgrading the packages, there is a version conflict error for
Feel like this is an issue for Tried to release an alpha version of |
The only option to fix this I found is to upgrade |
@@ -22,6 +22,8 @@ | |||
|
|||
<ItemGroup> | |||
<PackageReference Include="Azure.Core" /> |
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.
Rather than adding temp references to STJ in these packages, I'd strongly suggest that you use a temporary project reference to Azure.Core to align all of the transitive dependencies.
/azp run net - eventgrid - tests |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run net - eventgrid - tests |
Azure Pipelines successfully started running 1 pipeline(s). |
fixing in #44208 instead |
Resolves #44127
https://github.com/dotnet/runtime/blob/3b95ae9494c6d1580b2ccbf06db7b04af1a265b1/src/libraries/System.Memory.Data/src/System/BinaryData.cs#L88-L92
to
https://github.com/dotnet/runtime/blob/3b95ae9494c6d1580b2ccbf06db7b04af1a265b1/src/libraries/System.Memory.Data/src/System/BinaryData.cs#L140
https://github.com/dotnet/runtime/blob/3b95ae9494c6d1580b2ccbf06db7b04af1a265b1/src/libraries/System.Memory.Data/src/System/BinaryData.cs#L424-L427
to
https://github.com/dotnet/runtime/blob/3b95ae9494c6d1580b2ccbf06db7b04af1a265b1/src/libraries/System.Text.Json/src/System/Text/Json/Nodes/JsonNode.Parse.cs#L96-L99
JsonNode
, upgradeSystem.Text.Json
to6.0.0
and its dependencies to6.0.0
The AOT warnings now running from https://github.com/Azure/azure-sdk-for-net/blob/main/sdk/monitor/Azure.Monitor.OpenTelemetry.Exporter/tests/Azure.Monitor.OpenTelemetry.Exporter.AotCompatibilityTestApp/test-aot-compat.ps1 are
Other options are:
JsonElement
, which we need to traverse it every time we look up a value, not efficient asJsonObject
.NextLinkOperationImplementation.cs
from shared core, so that we can use internal members ofRehdyrationToken
in it directlyRehydrationToken
public as in Make RehydrationToken members public #43549