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.

Sign up for GitHub

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

Merged
merged 16 commits into from
Sep 20, 2024
Merged

[repo] Replace .NET6 target with .NET9 #5832

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
d818a1a
Update System.Diagnostics.DiagnosticSource and Microsoft.Extensions.*…
rajkumar-rangaraj Sep 13, 2024
61b1178
Fix build error from .NET 6.0 complex-objects
rajkumar-rangaraj Sep 16, 2024
e839b5d
Fix build errors
rajkumar-rangaraj Sep 16, 2024
265fc44
Replace .NET6 target with .NET9
rajkumar-rangaraj Sep 18, 2024
979eca4
Merge branch 'main' into rajrang/dotnet9.0-rc.1
rajkumar-rangaraj Sep 18, 2024
85df505
fix lint
rajkumar-rangaraj Sep 18, 2024
9c35c67
Merge branch 'rajrang/dotnet9.0-rc.1' of https://github.com/rajkumar-…
rajkumar-rangaraj Sep 18, 2024
a57980d
revert examples/Directory.Packages.props
rajkumar-rangaraj Sep 18, 2024
9ace489
revert test/Directory.Packages.props
rajkumar-rangaraj Sep 18, 2024
8554c11
Fix docfx and package validation.
rajkumar-rangaraj Sep 18, 2024
5775df8
PR feedback
rajkumar-rangaraj Sep 19, 2024
ae13543
Style tweaks.
CodeBlanch Sep 19, 2024
b28eb52
Update .github/ISSUE_TEMPLATE/bug_report.yml
CodeBlanch Sep 19, 2024
d42fbef
Tweak.
CodeBlanch Sep 19, 2024
1eb93cb
Workaround odd dotnet-format behavior.
CodeBlanch Sep 19, 2024
d6940fd
Merge branch 'main' into rajrang/dotnet9.0-rc.1
CodeBlanch Sep 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions .editorconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,15 @@ dotnet_diagnostic.IDE0002.severity = warning
# IDE0005: Remove unnecessary import
dotnet_diagnostic.IDE0005.severity = warning

# IDE0058: Remove unnecessary expression value
dotnet_diagnostic.IDE0058.severity = none

rajkumar-rangaraj marked this conversation as resolved.
Show resolved Hide resolved
# IDE0130: Namespace does not match folder structure
dotnet_diagnostic.IDE0130.severity = none

# IDE0290: Use primary constructor
dotnet_diagnostic.IDE0290.severity = none

# RS0041: Public members should not use oblivious types
dotnet_diagnostic.RS0041.severity = suggestion

Expand Down
2 changes: 1 addition & 1 deletion .github/ISSUE_TEMPLATE/bug_report.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ body:
- type: input
attributes:
label: Runtime Version
description: What .NET runtime version did you use? (e.g. `net462`, `net48`, `netcoreapp3.1`, `net6.0` etc. You can find this information from the `*.csproj` file)
description: What .NET runtime version did you use? (e.g. `net462`, `net48`, `net8.0` etc. You can find this information from the `*.csproj` file)
CodeBlanch marked this conversation as resolved.
Show resolved Hide resolved
validations:
required: true

Expand Down
4 changes: 2 additions & 2 deletions .github/workflows/Component.BuildTest.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ on:
required: false
type: string
tfm-list:
default: '[ "net462", "net6.0", "net8.0" ]'
default: '[ "net462", "net8.0", "net9.0" ]'
required: false
type: string

Expand All @@ -42,7 +42,7 @@ jobs:
- os: otel-linux-arm64
version: net462
- os: otel-linux-arm64
version: net6.0
version: net8.0

runs-on: ${{ matrix.os }}
steps:
Expand Down
4 changes: 2 additions & 2 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ jobs:
strategy:
fail-fast: false
matrix:
version: [ net6.0, net8.0 ]
version: [ net8.0, net9.0 ]
steps:
- uses: actions/checkout@v4
- name: Run OTLP Exporter docker compose
Expand All @@ -129,7 +129,7 @@ jobs:
strategy:
fail-fast: false
matrix:
version: [ net6.0, net8.0 ]
version: [ net8.0, net9.0 ]
steps:
- uses: actions/checkout@v4
- name: Run W3C Trace Context docker compose
Expand Down
3 changes: 3 additions & 0 deletions .github/workflows/docfx.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@ jobs:
- name: check out code
uses: actions/checkout@v4

- name: Setup dotnet
uses: actions/setup-dotnet@v4

- name: install docfx
run: dotnet tool install -g docfx

Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/verifyaotcompat.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ jobs:
fail-fast: false # ensures the entire test matrix is run, even if one permutation fails
matrix:
os: [ ubuntu-latest, windows-latest ]
version: [ net8.0 ]
version: [ net8.0, net9.0 ]

runs-on: ${{ matrix.os }}
steps:
Expand Down
6 changes: 3 additions & 3 deletions Directory.Packages.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,10 +97,10 @@
<PackageVersion Include="xunit.runner.visualstudio" Version="[2.8.2,3.0)" />
</ItemGroup>

<ItemGroup Condition="'$(TargetFramework)' == 'net6.0'">
<PackageVersion Include="Microsoft.AspNetCore.TestHost" Version="6.0.33" />
</ItemGroup>
<ItemGroup Condition="'$(TargetFramework)' == 'net8.0'">
<PackageVersion Include="Microsoft.AspNetCore.TestHost" Version="8.0.8" />
</ItemGroup>
<ItemGroup Condition="'$(TargetFramework)' == 'net9.0'">
<PackageVersion Include="Microsoft.AspNetCore.TestHost" Version="9.0.0-rc.1.24452.1" />
</ItemGroup>
</Project>
2 changes: 1 addition & 1 deletion OpenTelemetry.sln
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,8 @@ Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "build", "build", "{7CB2F02E
build\debug.snk = build\debug.snk
Directory.Packages.props = Directory.Packages.props
build\docfx.cmd = build\docfx.cmd
build\docker-compose.net6.0.yml = build\docker-compose.net6.0.yml
build\docker-compose.net8.0.yml = build\docker-compose.net8.0.yml
build\docker-compose.net9.0.yml = build\docker-compose.net9.0.yml
build\GlobalAttrExclusions.txt = build\GlobalAttrExclusions.txt
build\opentelemetry-icon-color.png = build\opentelemetry-icon-color.png
build\OpenTelemetry.prod.loose.ruleset = build\OpenTelemetry.prod.loose.ruleset
Expand Down
2 changes: 1 addition & 1 deletion build/Common.nonprod.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
</PropertyGroup>

<PropertyGroup>
<DefaultTargetFrameworkForExampleApps>net8.0</DefaultTargetFrameworkForExampleApps>
<DefaultTargetFrameworkForExampleApps>net9.0</DefaultTargetFrameworkForExampleApps>
rajkumar-rangaraj marked this conversation as resolved.
Show resolved Hide resolved
</PropertyGroup>

<PropertyGroup Condition="$(MSBuildProjectName.EndsWith('.Tests'))">
Expand Down
4 changes: 4 additions & 0 deletions build/Common.prod.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,10 @@
<ContinuousIntegrationBuild Condition="'$(Deterministic)'=='true'">true</ContinuousIntegrationBuild>
</PropertyGroup>

<ItemGroup>
<PackageValidationBaselineFrameworkToIgnore Include="net6.0" />
</ItemGroup>

<ItemGroup>
<PackageReference Include="MinVer" PrivateAssets="All" Condition="'$(IntegrationBuild)' != 'true'" />
<PackageReference Include="Microsoft.SourceLink.GitHub" PrivateAssets="All" Condition="'$(IntegrationBuild)' != 'true'" />
Expand Down
14 changes: 7 additions & 7 deletions build/Common.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,18 +28,18 @@
<NetFrameworkSupportedVersions>net481;net48;net472;net471;net47;net462</NetFrameworkSupportedVersions>

<!-- production TFMs -->
<TargetFrameworksForLibraries>net8.0;net6.0;netstandard2.0;$(NetFrameworkMinimumSupportedVersion)</TargetFrameworksForLibraries>
<TargetFrameworksForLibrariesExtended>net8.0;net6.0;netstandard2.1;netstandard2.0;$(NetFrameworkMinimumSupportedVersion)</TargetFrameworksForLibrariesExtended>
<TargetFrameworksForPrometheusAspNetCore>net8.0;net6.0</TargetFrameworksForPrometheusAspNetCore>
<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>
Comment on lines +31 to +33
Copy link
Contributor

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

Copy link
Contributor Author

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?

Copy link
Member

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...

Copy link
Member

@CodeBlanch CodeBlanch Sep 19, 2024 •
edited
Loading

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Was net8.0 more valid to add because it's a LTS release? net7.0 end of support is before net6.0, so it'll be removed before net6.0 anyways. net8.0, however, will outlive net6.0. Not adding net8.0 will unavoidably leave a gap somewhere in the timeline.

image

Copy link
Member

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.


