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

Fix TFM bugs #4912

merged 4 commits into from
Oct 3, 2023

Conversation

reyang
Copy link
Member

@reyang reyang commented Oct 3, 2023

Fix two issues:

  1. the TFM comparsion logic should use != true instead of == false since in certain environments the property doesn't exist.
  2. the System.Net.Http reference should be coming from framework instead of package, @cijothomas explained to me that https://www.nuget.org/packages/System.Net.Http/ nuget should not be used at all because it's abandoned since year 2018.

@reyang reyang requested a review from a team October 3, 2023 04:39
@@ -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).

@codecov
Copy link

codecov bot commented Oct 3, 2023

Codecov Report

Merging #4912 (3085fb3) into main (c18ff65) will increase coverage by 0.20%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    open-telemetry/opentelemetry-dotnet#4912      +/-   ##
==========================================
+ Coverage   83.30%   83.51%   +0.20%     
==========================================
  Files         295      295              
  Lines       12294    12294              
==========================================
+ Hits        10242    10267      +25     
+ Misses       2052     2027      -25     
Flag Coverage Δ
unittests 83.51% <ø> (+0.20%) ⬆️

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

see 6 files with indirect coverage changes

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

@CodeBlanch CodeBlanch merged commit dc1f09d into open-telemetry:main Oct 3, 2023
34 checks passed
@reyang reyang deleted the reyang/fix-TFMs branch October 3, 2023 18:59
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.

3 participants