search

dotnet / sdk

Core functionality needed to create .NET Core projects, that is shared between Visual Studio and CLI
https://dot.net/core
MIT License
2.71k stars 1.06k forks source link

EnablePreviewFeatures not working in 8.0.100-alpha.1.22469.4 #28016

Open Tratcher opened 2 years ago

Tratcher commented 2 years ago

Describe the bug

https://github.com/dotnet/aspnetcore/pull/44066 updated the SDK from 7.0 to 8.0.100-alpha.1.22469.4 and now EnablePreviewFeatures isn't working in tests and samples.

Exceptions (if any)

D:\github\aspnetcore\src\Servers\Kestrel\Transport.Quic\test\QuicConnectionContextTests.cs(60,50): error CA2252: Using
'ConnectAsync' requires opting into preview features. See https://aka.ms/dotnet-warnings/preview-features for more info
rmation. [D:\github\aspnetcore\src\Servers\Kestrel\Transport.Quic\test\Microsoft.AspNetCore.Server.Kestrel.Transport.Qu
ic.Tests.csproj]
dougbu commented 2 years ago

@marcpopMSFT @dsplaisted @jaredpar what's happening here and is there a workaround❔ Seeing the same issue w/ the 8.0.100-alpha.1.22478.15 SDK.

The odd thing is our Microsoft.AspNetCore.Server.Kestrel.Core package uses <EnablePreviewFeatures>true</EnablePreviewFeatures> too but is compiling cleanly. @JamesNK does that project still use preview features❔ (Both projects were updated in dotnet/aspnetcore@b120cefca18)

JamesNK commented 2 years ago

Yes. Some APIs are marked as preview.

dsplaisted commented 2 years ago

@jeffhandley Is this the same issue as #28049? In this case it looks like switching to the .NET 8 SDK is breaking some stuff related to preview features in ASP.NET.

@Tratcher @dougbu Do the projects that are getting broken target .NET 7 or .NET 8?

jaredpar commented 2 years ago

@terrajobst

If ConnectAsync is marked preview then I would expect this error to happen. I am surprised that moving to a 8.0.0 SDK, and doing nothing else, would trigger such a warning though. That would imply an API was not preview in 7.0.something but was in 8.0.0 alpha which seems wrong.

JamesNK commented 2 years ago

This line that generates the error: https://github.com/dotnet/aspnetcore/blob/c226a4b7e49a7740a27f0cfa6f517d120c57aafd/src/Servers/Kestrel/Transport.Quic/test/QuicConnectionContextTests.cs#L60

I think QuicConnection.ConnectAsync is preview because the assembly is marked with RequiresPreviewFeaturesAttribute. It's present in main and release/7.0.

jaredpar commented 2 years ago

I think QuicConnection.ConnectAsync is preview because the assembly is marked with RequiresPreviewFeaturesAttribute. It's present in main and release/7.0.

Am I understanding that the assembly shipped in 7.0 without the [ReqiresPreviewFeatures] attribute but it got added for 8.0? If so that seems wrong. It would mean we downgraded APIs from non-preview to preview right?

dougbu commented 2 years ago

Do the projects that are getting broken target .NET 7 or .NET 8?

net7.0; we haven't made any TFM changes in main yet. (And won't until runtime changes their targets.)

https://github.com/dotnet/sdk/issues/28049

That issue mentions the "workaround" of including <GenerateRequiresPreviewFeaturesAttribute>true</GenerateRequiresPreviewFeaturesAttribute> in the project. This seems counter-intuitive, but could it help us❔

https://github.com/dotnet/designs/blob/main/accepted/2021/preview-features/preview-features.md#meaning-of-property-in-multi-targeted-projects

This section seems fine at first glance but would be far more understandable if a multi-targeting project could use <EnablePreviewFeaturesForNet7.0>true</EnablePreviewFeaturesForNet7.0> or similar. Even <EnablePreviewFeatures Condition=" '$(TargetFramework)' == 'net7.0' " would be more understandable.

Am I understanding that the assembly shipped in 7.0 without the [ReqiresPreviewFeatures] attribute but it got added for 8.0?

More likely the assembly included [RequiresPreviewFeatures] in 7.0.0 but that hasn't been removed from dotnet/runtime's main branch yet. @jeffhandley is that right❔

JamesNK commented 2 years ago

I think QuicConnection.ConnectAsync is preview because the assembly is marked with RequiresPreviewFeaturesAttribute. It's present in main and release/7.0.

Am I understanding that the assembly shipped in 7.0 without the [ReqiresPreviewFeatures] attribute but it got added for 8.0? If so that seems wrong. It would mean we downgraded APIs from non-preview to preview right?

