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

Fix TFM bugs #4912

Merged
merged 4 commits into from
Oct 3, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 0 additions & 1 deletion Directory.Packages.props
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,6 @@
<PackageVersion Include="RabbitMQ.Client" Version="[6.5.0,7.0)" />
<PackageVersion Include="StyleCop.Analyzers" Version="[1.2.0-beta.507,2.0)" />
<PackageVersion Include="Swashbuckle.AspNetCore" Version="[6.4.0]" />
<PackageVersion Include="System.Net.Http" Version="4.3.4" />
Copy link
Member Author

@reyang reyang Oct 3, 2023

Choose a reason for hiding this comment

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

This shouldn't be used at all since it's a dead package https://www.nuget.org/packages/System.Net.Http/ (no updates since year 2018).

<PackageVersion Include="Testcontainers.MsSql" Version="3.3.0" />
<PackageVersion Include="Testcontainers.SqlEdge" Version="3.3.0" />
<PackageVersion Include="xunit" Version="[2.5.0,3.0)" />
Expand Down
2 changes: 1 addition & 1 deletion build/Common.props
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
<TargetFrameworksForAspNetCoreTests>net8.0;net7.0;net6.0</TargetFrameworksForAspNetCoreTests>
<TargetFrameworksForAotCompatibilityTests>net8.0</TargetFrameworksForAotCompatibilityTests>
<TargetFrameworksForDocs>net7.0;net6.0</TargetFrameworksForDocs>
<TargetFrameworksForDocs Condition="$(OS) == 'Windows_NT' And '$(UsingMicrosoftNETSdkWeb)' == 'False'">
<TargetFrameworksForDocs Condition="$(OS) == 'Windows_NT' And '$(UsingMicrosoftNETSdkWeb)' != 'True'">
$(TargetFrameworksForDocs);net481;net48;net472;net471;net47;net462
</TargetFrameworksForDocs>
<TargetFrameworksForTests>net8.0;net7.0;net6.0</TargetFrameworksForTests>
Expand Down
2 changes: 1 addition & 1 deletion docs/trace/getting-started-jaeger/Program.cs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
// </copyright>

using System.Diagnostics;
#if !NET6_0_OR_GREATER
#if NETFRAMEWORK
using System.Net.Http;
#endif
using OpenTelemetry;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@
<ProjectReference Include="$(RepoRoot)\src\OpenTelemetry.Exporter.OpenTelemetryProtocol\OpenTelemetry.Exporter.OpenTelemetryProtocol.csproj" />
<ProjectReference Include="$(RepoRoot)\src\OpenTelemetry.Instrumentation.Http\OpenTelemetry.Instrumentation.Http.csproj" />
</ItemGroup>

<ItemGroup>
<PackageReference Include="System.Net.Http" Condition="$(TargetFramework) == 'net472' or $(TargetFramework) == 'net48'"/>
<Reference Include="System.Net.Http" Condition="'$(TargetFrameworkIdentifier)' != '.NETCoreApp'" />
</ItemGroup>
</Project>
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
</ItemGroup>

<ItemGroup>
<PackageReference Include="System.Net.Http" Condition="'$(TargetFramework)' == 'net462'" />
Copy link
Member

Choose a reason for hiding this comment

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

I'm going to approve this but FYI what I really want is for the tests to verify everything works if the user decides to take the NuGet bits. But we should also make sure it works with the direct reference. Probably need to run the tests through both modes to do it correctly? Going to take a bit of effort to pull that off, not sure if the juice is worth the squeeze.

Copy link
Member Author

Choose a reason for hiding this comment

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

With this PR, the test is going to cover the net462 framework reference of System.Net.Http, do you think such test coverage can represent NuGet System.Net.Http (especially considering that the NuGet hasn't been updated since 2018) or we need both?

what I really want is for the tests to verify everything works if the user decides to take the NuGet bits

Could you explain a bit more here? What's the main scenario and how big is the user base?

Copy link
Member

Choose a reason for hiding this comment

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

It is definitely old, but I don't see anything on the NuGet saying it is deprecated and it has been downloaded 1.7B times so a lot of people are consuming it! The scenario is user takes a dependency on the NuGet and its bits are used instead of the shipped runtime assembly,

Copy link
Member Author

Choose a reason for hiding this comment

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

#4915

<PackageReference Include="Microsoft.NET.Test.Sdk" />
<PackageReference Include="Moq" />
<PackageReference Include="System.Text.Json" />
Expand All @@ -25,6 +24,10 @@
</PackageReference>
</ItemGroup>

<ItemGroup>
<Reference Include="System.Net.Http" Condition="'$(TargetFramework)' == 'net462'" />
</ItemGroup>

<ItemGroup>
<EmbeddedResource Include="http-out-test-cases.json">
<CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
Expand Down