Pass generators to CSC during component discovery #25292

chsienki · 2020-08-26T23:22:40Z

Pass the @(Analyzer) item group to CSC during component discovery
Disable execution of analyzers

Addresses #24526

chsienki · 2020-08-26T23:23:01Z

This will presumably need tests etc, but I'm pretty unfamiliar with the ASP.Net repo

jaredpar · 2020-08-26T23:25:41Z

...r/Microsoft.NET.Sdk.Razor/src/build/netstandard2.0/Microsoft.NET.Sdk.Razor.Component.targets

@@ -231,6 +232,7 @@ Copyright (c) .NET Foundation. All rights reserved.
         ResponseFiles="$(CompilerResponseFile)"
         RuntimeMetadataVersion="$(RuntimeMetadataVersion)"
         SharedCompilationId="$(SharedCompilationId)"
+         SkipAnalyzers="true"


Note: this will only work once they are using the RC1 SDK correct? This change in our targets didn't make P8 IIRC

I'm not 100% sure, but I just grabbed the preview8 SDK bits and it seems to be there in the targets.

Just verified myself and yes it made .NET P8.

DamianEdwards · 2020-08-26T23:36:03Z

@mkArtakMSFT @pranavkm

Pilchie · 2020-08-26T23:55:52Z

Thanks @chsienki. Should we consider this for RC1 (in which case a retarget to release/5.0 would be in order)?

pranavkm · 2020-08-27T00:44:17Z

@chsienki could you rebase this change (or make this change) in the release/5.0 branch? We'd want to take this for the 5.0 release. master targets 6.0.

Just noticed @Pilchie's comment stating the same. Retargeting this PR would bring in random changes from the master branch, so rebasing would be the way to go.

chsienki · 2020-08-27T01:10:08Z

Rebased and re-targeted onto release/5.0

chsienki · 2020-08-27T01:46:51Z

Hmm, looks like 5.0 is on an earlier SDK version; we don't have access to SkipAnalyzers, which means Analyzers will get run as part of Component Discovery.

Unfortunately that means users would see double warnings from analyzers, as they'll be emitted as part of discovery and then again during compilation. Generally VS seems to hide the duplicates when they occur at the same location, which is true for general compiler warnings as they're #lined back to the original file. Analyzer warnings though seem to show duplicated because the .g. files aren't identical between the two phases.

dougbu · 2020-08-27T02:22:18Z

Hmm, looks like 5.0 is on an earlier SDK version

Yes, we ran into too many problems attempting to update the SDK and gave up. A nicer way to say it is that we deferred the work to early in RC2.

Unfortunately that means users would see double warnings from analyzers

Would this affect anyone outside this repo❔ If it's just those changing code in VS, I suggest that's not great but is livable. CI builds run w/ warn-as-error in any case.

Which reminds me: The analyzers will run in command line (dotnet msbuild) builds, correct❔

dougbu · 2020-08-27T02:26:35Z

BTW build should be using the 5.0.100-rc.1.20379.10 SDK in our release/5.0 and master branches. Surprised that doesn't contain fixes that made it into the preview.8 SDK.

pranavkm · 2020-08-27T13:40:03Z

Would this affect anyone outside this repo❔ If it's just those changing code in VS, I suggest that's not great but is livable. CI builds run w/ warn-as-error in any case.

Yes, those targets are in the SDK and will affect RC1 users.

NTaylorMullen · 2020-08-27T19:56:33Z

...r/Microsoft.NET.Sdk.Razor/src/build/netstandard2.0/Microsoft.NET.Sdk.Razor.Component.targets

@@ -187,6 +187,7 @@ Copyright (c) .NET Foundation. All rights reserved.
         AddModules="@(AddModules)"
         AdditionalFiles="@(AdditionalFiles)"
         AllowUnsafeBlocks="$(AllowUnsafeBlocks)"
+         Analyzers="@(Analyzer)"


@pranavkm couldn't we just remove the ANalyzers from this phase wholesale?

Source generators are also considered analyzers, and they need to happen in this phase.

Yep, thats the problem, and why we added the new SkipAnalyzers argument.

Equally we're going to have this issue if a generator emits a warning too, so we might need to re-think the strategy a little bit? I wonder if we should just disable all wanings (-warn 0) for the component discovery phase? That way any warnings would only show up in the second phase, and errors in the first phase would stop the second running anyway.

I'll give bumping up the SDK another try. If we get it updated to the latest and greatest, perhaps we get this change in as-is.

I wonder if we should just disable all wanings (-warn 0) for the component discovery phase? That way any warnings would only show up in the second phase, and errors in the first phase would stop the second running anyway.

ya I think that'd be best

Ok, I've disabled warnings and removed the SkipAnalyzers param. Means that phase might be a little slower until you upgrade the SDK and can use it again, but everything should work as expected.

Should we file a follow up issue to move to SkipAnalyzers at some point in the future?

Yes, I'll open an issue to track.

mkArtakMSFT · 2020-08-28T04:46:14Z

@chsienki are you able to merge this change or do you need help with it?

chsienki · 2020-08-28T05:18:43Z

I don't have permission to do the actual merge :)

mkArtakMSFT · 2020-08-28T15:33:51Z

@pranavkm, @jaredpar are additional feedback or are you happy with the change?

pranavkm

@chsienki could you file the follow up issue?

chsienki · 2020-08-28T17:48:21Z

Filled #25365 to follow up.

mkArtakMSFT · 2020-08-28T18:32:41Z

Thanks @chsienki !

EdCharbeneau · 2021-03-25T00:35:40Z

I'm having a similar issue to this and #24526

Where is the proper place to report the issue? @chsienki @jaredpar
EdCharbeneau/BlazorSize.CssGenerator#1

ghost · 2021-03-25T00:35:42Z

Hi @EdCharbeneau. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

ghost added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Aug 26, 2020

chsienki mentioned this pull request Aug 26, 2020

Razor components break source generators #24526

Closed

jaredpar reviewed Aug 26, 2020

View reviewed changes

Pass generators to CSC during component discovery

d959084

chsienki force-pushed the razor_source_generators branch from 2d845e4 to d959084 Compare August 27, 2020 01:09

chsienki requested review from BrennanConroy, dougbu, halter73, jkotalik, SteveSandersonMS, Tratcher and a team as code owners August 27, 2020 01:09

chsienki changed the base branch from master to release/5.0 August 27, 2020 01:09

chsienki removed request for a team, halter73, SteveSandersonMS, Tratcher, dougbu, BrennanConroy and jkotalik August 27, 2020 01:09

mkArtakMSFT requested review from pranavkm and NTaylorMullen August 27, 2020 04:09

NTaylorMullen reviewed Aug 27, 2020

View reviewed changes

chsienki added 2 commits August 27, 2020 15:57

Disable compiler warnings during component discovery phase

866df80

Remove unsupported SkipAnalyzers param

aa51b8c

NTaylorMullen approved these changes Aug 27, 2020

View reviewed changes

pranavkm approved these changes Aug 28, 2020

View reviewed changes

chsienki mentioned this pull request Aug 28, 2020

Razor component discovery should use 'SkipAnalyzers' #25365

Closed

mkArtakMSFT merged commit 4df35c7 into release/5.0 Aug 28, 2020

mkArtakMSFT deleted the razor_source_generators branch August 28, 2020 18:32

mkArtakMSFT linked an issue Aug 28, 2020 that may be closed by this pull request

Razor components break source generators #24526

Closed

Pass generators to CSC during component discovery #25292

Pass generators to CSC during component discovery #25292

Uh oh!

Conversation

chsienki commented Aug 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chsienki commented Aug 26, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DamianEdwards commented Aug 26, 2020

Uh oh!

Pilchie commented Aug 26, 2020

Uh oh!

pranavkm commented Aug 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chsienki commented Aug 27, 2020

Uh oh!

chsienki commented Aug 27, 2020

Uh oh!

dougbu commented Aug 27, 2020

Uh oh!

dougbu commented Aug 27, 2020

Uh oh!

pranavkm commented Aug 27, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mkArtakMSFT commented Aug 28, 2020

Uh oh!

chsienki commented Aug 28, 2020

Uh oh!

mkArtakMSFT commented Aug 28, 2020

Uh oh!

pranavkm left a comment

Choose a reason for hiding this comment

Uh oh!

chsienki commented Aug 28, 2020

Uh oh!

mkArtakMSFT commented Aug 28, 2020

Uh oh!

EdCharbeneau commented Mar 25, 2021

Uh oh!

ghost commented Mar 25, 2021

Uh oh!

Uh oh!

chsienki commented Aug 26, 2020 •

edited

Loading

pranavkm commented Aug 27, 2020 •

edited

Loading