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

Jump to bottom

Enable AOT compatibility for all libraries #4625

Merged
merged 3 commits into from
Nov 28, 2023
Merged

Enable AOT compatibility for all libraries #4625

merged 3 commits into from
Nov 28, 2023

Conversation

eerhardt
Copy link
Member

@eerhardt eerhardt commented Oct 27, 2023 •
edited
Loading

Enable AOT compatibility for all libraries and fix AOT warnings.

When we get a build of .NET 8 with all the source generator bugs fixed, I will port this to main.

Microsoft Reviewers: Open in CodeFlow

@ghost ghost assigned eerhardt Oct 27, 2023
@eerhardt eerhardt mentioned this pull request Oct 27, 2023
Open
@eerhardt
Enable AOT compatibility for all libraries. Fix warnings.
701ffec 
- Enable configuration binder source generator
- The only library that can't use the ConfigBinder SG is the HeaderParsing library.
  - Blocked by dotnet/runtime#94547
@eerhardt eerhardt force-pushed the EnableConfigBinderSG branch from 23b1714 to 701ffec Compare November 14, 2023 23:40
@eerhardt eerhardt changed the base branch from main to dev November 14, 2023 23:40
@eerhardt eerhardt changed the title Initial enable configuration binder source gen Enable AOT compatibility for all libraries Nov 14, 2023
@eerhardt eerhardt marked this pull request as ready for review November 14, 2023 23:42
@dotnet-comment-bot
Copy link
Collaborator

‼️ Found issues ‼️

Project Coverage Type Expected Actual
Microsoft.AspNetCore.Diagnostics.Middleware Line 100 99.17 🔻

Full code coverage report: https://dev.azure.com/dnceng-public/public/_build/results?buildId=470141&view=codecoverage-tab

@dotnet-comment-bot
Copy link
Collaborator

‼️ Found issues ‼️

Project Coverage Type Expected Actual
Microsoft.AspNetCore.Diagnostics.Middleware Line 100 99.17 🔻

Full code coverage report: https://dev.azure.com/dnceng-public/public/_build/results?buildId=470292&view=codecoverage-tab

@RussKie
Copy link
Member

RussKie commented Nov 15, 2023

@eerhardt I suggest reducing the threshold in the csproj. The coverage report is showing 100%, but it's likely bugged (we likely need to use the newer PublishCodeCoverageResults task, but it's broken too).

RussKie
RussKie reviewed Nov 15, 2023
View reviewed changes
eng/packages/General.props Outdated Show resolved Hide resolved
...tensions.AmbientMetadata.Application/Microsoft.Extensions.AmbientMetadata.Application.csproj
@@ -5,6 +5,10 @@
<Workstream>Fundamentals</Workstream>
</PropertyGroup>

<PropertyGroup>
<EnableConfigurationBindingGenerator>true</EnableConfigurationBindingGenerator>
Copy link
Member

Choose a reason for hiding this comment

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

Is this something we could configure at the global level as opposed to doing it in each individual project?

Copy link
Member Author

Choose a reason for hiding this comment

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

I wouldn't recommend it for now. This source generator uses the experimental "Interceptors" feature in C# 12. That's why it isn't enabled by default everywhere. Plus we are only using it in 10 of the ~35 projects. So I don't think it makes sense to turn it on for all. Just like we don't turn on the Logging and Metrics source generators globally.

@dariusclay dariusclay mentioned this pull request Nov 15, 2023
Closed
@eerhardt
Copy link
Member Author

@eerhardt I suggest reducing the threshold in the csproj. The coverage report is showing 100%, but it's likely bugged (we likely need to use the newer PublishCodeCoverageResults task, but it's broken too).

I'm assuming it is this change that is confusing/breaking it:

image

I'll change MinCodeCoverage to 99 for this assembly.

@eerhardt
Copy link
Member Author

FYI - @ericstj @eiriktsarpalis @tarekgh - This is changing dotnet/extensions to use the configuration binder source generator in 9.0. No new issues were found. The only outstanding issues that I know about are:

@eerhardt
Copy link
Member Author

@joperezr @RussKie @sebastienros - any thoughts on this PR?

RussKie
RussKie approved these changes Nov 28, 2023
View reviewed changes
Copy link
Member

@RussKie RussKie left a comment

Choose a reason for hiding this comment

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

LGTM (with my minimal knowledge of the area)

@RussKie
Copy link
Member

RussKie commented Nov 28, 2023

I'm happy to merge it if you think it's ready.

@eerhardt
Copy link
Member Author

I'm happy to merge it if you think it's ready.

I'm going to run CI on it one more time just to make sure no breaks came in the meantime. Once that passes, I believe this can be merged.

@eerhardt
Copy link
Member Author

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@RussKie RussKie merged commit 322e507 into dotnet:dev Nov 28, 2023
6 checks passed
@eerhardt eerhardt deleted the EnableConfigBinderSG branch November 28, 2023 22:16
@github-actions github-actions bot locked and limited conversation to collaborators Dec 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@RussKie RussKie RussKie approved these changes

@joperezr joperezr Awaiting requested review from joperezr

@sebastienros sebastienros Awaiting requested review from sebastienros

Assignees

@eerhardt eerhardt

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@eerhardt @dotnet-comment-bot @RussKie