Skip to content

[release/6.0-rc2] Update sdk to 6.0.100-rc.2.21470.55 #36783

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

Merged
merged 6 commits into from
Sep 23, 2021

Conversation

halter73
Copy link
Member

No description provided.

@halter73 halter73 added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Sep 21, 2021
@halter73 halter73 requested a review from a team as a code owner September 21, 2021 05:00
@halter73 halter73 changed the title [release/6.0rc2] Update sdk to 6.0.100-rc.2.21470.55 [release/6.0-rc2] Update sdk to 6.0.100-rc.2.21470.55 Sep 21, 2021
@dougbu
Copy link
Contributor

dougbu commented Sep 21, 2021

Actually, @wtgodbe @Pilchie is it too late for optional 6.0-rc2 infra changes❔ Or, @halter73 is there a reason we need to bump the SDK here❔

@halter73
Copy link
Member Author

Or, @halter73 is there a reason we need to bump the SDK here?

No. I thought I was supposed to since my build-ops rotation fell on a Monday and this branch is still somewhat active.

@wtgodbe
Copy link
Member

wtgodbe commented Sep 21, 2021

We do want to ingest the change to make preview features give an error by default, though I'm surprised that's coming in via the SDK update rather than the runtime update - I'll make the changes in a bit to suppress the error for the quic projects (CC @JamesNK @adityamandaleeka @Tratcher)

@wtgodbe wtgodbe enabled auto-merge (squash) September 21, 2021 16:16
@wtgodbe wtgodbe disabled auto-merge September 21, 2021 16:16
@wtgodbe
Copy link
Member

wtgodbe commented Sep 21, 2021

This is concerning:

/datadisks/disk1/workspace/_work/1/s/src/Servers/Kestrel/Kestrel/src/WebHostBuilderKestrelExtensions.cs(38,17): error CA2252: Using 'KestrelServerOptions' requires opting into preview features. See https://aka.ms/dotnet-warnings/preview-features for more information. [/datadisks/disk1/workspace/_work/1/s/src/Servers/Kestrel/Kestrel/src/Microsoft.AspNetCore.Server.Kestrel.csproj]

Same for KestrelServerImpl. I don't see these using quic, or even HttpProtocols - any idea what gives @pgovind @jeffhandley @jkotas?

@jeffhandley
Copy link
Member

Setting <EnablePreviewFeatures>true</EnablePreviewFeatures> will treat every API in the assembly as being a preview feature by generating an [assembly:RequiresPreviewFeatures] attribute. The result is then calling into anything from that assembly from a different assembly (that doesn't opt in to preview feature usage) will see these errors. I believe that's what's causing all of these errors.

@halter73 I suspect you didn't want the result of the entire assembly being treated as preview, and instead only specific APIs were intended to be annotated.

Note, if you also include <GenerateRequiresPreviewFeaturesAttribute>False</GenerateRequiresPreviewFeaturesAttribute>, then preview feature usage is enabled without generating the assembly-wide attribute, but I'm not sure that's the right fix here; I defer to @pgovind and @terrajobst.

Here's the preview of the documentation for this analyzer's behavior, in case that's helpful:
https://review.docs.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca2252?branch=pr-en-us-26149

@wtgodbe
Copy link
Member

wtgodbe commented Sep 21, 2021

So is the fix to mark the call sites with RequiresPreviewFeaturesAttribute? We don't want customers using kestrel without Http3 to have to opt in to using preview features

@jeffhandley
Copy link
Member

So is the fix to mark the call sites with RequiresPreviewFeaturesAttribute? We don't want customers using kestrel without Http3 to have to opt in to using preview features

Instead of marking the entire assembly as enabling preview features, new APIs that are Preview Features should be annotated with the attribute. Existing APIs that are going to start calling into those new APIs get tricky, and the approach depends on your intent.

        public static IWebHostBuilder UseKestrel(this IWebHostBuilder hostBuilder)
        {
            hostBuilder.UseQuic();
            return hostBuilder.ConfigureServices(services =>
            {
                // Don't override an already-configured transport
                services.TryAddSingleton<IConnectionListenerFactory, SocketTransportFactory>();

                services.AddTransient<IConfigureOptions<KestrelServerOptions>, KestrelServerOptionsSetup>();
                services.AddSingleton<IServer, KestrelServerImpl>();
            });
        }

In this code, hostBuilder.UseKestrel() is unconditionally calling hostBuilder.UseQuic(), which is a preview feature. If the caller hasn't opted into preview features, then should UseKestrel() be calling UseQuic()?

@Tratcher
Copy link
Member

hostBuilder.UseQuic() does feature detection internally and should suppress any preview warnings. Kestrels preview features are controlled elsewhere.

@jeffhandley
Copy link
Member

In that case, I recommend using a #pragma warning disable CA2252 // Preview Features -- UseQuic performs feature detection directive around that API call, and any others that are similar where you're calling a feature-detecting-preview-feature API from a non-preview-feature API. Then you can remove <EnablePreviewFeatures>true</>, returning the assembly as a whole to non-preview-feature mode.

@wtgodbe wtgodbe force-pushed the halter73/sdk-6.0-rc2 branch from 4ea814f to 7eacd8a Compare September 22, 2021 21:29
@jeffhandley
Copy link
Member

It looks like there are still some call sites that need the pragmas, but the direction here looks good to me.

@pgovind
Copy link

pgovind commented Sep 23, 2021

Just went through the thread here. +1 to Jeff's suggestion. I also just saw the latest commit adding more pragmas. It looks correct.

Copy link

@pgovind pgovind left a comment

Choose a reason for hiding this comment

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

CA2252 suppressions LGTM

@wtgodbe wtgodbe enabled auto-merge (squash) September 23, 2021 23:31
@wtgodbe wtgodbe merged commit 487f484 into release/6.0-rc2 Sep 23, 2021
@wtgodbe wtgodbe deleted the halter73/sdk-6.0-rc2 branch September 23, 2021 23:47
BrennanConroy added a commit that referenced this pull request Sep 24, 2021
* Update WiX to signed build (#36865)

- #12078

* Always download blazor-hotreload.js from app root (#36897)

The middleware that we inject always listens to the root of the app. While this might not play very nicely if the app
is running off a virtual path, listening to the root is what we do with the aspnet-browser-refresh.js script and we've had no issue
reports with it thus far.

Fixes #35555

* Improve Results.Problem and Results.ValidationProblem APIs (#36856)

* [release/6.0-rc2] Update sdk to 6.0.100-rc.2.21470.55 (#36783)

* Update sdk to 6.0.100-rc.2.21470.55

Co-authored-by: Will Godbe <[email protected]>

Co-authored-by: Doug Bunting <[email protected]>
Co-authored-by: Pranav K <[email protected]>
Co-authored-by: Safia Abdalla <[email protected]>
Co-authored-by: Stephen Halter <[email protected]>
Co-authored-by: Will Godbe <[email protected]>
Co-authored-by: Brennan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants