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

Add support for .NET 9 #2946

Merged
merged 83 commits into from
Nov 25, 2024
Merged

Add support for .NET 9 #2946

merged 83 commits into from
Nov 25, 2024

Conversation

mdaigle
Copy link
Contributor

@mdaigle mdaigle commented Oct 31, 2024

Enhancements:

  • Adds .NET 9 to target frameworks

Fixes:

Package Versions:

  • Adds System.Security.Cryptography.Pkcs:9.0.0 to address SYSLIB0057
  • Adds Microsoft.Bcl.Cryptography:9.0.0, dependency of the pkcs package
  • Bumps Microsoft.Extensions.Caching.Memory to 9.0.0
  • Bumps System.Configuration.ConfigurationManager to 9.0.0

Test Dependencies:

  • Bumps Microsoft.DotNet.RemoteExecutor to 10.0.0-beta.24564.1
  • Bumps Xunit to 2.6.3 for all frameworks

@mdaigle mdaigle changed the title Add support for .NET 9 Not Ready for Review | Add support for .NET 9 Oct 31, 2024
@mdaigle mdaigle marked this pull request as ready for review October 31, 2024 19:27
Copy link
Member

@roji roji left a comment

Choose a reason for hiding this comment

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

Am curious - is there any net9.0 API that's actually needed/used? If not, is there a specific reason to add the net9.0 TFM?

@David-Engel
Copy link
Contributor

Am curious - is there any net9.0 API that's actually needed/used? If not, is there a specific reason to add the net9.0 TFM?

@roji JSON support in SqlDbType: dotnet/runtime#103925

@mdaigle mdaigle added the 💡 Enhancement Issues that are feature requests for the drivers we maintain. label Nov 1, 2024
@roji
Copy link
Member

roji commented Nov 2, 2024

@David-Engel of course, I forgot about that, thanks :)

Copy link

codecov bot commented Nov 22, 2024

Codecov Report

Attention: Patch coverage is 35.71429% with 9 lines in your changes missing coverage. Please review.

Project coverage is 72.60%. Comparing base (1348c3c) to head (6ac682e).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...c/Microsoft/Data/SqlClient/Server/SqlNormalizer.cs 0.00% 9 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2946      +/-   ##
==========================================
- Coverage   72.67%   72.60%   -0.08%     
==========================================
  Files         285      285              
  Lines       59160    59162       +2     
==========================================
- Hits        42997    42954      -43     
- Misses      16163    16208      +45     
Flag Coverage Δ
addons 92.58% <ø> (ø)
netcore 75.40% <35.71%> (-0.04%) ⬇️
netfx 71.00% <30.76%> (-0.07%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

namespace System.IO
{
// Helpers to read/write Span/Memory<byte> to Stream before netstandard 2.1
internal static class StreamExtensions
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a shared path we can put this which both the test projects and M.D.S can refer to? It's identical to src/Microsoft.Data.SqlClient/src/System/IO/StreamExtensions.netfx.cs.

Copy link
Member

Choose a reason for hiding this comment

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

Ideally, I would create a Common/Shared project that both test and src projects depend on, but this class is too small for that. If we have more cases like this, we can consider coming up with a common project.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mdaigle Like I said in the previous PR, internals of the MDS projects should be visible to the test projects. If they aren't, then we should make them so.

@cheenamalhotra I personally don't think we need to add another project for this purpose, if we leverage internalsvisibleto. We can discuss more if we need to, tho.

tools/props/VersionsNet9OrLater.props Outdated Show resolved Hide resolved
tools/props/Versions.props Outdated Show resolved Hide resolved
tools/props/VersionsNet8OrLater.props Outdated Show resolved Hide resolved
tools/props/Versions.props Outdated Show resolved Hide resolved
namespace System.IO
{
// Helpers to read/write Span/Memory<byte> to Stream before netstandard 2.1
internal static class StreamExtensions
Copy link
Member

Choose a reason for hiding this comment

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

Ideally, I would create a Common/Shared project that both test and src projects depend on, but this class is too small for that. If we have more cases like this, we can consider coming up with a common project.

@@ -180,7 +180,9 @@ static DataTestUtility()
AliasName = c.AliasName;
IsJsonSupported = c.IsJsonSupported;

#if NETFRAMEWORK
Copy link
Member

Choose a reason for hiding this comment

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

Can you elaborate on this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ServicePointManager is marked as obsolete in Net 9.
https://learn.microsoft.com/en-us/dotnet/fundamentals/syslib-diagnostics/syslib0014
I removed the call to no effect in the tests, so it didn't seem to be changing anything. But if there's any concern, I can add a suppression instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mdaigle Are you sure it's the ServicePointManager? I had seen similar warnings in the code when using Tls12 security type.

Copy link
Contributor

@benrr101 benrr101 left a comment

Choose a reason for hiding this comment

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

Looks good to me in general. I think we should make InternalsVisibleTo test projects (I think that change was only made in the clientx branch) so we can avoid duplication of the stream extensions. But I won't hold up approval on that.

@mdaigle mdaigle merged commit 22ac587 into dotnet:main Nov 25, 2024
252 checks passed
@mdaigle mdaigle deleted the net9-add-support branch November 25, 2024 19:13
@mdaigle mdaigle linked an issue Nov 25, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💡 Enhancement Issues that are feature requests for the drivers we maintain.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update to .Net 9
9 participants