Skip to content
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

Remove package references to runtime and ref pack files when targeting .NET 6+. #1536

Closed
wants to merge 3 commits into from

Conversation

AraHaan
Copy link
Member

@AraHaan AraHaan commented Mar 7, 2022

In .NET 6+ minimize network load on restore by using the ones from the ref and runtime packs over the package versions.

@AraHaan
Copy link
Member Author

AraHaan commented Mar 7, 2022

Sad the CI does not have the .NET 6 SDK it looks like.

@JRahnama
Copy link
Contributor

JRahnama commented Mar 7, 2022

@AraHaan we are adding net6 to driver build pretty soon. Maybe in preview 2 release. That will unblock some other features we are trying to add.

@AraHaan
Copy link
Member Author

AraHaan commented Mar 7, 2022

Sad, too bad it cant be done right before the preview 1 release so we can see some simple configuration changes like this make their way into preview 1 as well (also so that way I do not have to wait long to notice the changes I made getting pulled into my codebase as well) thanks to having this as an indirect dependency from EFCore.

<PackageReference Include="Microsoft.Data.SqlClient.SNI.runtime" Version="$(MicrosoftDataSqlClientSNIRuntimeVersion)" />
<PackageReference Condition="$(TargetGroup) == 'netcoreapp' " Include="System.Diagnostics.DiagnosticSource" Version="$(SystemDiagnosticsDiagnosticSourceVersion)" />
<PackageReference Condition="'$(TargetGroup)' == 'netstandard' or ('$(TargetGroup)' == 'netcoreapp' and '$(_TargetFrameworkVersionWithoutV)' &lt;= '6.0')" Include="System.Diagnostics.DiagnosticSource" Version="$(SystemDiagnosticsDiagnosticSourceVersion)" />
<PackageReference Include="Azure.Identity" Version="$(AzureIdentityVersion)" />
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will need to be made to not reference the nuget package versions of the assemblies in the normal runtime and ref pack files as well when targeting .NET 6+ (will require PR to their repo).

@AraHaan
Copy link
Member Author

AraHaan commented Mar 23, 2022

@JRahnama so maybe sometime in April. Mind adding this to the p2 or p3 milestone then so when it happens this can get merged when it's unblocked?

@JRahnama
Copy link
Contributor

@AraHaan I was going through this PR and made some testing. TargetFrameworkIdentifier and similar names apparently wont work when multi frame works are being specified. Netcoreapp 3.1 is going out of support by end of this year and we are working on adding net6 builds to the driver. Just bear with us for the moment. Thanks for you patience.

@AraHaan
Copy link
Member Author

AraHaan commented Apr 30, 2022

So would it be ok to eventually remove .NET Core 3.1 from SqlClient when that happens (to force users to update to .NET 6+) if they are not targeting .NET Framework using SqlClient?

@AraHaan
Copy link
Member Author

AraHaan commented Apr 30, 2022

@JRahnama eta on when adding .NET 6 might be completed?

@JRahnama
Copy link
Contributor

JRahnama commented May 1, 2022

@JRahnama eta on when adding .NET 6 might be completed?

We will resume working on this after first GA at the end of May. Since 3.1 EOL is Dec 3, 2022.

@AraHaan
Copy link
Member Author

AraHaan commented May 1, 2022

Sad that the .NET 6 SDK (at least) cant be installed before then as it can be used to build the older TFMs still.

@AraHaan
Copy link
Member Author

AraHaan commented May 9, 2022

I have almost got the codebase to build for .NET 6, I had to remove TargetGroup as for some reason it does not work well with TargetFrameworkIdentifier.

After that I had to work around errors from the usage of:

  • WebClient (Suppress for now)
  • CAS (Avoid usage in .NET 6+ using #if)
  • CER (Avoid usage in .NET 6+ using #if)
  • Suppress CA1416 in .editorconfig for files with the pattern *.Windows.cs as they were false positives (at least I hope they were).

And with that the codebase now builds with .NET 6 added (locally).

…g .NET 6+.

In .NET 6+ minimize network load on restore by using the ones from the ref and runtime packs over the package versions.
@AraHaan AraHaan force-pushed the patch-1 branch 3 times, most recently from 4db9a86 to 0fab613 Compare May 9, 2022 04:23
<PackageReference Include="Microsoft.Data.SqlClient.SNI.runtime" Version="$(MicrosoftDataSqlClientSNIRuntimeVersion)" />
<PackageReference Condition="$(TargetGroup) == 'netcoreapp' " Include="System.Diagnostics.DiagnosticSource" Version="$(SystemDiagnosticsDiagnosticSourceVersion)" />
<PackageReference Condition="'$(TargetFrameworkIdentifier)' == '.NETStandard' or ('$(TargetFrameworkIdentifier)' == '.NETCoreApp' and $([MSBuild]::VersionLessThan($(TargetFrameworkVersion), 3.1)))" Include="System.Diagnostics.DiagnosticSource" Version="$(SystemDiagnosticsDiagnosticSourceVersion)" />
<PackageReference Include="Azure.Identity" Version="$(AzureIdentityVersion)" />
<PackageReference Include="Microsoft.Identity.Client" Version="$(MicrosoftIdentityClientVersion)" />
<PackageReference Include="Microsoft.IdentityModel.Protocols.OpenIdConnect" Version="$(MicrosoftIdentityModelProtocolsOpenIdConnectVersion)" />
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Microsoft.IdentityModel.Tokens (dependency of this) seems to needlessly depend on the Microsoft.CSharp, and the System.Security.Cryptography.Cng package.

When these 2 are fixed that would be a big help to me and others.

Signed-off-by: AraHaan <seandhunt_7@yahoo.com>
@ErikEJ
Copy link
Contributor

ErikEJ commented Jan 25, 2024

@JRahnama This PR seems obsolete now?

@JRahnama
Copy link
Contributor

Closing this PR as it is obsolete now.

@JRahnama JRahnama closed this Jan 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants