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 .NET8.0 targets to projects #4918

Merged
merged 22 commits into from
Oct 18, 2023

Conversation

vishweshbankwar
Copy link
Member

@vishweshbankwar vishweshbankwar commented Oct 4, 2023

Adds .NET8.0 target to all projects.

@vishweshbankwar vishweshbankwar marked this pull request as ready for review October 4, 2023 22:25
@vishweshbankwar vishweshbankwar requested a review from a team October 4, 2023 22:25
@codecov
Copy link

codecov bot commented Oct 4, 2023

Codecov Report

Merging #4918 (1be9bb8) into main (83ea1ec) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4918      +/-   ##
==========================================
+ Coverage   83.28%   83.30%   +0.01%     
==========================================
  Files         295      295              
  Lines       12360    12361       +1     
==========================================
+ Hits        10294    10297       +3     
+ Misses       2066     2064       -2     
Flag Coverage Δ
unittests 83.30% <100.00%> (+0.01%) ⬆️

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

Files Coverage Δ
...metheus.AspNetCore/PrometheusExporterMiddleware.cs 62.06% <100.00%> (+1.35%) ⬆️

... and 8 files with indirect coverage changes

@vishweshbankwar vishweshbankwar changed the title [ASP.NET Core] Add .NET8.0 target Add .NET8.0 targets to projects Oct 4, 2023
Copy link
Contributor

@Yun-Ting Yun-Ting left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you.

Copy link
Member

@CodeBlanch CodeBlanch left a comment

Choose a reason for hiding this comment

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

See comment about unshipped vs shipped files.

@CodeBlanch
Copy link
Member

CodeBlanch commented Oct 5, 2023

I asked @Yun-Ting if this was required for AOT and she said: no. So my question is, why do we need net8.0 on all the targets?

@utpilla
Copy link
Contributor

utpilla commented Oct 5, 2023

I asked @Yun-Ting if this was required for AOT and she said: no. So my question is, why do we need net8.0 on all the targets?

This goes back to the discussion for #4595, which added a net6.0 target to every project even if a particular project didn't really need any API from .NET 6. I think back then we agreed that doing this comes with a few benefits: overall ease of maintenance, newer code analysis etc.

@reyang
Copy link
Member

reyang commented Oct 5, 2023

I asked @Yun-Ting if this was required for AOT and she said: no. So my question is, why do we need net8.0 on all the targets?

I think the answer is YES now. @Yun-Ting to confirm.

@vishweshbankwar
Copy link
Member Author

I asked @Yun-Ting if this was required for AOT and she said: no. So my question is, why do we need net8.0 on all the targets?

This goes back to the discussion for #4595, which added a net6.0 target to every project even if a particular project didn't really need any API from .NET 6. I think back then we agreed that doing this comes with a few benefits: overall ease of maintenance, newer code analysis etc.

+1 And eventually .net6.0 targets will be removed next year.

@reyang
Copy link
Member

reyang commented Oct 5, 2023

I asked @Yun-Ting if this was required for AOT and she said: no. So my question is, why do we need net8.0 on all the targets?

This goes back to the discussion for #4595, which added a net6.0 target to every project even if a particular project didn't really need any API from .NET 6. I think back then we agreed that doing this comes with a few benefits: overall ease of maintenance, newer code analysis etc.

+1 And eventually .net6.0 targets will be removed next year.

+1

@CodeBlanch
Copy link
Member

I think back then we agreed that doing this comes with a few benefits: overall ease of maintenance, newer code analysis etc.

  • Overall ease of maintenance I disagree with. Each new target adds a lot of maintenance burden. Api files to maintain being the primary headache. When it comes time to remove net6.0 we can add whatever new supported target we need then. That way we lose the api files for one framework and gain them for another (net zero). Right now we are gaining burden for unclear benefit (for me).

  • Code analysis comes with the SDK, not the targets. By upgrading to .NET 8 SDK we have already gained that. PS: For most projects we're not currently running the analyzers 🤣

I think we should consider adding the targets if/when we actually need something.

Copy link
Member

@CodeBlanch CodeBlanch left a comment

Choose a reason for hiding this comment

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

I'm not totally convinced adding these targets is good, but the work LGTM 😄

@Yun-Ting
Copy link
Contributor

Yun-Ting commented Oct 5, 2023

I asked @Yun-Ting if this was required for AOT and she said: no. So my question is, why do we need net8.0 on all the targets?

I think the answer is YES now. @Yun-Ting to confirm.

The benefits of bringing net8 to the projects are that we are able to access to new APIs and we can get better AOT/trimming experience at build-time if targeting net8 directly.

cc @eerhardt, @vitek-karas

@vitek-karas
Copy link
Contributor

Code analysis comes with the SDK, not the targets. By upgrading to .NET 8 SDK we have already gained that. PS: For most projects we're not currently running the analyzers 🤣

This actually is not the case for trimming/AOT analyzer. Those are tied to the TFM of the project. (This is intentional because there's some interaction between the analyzers and the framework methods being annotated and maintaining a working matrix would be a giant headache).

That said, I would not consider this the sole reason to upgrade (unless you have more specific needs which are met by the upgrade). There will be some slight differences in analyzer and Native AOT compiler behavior anyway and in case of discrepancies the NativeAOT compiler is always the correct one (presuming no bugs in the compiler). It should be enough to run the AOT test app (in CI and locally) targeting net8.0 as that will get you the .NET 8 NativeAOT compiler (the compiler also versions with the TFM of the app, not the SDK).

@utpilla utpilla merged commit 1ec7a63 into open-telemetry:main Oct 18, 2023
35 checks passed
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.

6 participants