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

Add support for packaging analyzers into ref-pack #33977

Merged
merged 4 commits into from
Jul 2, 2021

Conversation

ericstj
Copy link
Member

@ericstj ericstj commented Jun 30, 2021

Related: dotnet/runtime#54687
Spec: https://github.com/dotnet/designs/blob/main/accepted/2021/InboxSourceGenerators.md

We'll start flowing source-generators through the transport package in dotnet/runtime#54950. This will pick them up and include them in the ASP.NET refpack.

The framework list change is a port of dotnet/arcade#7561.

@ericstj ericstj requested review from Pilchie and a team as code owners June 30, 2021 19:30
@ghost ghost added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Jun 30, 2021
Co-authored-by: Doug Bunting <6431421+dougbu@users.noreply.github.com>
@dougbu
Copy link
Member

dougbu commented Jun 30, 2021

Separately, will the CI actually validate the new code path prior to dotnet/runtime#54950 getting in and a new runtime build flowing into this repo❔

@ericstj
Copy link
Member Author

ericstj commented Jun 30, 2021

Separately, will the CI actually validate the new code path prior to dotnet/runtime#54950 getting in and a new runtime build flowing into this repo❔

Not at the moment (it's a feature that lets the work remain decoupled 😄). Do you have a place where you test that FrameworkList task?

@dougbu
Copy link
Member

dougbu commented Jun 30, 2021

Do you have a place where you test that FrameworkList task?

Yes, please see https://github.com/dotnet/aspnetcore/blob/main/src/Framework/test/TargetingPackTests.cs#L297-L393

@ericstj
Copy link
Member Author

ericstj commented Jun 30, 2021

Those tests appear to be testing the product content, not running the task itself with test-inputs, is that correct?

In either case I should update those tests so that they don't break when the actual transport package arrives. Is there any other place where you'd want this to be tested in lieu of the transport package?

@dougbu
Copy link
Member

dougbu commented Jun 30, 2021

Those tests appear to be testing the product content, not running the task itself with test-inputs, is that correct?

Yes. Our RepoTasks project has no associated unit tests, partially because later stages of the build exercise most of the tasks pretty completely. We're adding an untested code path here though the same code may already be exercised elsewhere.

In either case I should update those tests so that they don't break when the actual transport package arrives. Is there any other place where you'd want this to be tested in lieu of the transport package?

No that seems sufficient. I like the thought of not only being ready in advance of whatever Maestro++ PR brings in your dotnet/runtime#54950 work but also doing more than just ignoring the new files listed in the framework list.

@dougbu
Copy link
Member

dougbu commented Jul 1, 2021

I don't see any failures clearly tied to your changes but would rather merge on green.

Most test failures look like transient problems but the Reset_PriorOSVersions_NotSupported() failure on Windows is a known issue (see #34003). Fix (see #34004) should get in in about an hour if other flakiness doesn't hurt the CI checks. Then you'll be able to /azp run to get this validating (also hopefully successfully).

@dougbu
Copy link
Member

dougbu commented Jul 1, 2021

Separately I saw you merged dotnet/runtime#54950 and fix should have affected Microsoft.AspNetCore.Internal.Transport and Microsoft.NETCore.App.Ref v6.0.0-preview.7.21351.5 (from build https://dev.azure.com/dnceng/internal/_build/results?buildId=1215933&view=results) or newer. However, while System.Text.Json.SourceGeneration.dll was in Microsoft.NETCore.App.Ref, Microsoft.Extensions.Logging.Generators.dll was not in Microsoft.AspNetCore.Internal.Transport. The transport package still lacks an analyzers/ folder. Is something more needed before that shows up❔

@ericstj
Copy link
Member Author

ericstj commented Jul 1, 2021

The transport package still lacks an analyzers/ folder. Is something more needed before that shows up❔

Good call. Let me fix that. My test used a manual mockup on the version ASP.NET is using today.

@ericstj
Copy link
Member Author

ericstj commented Jul 1, 2021

/azp run aspnetcore-ci

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ericstj
Copy link
Member Author

ericstj commented Jul 2, 2021

Seems to be passing now @dougbu. Also fix should be in here: dotnet/runtime#55042

Copy link
Member

@dougbu dougbu left a comment

Choose a reason for hiding this comment

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

Should have done this hours ago :shipit:

@dougbu dougbu merged commit 640b77f into dotnet:main Jul 2, 2021
@ghost ghost added this to the 6.0-preview7 milestone Jul 2, 2021
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.

2 participants