<!-- non-production TFMs -->
<TargetFrameworksForAspNetCoreTests>net8.0;net6.0</TargetFrameworksForAspNetCoreTests>
<TargetFrameworksForAotCompatibilityTests>net8.0</TargetFrameworksForAotCompatibilityTests>
<TargetFrameworksForDocs>net8.0;net6.0</TargetFrameworksForDocs>
<TargetFrameworksForAspNetCoreTests>net9.0;net8.0</TargetFrameworksForAspNetCoreTests>
<TargetFrameworksForAotCompatibilityTests>net9.0;net8.0</TargetFrameworksForAotCompatibilityTests>
<TargetFrameworksForDocs>net9.0;net8.0</TargetFrameworksForDocs>
<TargetFrameworksForDocs Condition="$(OS) == 'Windows_NT' And '$(UsingMicrosoftNETSdkWeb)' != 'True'">
$(TargetFrameworksForDocs);$(NetFrameworkSupportedVersions)
</TargetFrameworksForDocs>
<TargetFrameworksForTests>net8.0;net6.0</TargetFrameworksForTests>
<TargetFrameworksForTests>net9.0;net8.0</TargetFrameworksForTests>
<TargetFrameworksForTests Condition="$(OS) == 'Windows_NT'">
$(TargetFrameworksForTests);$(NetFrameworkMinimumSupportedVersion)
</TargetFrameworksForTests>
Expand Down
9 changes: 0 additions & 9 deletions build/docker-compose.net6.0.yml
View file
Open in desktop

This file was deleted.

2 changes: 1 addition & 1 deletion build/docker-compose.net8.0.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,4 @@ services:
args:
PUBLISH_FRAMEWORK: net8.0
TEST_SDK_VERSION: "8.0"
BUILD_SDK_VERSION: "8.0"
BUILD_SDK_VERSION: "9.0"
9 changes: 9 additions & 0 deletions build/docker-compose.net9.0.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
version: '3.7'

services:
tests:
build:
args:
PUBLISH_FRAMEWORK: net9.0
TEST_SDK_VERSION: "9.0"
BUILD_SDK_VERSION: "9.0"
2 changes: 1 addition & 1 deletion global.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"sdk": {
"rollForward": "latestFeature",
"version": "8.0.100"
"version": "9.0.100-rc.1.24452.12"
}
}
4 changes: 3 additions & 1 deletion src/OpenTelemetry/Internal/SelfDiagnosticsConfigParser.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,9 @@ public bool TryGetConfiguration(
this.configBuffer = buffer;
}

file.Read(buffer, 0, buffer.Length);
// TODO: Fix CA2022 - Avoid inexact read with 'System.IO.FileStream.Read(byte[], int, int)'
rajkumar-rangaraj marked this conversation as resolved.
Show resolved Hide resolved
// Added _ = as a workaround to suppress the warning
_ = file.Read(buffer, 0, buffer.Length);
string configJson = Encoding.UTF8.GetString(buffer);

if (!TryParseLogDirectory(configJson, out logDirectory))
Expand Down
4 changes: 2 additions & 2 deletions test/OpenTelemetry.Instrumentation.W3cTraceContext.Tests/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@
# This should be run from the root of the repo:
# docker build --file test/OpenTelemetry.Instrumentation.W3cTraceContext.Tests/Dockerfile .

ARG BUILD_SDK_VERSION=8.0
ARG TEST_SDK_VERSION=8.0
ARG BUILD_SDK_VERSION=9.0
rajkumar-rangaraj marked this conversation as resolved.
Show resolved Hide resolved
ARG TEST_SDK_VERSION=9.0

FROM ubuntu AS w3c
#Install git
Expand Down
8 changes: 6 additions & 2 deletions test/OpenTelemetry.Tests/Internal/SelfDiagnosticsEventListenerTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,9 @@ public void SelfDiagnosticsEventListener_EmitEvent_OmitAsConfigured()

using FileStream file = File.Open(LOGFILEPATH, FileMode.Open, FileAccess.Read, FileShare.ReadWrite | FileShare.Delete);
var buffer = new byte[256];
file.Read(buffer, 0, buffer.Length);

// Suppress CA2022 error: Avoid inexact read with 'System.IO.FileStream.Read(byte[], int, int)'
_ = file.Read(buffer, 0, buffer.Length);
Assert.Equal('\0', (char)buffer[0]);
}

Expand Down Expand Up @@ -256,7 +258,9 @@ private static void AssertFileOutput(string filePath, string eventMessage)
{
using FileStream file = File.Open(filePath, FileMode.Open, FileAccess.Read, FileShare.ReadWrite | FileShare.Delete);
var buffer = new byte[256];
file.Read(buffer, 0, buffer.Length);

// Suppress CA2022 error: Avoid inexact read with 'System.IO.FileStream.Read(byte[], int, int)'
_ = file.Read(buffer, 0, buffer.Length);
string logLine = Encoding.UTF8.GetString(buffer);
string logMessage = ParseLogMessage(logLine);
Assert.StartsWith(eventMessage, logMessage);
Expand Down
Loading