The assembly is marked with RequiresPreviewFeaturesAttribute in main and release/7.0:

https://github.com/dotnet/runtime/blob/release/7.0/src/libraries/System.Net.Quic/src/System.Net.Quic.csproj#L17

cc @karelz

JamesNK commented 2 years ago

.NET 7 app with QuicConnection.ConnectAsync:

image

jaredpar commented 2 years ago

The assembly is marked with RequiresPreviewFeaturesAttribute in main and release/7.0:

That is sensible but it means I can't explain how moving to a new toolset, and doing nothing else, would cause this error. My mental model would say it needs to be paired with a change like moving to a new target framework.

dougbu commented 2 years ago

My mental model would say it needs to be paired with a change like moving to a new target framework.

According to https://github.com/dotnet/designs/blob/main/accepted/2021/preview-features/preview-features.md#meaning-of-property-in-multi-targeted-projects, your mental model is upside down and backwards :grin: 😬

The generality of the preview feature msbuild property names is questionable at best. Maybe this is only an issue w/in our repos while we're in the midst of adding a new TFM and (I hope) removing [RequiresPreviewFeatures] from the previous release❔

jaredpar commented 2 years ago

According to https://github.com/dotnet/designs/blob/main/accepted/2021/preview-features/preview-features.md#meaning-of-property-in-multi-targeted-projects, your mental model is upside down and backwards 😁 😬

Well at least that explains what we're seeing.

dougbu commented 2 years ago

New news: <GenerateRequiresPreviewFeaturesAttribute>true</GenerateRequiresPreviewFeaturesAttribute> works. Didn't use it everywhere in my first attempt but that at least found other projects to change.

I found a couple of projects (src\Servers\Kestrel\test\Interop.FunctionalTests\Interop.FunctionalTests.csproj and src\Servers\Kestrel\samples\Http3SampleApp\Http3SampleApp.csproj, /fyi @JamesNK) that don't need that setting for some reason. Might no longer need <EnablePreviewFeatures>true</EnablePreviewFeatures>❔

nagilson commented 2 years ago

@dougbu Thanks for following up on this -- I see your PR was merged. May you please confirm if this is resolved and we can close the issue? Looks like we should close https://github.com/dotnet/sdk/issues/28049 as well?

dougbu commented 2 years ago

@nagilson the requirement to use <EnablePreviewFeatures>true</EnablePreviewFeatures> when building for a non-default TFM is a mess and I have no understanding of why things work that way. Read the design document and that didn't really help. Suggest this is going to trip up many external customers. To me, that means the issue should be addressed rather than papered over as we're doing now.

nagilson commented 2 years ago

I see; sorry, I didn't understand the full context of the issue until now. We'll discuss this some more on our end.

marcpopMSFT commented 2 years ago

Per the code, you have to be targeting the latest TFM for any of this to work. If you do, EnablePreviewFeatures will set the LangVersion to preview and GenerateRequiresPreviewFeaturesAttribute will set the preview attribute (assuming you also set the other preview property). I'm not sure how this is supposed to work and my team didn't create this feature.

@terrajobst how should this feature work? Should EnablePreviewFeatures also set GenerateRequiresPreviewFeaturesAttribute? Should we support it on older TFMs or only on the latest?

curt-w commented 1 year ago

I just updated to Visual Studio release v17.4.0 and this bug is affecting all projects as I was running net6.0.

As noted in this thread, seems only to work with latest version. Since NET 7.0 is released now, updating my framework reference resolved issues. May not be a solution for everyone though.

emex-yakovlev commented 1 year ago

I just updated to Visual Studio release v17.4.0 and this bug is affecting all projects as I was running net6.0...

Editing the project file helped me

<Project Sdk="Microsoft.NET.Sdk.Web">

  <PropertyGroup>
    <TargetFramework>net6.0</TargetFramework>
    <LangVersion>Preview</LangVersion>
    <EnablePreviewFeatures>True</EnablePreviewFeatures>
    <GenerateRequiresPreviewFeaturesAttribute>True</GenerateRequiresPreviewFeaturesAttribute>
    ..
  </PropertyGroup>
bjornenalfa commented 11 months ago

Since Visual studio 17.8.0 force installed dotnet 8 I could no longer publish projects. It seems this issue was never fixed before the version 8 release. Luckily <GenerateRequiresPreviewFeaturesAttribute>True</GenerateRequiresPreviewFeaturesAttribute> works for now, but will this be fixed? What is the state of the preview features system?