-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Added explicit .NET 6 TFMs. #28911
base: main
Are you sure you want to change the base?
Added explicit .NET 6 TFMs. #28911
Conversation
Signed-off-by: AraHaan <[email protected]> Fixes Azure#28477. Fixes Azure#28492. Fixes Azure#28577.
Thank you for your contribution @AraHaan! We will review the pull request and get back to you soon. |
Will fix compile error in Azure/azure-sdk-for-net#28911.
Will fix compile error in Azure/azure-sdk-for-net#28911.
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.
Thank you for your contribution and for helping to improve the Azure SDK experience. Though this is still in draft form, there were a couple of things that caught my eye that I wanted to mention, and one request that I had for the overall pattern.
Please consider moving the conditionals to a new item group with the condition applied there. An explanation of why we're conditionalizing would also be appreciated.
Example:
<ItemGroup>
<PackageReference Include="Azure.Core" />
</ItemGroup>
<!-- These packages are included as part net6.0; the external references are not needed. -->
<ItemGroup Condition="'$(TargetFramework)' != 'net6.0'">
<PackageReference Include="Microsoft.Bcl.AsyncInterfaces" />
<PackageReference Include="System.Text.Json" />
</ItemGroup>
...hub/Azure.Messaging.EventHubs.Experimental/src/Azure.Messaging.EventHubs.Experimental.csproj
Outdated
Show resolved
Hide resolved
...e.Messaging.EventHubs.Experimental/tests/Azure.Messaging.EventHubs.Experimental.Tests.csproj
Outdated
Show resolved
Hide resolved
sdk/eventhub/Microsoft.Azure.EventHubs/src/Microsoft.Azure.EventHubs.csproj
Outdated
Show resolved
Hide resolved
sdk/servicebus/Microsoft.Azure.ServiceBus/src/Microsoft.Azure.ServiceBus.csproj
Outdated
Show resolved
Hide resolved
I feel like I might need to look into why the merge conflicts are present and if possible resolve them. |
Merging main was painless. So I have done it via: AraHaan#504 |
Merged main back in
@@ -13,8 +13,8 @@ | |||
<ItemGroup> | |||
<PackageReference Include="Azure.Core" /> | |||
<PackageReference Include="Azure.Identity" /> | |||
<PackageReference Include="System.Memory" /> | |||
<PackageReference Include="System.Text.Json" /> | |||
<PackageReference Include="System.Memory" Condition="'$(TargetFramework)' != 'net6.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.
For consistency I feel these should be in an item group rather than directly on the reference.
@@ -12,8 +12,8 @@ | |||
</PropertyGroup> | |||
<ItemGroup> | |||
<PackageReference Include="Azure.Core" /> | |||
<PackageReference Include="System.Memory" /> | |||
<PackageReference Include="System.Text.Json" /> | |||
<PackageReference Include="System.Memory" Condition="'$(TargetFramework)' != 'net6.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.
For consistency I feel these should be in an item group rather than directly on the reference.
</ItemGroup> | ||
|
||
<!-- These packages are included as part net6.0; the external references are not needed. --> | ||
<ItemGroup Condition="'$(TargetFramework)' != 'net6.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.
For maintainability should this be inverted so that adding future frameworks doesn't need the conditions to be touched?
</ItemGroup> | ||
|
||
<!-- These packages are included as part net6.0; the external references are not needed. --> | ||
<ItemGroup Condition="'$(TargetFramework)' != 'net6.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.
For maintainability should this be inverted so that adding future frameworks doesn't need the conditions to be touched?
</ItemGroup> | ||
|
||
<!-- These packages are included as part net6.0; the external references are not needed. --> | ||
<ItemGroup Condition="'$(TargetFramework)' != 'net6.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.
For maintainability should this be inverted so that adding future frameworks doesn't need the conditions to be touched?
</ItemGroup> | ||
|
||
<!-- These packages are included as part net6.0; the external references are not needed. --> | ||
<ItemGroup Condition="'$(TargetFramework)' != 'net6.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.
For maintainability should this be inverted so that adding future frameworks doesn't need the conditions to be touched?
</ItemGroup> | ||
|
||
<!-- These packages are included as part net6.0; the external references are not needed. --> | ||
<ItemGroup Condition="'$(TargetFramework)' != 'net6.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.
For maintainability should this be inverted so that adding future frameworks doesn't need the conditions to be touched?
</ItemGroup> | ||
|
||
<!-- These packages are included as part net6.0; the external references are not needed. --> | ||
<ItemGroup Condition="'$(TargetFramework)' != 'net6.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.
For maintainability should this be inverted so that adding future frameworks doesn't need the conditions to be touched?
#if NET6_0_OR_GREATER | ||
uri.Host.Contains('.', StringComparison.InvariantCulture) && | ||
#elif NETCOREAPP3_1 | ||
uri.Host.Contains(".", StringComparison.InvariantCulture) && |
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.
Should these be merged into 1 condition.
@@ -13,7 +13,11 @@ | |||
</PropertyGroup> | |||
<ItemGroup> | |||
<PackageReference Include="Azure.Core" /> | |||
<PackageReference Include="Azure.MixedReality.Authentication" /> | |||
<PackageReference Include="Azure.MixedReality.Authentication" VersionOverride="1.0.1" /> |
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.
Is this version override needed?
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.
I will need to check once more yeah.
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.
Added a comment about better way to check target platform that will work with newer NET versions too.
</ItemGroup> | ||
|
||
<!-- These packages are included as part net6.0; the external references are not needed. --> | ||
<ItemGroup Condition="'$(TargetFramework)' != 'net6.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.
It would be better to use Condition="!$([MSBuild]::IsTargetFrameworkCompatible($(TargetFramework), 'net6.0'))"
for all such conditions, compatibility check is future-proof
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.
That is true, alright I will do it when I can.
The issue that was open for this was automatically closed and locked so it could not be reopened by myself. Someone might have to manually reopen it that can do such. |
Hi @AraHaan. Thank you for your interest in helping to improve the Azure SDK experience and for your contribution. We've noticed that there hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by pushing some changes or leaving a comment. Otherwise, we'll close this out in 7 days. |
This is still a wip. |
Hi @AraHaan. Thank you for your interest in helping to improve the Azure SDK experience and for your contribution. We've noticed that there hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by pushing some changes or leaving a comment. Otherwise, we'll close this out in 7 days. |
This is still a wip. |
Hi @AraHaan. Thank you for your interest in helping to improve the Azure SDK experience and for your contribution. We've noticed that there hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by pushing some changes or leaving a comment. Otherwise, we'll close this out in 7 days. |
We are still awaiting a review |
Apologies for the long delay here; this turned out to be an issue with many more differing opinions than expected and required a good amount of internal discussion. From the feedback that was shared here and by other community members, we've formalized a plan for multitargeting going forward where we will always have an explicit target for the latest LTS. The working draft of this plan is:
With the pending end-of-life for |
Fixes #28477.
Fixes #28492.
Fixes #28577.
Prerequisites: