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

Handle obsolete InterceptorsPreview with .NET 8 RC2 SDK #92247

Closed
wants to merge 3 commits into from

Conversation

mthalman
Copy link
Member

@mthalman mthalman commented Sep 18, 2023

When building the runtime repo using the latest build of .NET 8 RC2 SDK, it produces the following error:

/vmr/src/runtime/artifacts/source-build/self/src/src/libraries/Microsoft.Extensions.Logging.Configuration/src/Microsoft.Extensions.Configuration.Binder.SourceGeneration/Microsoft.Extensions.Configuration.Binder.SourceGeneration.ConfigurationBindingGenerator/BindingExtensions.g.cs(33,10): error CS9137: The 'interceptors' experimental feature is not enabled in this namespace. Add '<InterceptorsPreviewNamespaces>$(InterceptorsPreviewNamespaces);Microsoft.Extensions.Configuration.Binder.SourceGeneration</InterceptorsPreviewNamespaces>' to your project. [/vmr/src/runtime/artifacts/source-build/self/src/src/libraries/Microsoft.Extensions.Logging.Configuration/src/Microsoft.Extensions.Logging.Configuration.csproj::TargetFramework=net8.0]

This is due to a new change from Roslyn: dotnet/roslyn#69848. It replaces the usage of the InterceptorsPreview feature with an InterceptorsPreviewNamespaces property that needs to specify the applicable namespace.

The use of InterceptorsPreview in the project is still needed in order to work with the RC1 SDK. Both need to be specified as support for both versions of the SDK is required to support source-build scenarios. A separate issue will be logged as follow-up to remove the obsolete InterceptorsPreview feature usage.

This change is needed for RC2 release.

@ghost
Copy link

ghost commented Sep 18, 2023

Tagging subscribers to this area: @dotnet/area-extensions-configuration
See info in area-owners.md if you want to be subscribed.

Issue Details

When building the runtime repo using the latest build of .NET 8 RC2 SDK, it produces the following error:

/vmr/src/runtime/artifacts/source-build/self/src/src/libraries/Microsoft.Extensions.Logging.Configuration/src/Microsoft.Extensions.Configuration.Binder.SourceGeneration/Microsoft.Extensions.Configuration.Binder.SourceGeneration.ConfigurationBindingGenerator/BindingExtensions.g.cs(33,10): error CS9137: The 'interceptors' experimental feature is not enabled in this namespace. Add '<InterceptorsPreviewNamespaces>$(InterceptorsPreviewNamespaces);Microsoft.Extensions.Configuration.Binder.SourceGeneration</InterceptorsPreviewNamespaces>' to your project. [/vmr/src/runtime/artifacts/source-build/self/src/src/libraries/Microsoft.Extensions.Logging.Configuration/src/Microsoft.Extensions.Logging.Configuration.csproj::TargetFramework=net8.0]

This is due to a new change from Roslyn: dotnet/roslyn#69848. It replaces the usage of the InterceptorsPreview feature with an InterceptorsPreviewNamespaces property that needs to specify the applicable namespace.

The use of InterceptorsPreview in the project is still needed in order to work with the RC1 SDK. Both need to be specified as support for both versions of the SDK is required to support source-build scenarios. A separate issue will be logged as follow-up to remove the obsolete InterceptorsPreview feature usage.

Author: mthalman
Assignees: mthalman
Labels:

area-Extensions-Configuration

Milestone: -

@tarekgh tarekgh added Servicing-consider Issue for next servicing release review source-generator Indicates an issue with a source generator feature labels Sep 18, 2023
@tarekgh
Copy link
Member

tarekgh commented Sep 18, 2023

@mthalman isn't the SDK and the configuration binder packages already enable InterceptorsPreviewNamespaces when specifying EnableConfigurationBindingGenerator in the project?

https://github.com/dotnet/sdk/blob/96b9f735229a3b136c15bf8b47d5e9c0ca2cc1a4/src/WebSdk/ProjectSystem/Targets/Microsoft.NET.Sdk.Web.ProjectSystem.targets#L66

<InterceptorsPreviewNamespaces>$(InterceptorsPreviewNamespaces);Microsoft.Extensions.Configuration.Binder.SourceGeneration</InterceptorsPreviewNamespaces>

or non of these targets is executed in the failing cases?

Why do we need to add Microsoft.AspNetCore.Http.Generated in these projects?

CC @layomia @ericstj

Copy link
Member

@carlossanlop carlossanlop left a comment

Choose a reason for hiding this comment

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

@mthalman thanks for sending this change. I had also fixed it in the dotnet/roslyn dependency flow PR, but it's slightly differently: 8013838

Unfortunately, another CI failure showed up, so I suppose the value I selected for InterceptorsPreviewNamespaces was not the right one? #92150 (comment)

Anyway, if the CI is green in this PR, we can merge it before the roslyn deps flow and I can revert my change in that PR.

Tentatively approving.

cc @ericstj @layomia @tarekgh

@carlossanlop
Copy link
Member

Also - Marking this as servicing-approved as it is an infra change (tell-mode).

@carlossanlop carlossanlop added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Sep 18, 2023
@carlossanlop carlossanlop added this to the 8.0.0 milestone Sep 18, 2023
<Features>$(Features);InterceptorsPreview</Features>
<InterceptorsPreviewNamespaces>$(InterceptorsPreviewNamespaces);Microsoft.AspNetCore.Http.Generated</InterceptorsPreviewNamespaces>
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 think this is the right namespace for this repo. Shouldn't we use the Configuration namespace, like Carlos's PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

Whoops. Copy-paste error after making similar changes in aspnetcore.

@ericstj
Copy link
Member

ericstj commented Sep 18, 2023

The use of InterceptorsPreview in the project is still needed in order to work with the RC1 SDK

Ahh - source build is a good case. @carlossanlop this was what I was guessing at in our chat earlier when trying to think of something that might need to use the old SDK on the latest codebase. Source build bootstrapping.

@carlossanlop carlossanlop added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Sep 18, 2023
@carlossanlop
Copy link
Member

I might be able to address this problem in the the two dotnet/roslyn deps flow PRs (main and 8.0), thanks to @ericstj help. If the CI looks green, I'll merge those ones, and we can close this (I copied the TODO comment there).

@mthalman
Copy link
Member Author

isn't the SDK and the configuration binder packages already enable InterceptorsPreviewNamespaces when specifying EnableConfigurationBindingGenerator in the project?

@tarekgh - The targets file which implements that is not in scope for these other projects.

@carlossanlop
Copy link
Member

The deps flow PR has the issue handled, so I think it's safe to close this PR, @mthalman : #92149

Unfortunately, fixing it uncovered a test failure in Microsoft.Extensions.Configuration.Binder. We should take care of it in the deps flow PR too.

@mthalman mthalman deleted the interceptors branch September 20, 2023 19:37
@ghost ghost locked as resolved and limited conversation to collaborators Oct 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Extensions-Configuration NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) Servicing-approved Approved for servicing release source-generator Indicates an issue with a source generator feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants