-
Notifications
You must be signed in to change notification settings - Fork 773
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
[repo] Replace .NET6 target with .NET9 #5832
[repo] Replace .NET6 target with .NET9 #5832
Conversation
… pacakges to .NET 9.0 RC.1
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5832 +/- ##
==========================================
+ Coverage 83.38% 86.38% +2.99%
==========================================
Files 297 257 -40
Lines 12531 11168 -1363
==========================================
- Hits 10449 9647 -802
+ Misses 2082 1521 -561
Flags with carried forward coverage won't be shown. Click here to find out more.
|
LGTM, I think https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/CONTRIBUTING.md#development-environment needs to be updated to cover .NET 9 as part of the environment setup. Can be a separate PR though. |
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.
1 I think that this PR should include information about these changes in all affected CHANGELOG files.
2. It can be follow up, but you can potentially include it also here. Review alle conditional compilation. At least NET7_0_OR_GREATER
and NET8_0_OR_GREATER
. I think that all of them can be converted just to NET. There were decision to simplify condition where possible and remove exact versions.
3. Please update test\OpenTelemetry.Instrumentation.W3cTraceContext.Tests\W3CTraceContextTests.cs around line 83. Tests are no longer executed on .NET6 so the TODO comment and the code can be simplified.
<TargetFrameworksForLibraries>net9.0;net8.0;netstandard2.0;$(NetFrameworkMinimumSupportedVersion)</TargetFrameworksForLibraries> | ||
<TargetFrameworksForLibrariesExtended>net9.0;net8.0;netstandard2.1;netstandard2.0;$(NetFrameworkMinimumSupportedVersion)</TargetFrameworksForLibrariesExtended> | ||
<TargetFrameworksForPrometheusAspNetCore>net9.0;net8.0</TargetFrameworksForPrometheusAspNetCore> |
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.
One more thing, should we really add there net9? .NET8 will be supported longer than .NET8.
Previously there were no packages targeted .NET7. Ref: #5795
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.
@CodeBlanch What are your thoughts on it?
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 don't see any particular advantage with a net9 target...
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.
Reminds me of this: #4918
Question being, why did we add net8.0
to everything? 🤣 At the time I tried to block that because it exploded the amount of public API files we had and maintaining those is a pain. But I went and changed how the public API stuff works so we don't need public API files for a TFM unless there are customizations for that TFM.
Where we are now I don't have an issue with adding net9.0
. We did it for net8.0
. Which I guess is to say I'm fine with each year adding the latest version as a target when we upgrade. Don't have a strong reason for that other than when users look at NuGet and they see explicit targets I guess it is strong indication we were intentional 🤷
/cc @alanwest
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.
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 don't see any particular advantage with a net9 target...
Me either, and as @CodeBlanch points out this is something we have pushed against before, and honestly I never understood the reasons for adding a bunch of targets.
It stems back to this comment #4591 (comment). But again, I never understood the benefits to the OpenTelemetry .NET project.
All that said, I'm fine just adding net9.0 everywhere and continuing to make that the pattern going forward. If we want to circle back to this decision then I'd prefer it be in the context of a conversation about removing all unnecessary targets and going back to the state we had before.
The best place to update this would be the RELEASENOTES.md file. This impacts all the projects in this repository. We haven't previously included the removal or addition of .NET in the changelog. If its fine, we could update this in a follow-up PR
Created an issue to track - #5848
This is on my list; I will leave a TODO for this PR. We also need to identify similar code in the repo. Since we have a comment on this test, it was easier for us to find it. |
Co-authored-by: Piotr Kiełkowicz <pkiekowicz@splunk.com>
Fixes #5806
Changes
Please provide a brief description of the changes here.
PackageValidationBaselineFrameworkToIgnore
to ignore validation against.NET 6.0
, this has to be added, as previous versions of this package had.NET 6.0
support.Next steps (follow up PRs)
System.Diagnostics.DiagnosticSource
andMicrosoft.Extensions.*
packages. (Update System.Diagnostics.DiagnosticSource and Microsoft.Extensions.* packages to .NET 9.0 #5846)Merge requirement checklist
CHANGELOG.md
files updated for non-trivial changes