[release/6.0] Work around dotnet/msbuild#3274 by dougbu · Pull Request #39293 · dotnet/aspnetcore

dougbu · 2022-01-04T01:02:17Z

[release/6.0] Work around dotnet/msbuild#3274

Description

OpenAPIReference does not work for SDK generators that create C# output in a folder #38547
cherry-pick of e2707e1
handle a ApiDescription.Client code generator that uses %(OutputPath) as a directory
- see also https://stackoverflow.com/questions/48868060/can-a-task-itemgroup-glob-files
add tests for Microsoft.Extensions.ApiDescription.Client.targets
- make TemporaryCSharpProject slightly extensible
- allow project additions to TemporaryDirectory after Create()

nits:

add comment about batching w/ a code generator that uses %(OutputPath) as a directory
fix %(SourceDocument) metadata
- primarily for back-tracing in detailed / binary logs
remove useless %(OutputPathExtension) metadata; use %(Extension)
remove useless ConsoleClient test asset project

Handle symbolic links in TemporaryDirectory

e.g. on macOS /var/folders resolves to /private/var/folders

Customer impact

Without this change, customers cannot use OpenAPI code generators that generate multiple files. @darrelmiller reported #38547 for once such code generator but others exist or could exist soon-ish.

Regression

No, this problem wasn't noticed in the past due to inadequate testing i.e. we mainly focused on the NSwag code generator which generates a single file per OpenAPI document.

Risk

Very low. Using the recommended workaround for a known msbuild problem.

Verification

Both manual and automated testing. In fact, added a lot of missing integration tests covering many scenarios that were previously missed. (Previous automated tests mostly ignored our .targets files.)

Packaging changes reviewed?

N/A

- `cherry-pick` of e2707e1 Work around dotnet/msbuild#3274 - #38547 - handle a ApiDescription.Client code generator that uses `%(OutputPath)` as a directory - see also <https://stackoverflow.com/questions/48868060/can-a-task-itemgroup-glob-files> - add tests for Microsoft.Extensions.ApiDescription.Client.targets - make `TemporaryCSharpProject` slightly extensible - allow project additions to `TemporaryDirectory` after `Create()` nits: - add comment about batching w/ a code generator that uses `%(OutputPath)` as a directory - fix `%(SourceDocument)` metadata - primarily for back-tracing in detailed / binary logs - remove useless `%(OutputPathExtension)` metadata; use `%(Extension)` - remove useless `ConsoleClient` test asset project Handle symbolic links in `TemporaryDirectory` - e.g. on macOS /var/folders resolves to /private/var/folders

ghost · 2022-01-04T01:02:23Z

Hi @dougbu. If this is not a tell-mode PR, please make sure to follow the instructions laid out in the servicing process document.
Otherwise, please add tell-mode label.

dougbu · 2022-01-04T01:08:04Z

...xtensions.ApiDescription.Client/src/build/Microsoft.Extensions.ApiDescription.Client.targets

-      <!-- Otherwise, add all descendant files with the expected extension. -->
-      <TypeScriptCompile Include="@(_Directories -> '%(Identity)/**/*.ts;%(Identity)/**/*.tsx')"
-          Exclude="@(TypeScriptCompile)">
-        <SourceDocument>%(_Directories.FullPath)</SourceDocument>
-      </TypeScriptCompile>
+    <!--
+      Otherwise, add all descendant files with the expected extension. Glob into _Directories before updating
+      TypeScriptCompile or Compile items. See <https://github.com/dotnet/msbuild/issues/3274> and workaround in
+      <https://stackoverflow.com/questions/48868060/can-a-task-itemgroup-glob-files>. Unfortunately, this workaround
+      loses SourceDocument and other metadata.
+    -->
+    <PropertyGroup>
+      <_TypeScriptCompileItemsFromDirectories>@(_Directories -> '%(Identity)/**/*.ts;%(Identity)/**/*.tsx')</_TypeScriptCompileItemsFromDirectories>
+      <_CompileItemsFromDirectories>@(_Directories -> '%(Identity)/**/*$(DefaultLanguageSourceExtension)')</_CompileItemsFromDirectories>
+    </PropertyGroup>

-      <Compile Include="@(_Directories -> '%(Identity)/**/*.$(DefaultLanguageSourceExtension)')"
+    <ItemGroup>
+      <TypeScriptCompile Include="$(_TypeScriptCompileItemsFromDirectories)" Exclude="@(TypeScriptCompile)" />
+
+      <Compile Include="$(_CompileItemsFromDirectories)"
          Exclude="@(Compile)"
-          Condition="'$(DefaultLanguageSourceExtension)' != '.ts'">
-        <SourceDocument>%(_Directories.FullPath)</SourceDocument>
-      </Compile>
+          Condition="'$(DefaultLanguageSourceExtension)' != '.ts'" />

-      <FileWrites Exclude="@(FileWrites)"
-          Include="@(_Files);@(_Directories -> '%(Identity)/**/*.ts;%(Identity)/**/*.tsx;%(Identity)/**/*.$(DefaultLanguageSourceExtension)')" />
+      <FileWrites Include="@(_Files);$(_TypeScriptCompileItemsFromDirectories);$(_CompileItemsFromDirectories)"
+          Exclude="@(FileWrites)" />


These lines contain the core fix. Is it necessary to remove fixes for "nits" mentioned in PR description❔ I was hoping to avoid that extra work and lean toward keeping the branches consistent, especially in cases where the changes to src files remain narrow.

I'm cooling with keeping the nits. I think they make this code a little clearer.

dougbu · 2022-01-04T01:17:55Z

/fyi @wtgodbe since you approved #39146 for 'main'

ghost · 2022-01-04T01:18:28Z

Hi @dougbu. Please make sure you've updated the PR description to use the Shiproom Template. Also, make sure this PR is not marked as a draft and is ready-to-merge.

To learn more about how to prepare a servicing PR click here.

dougbu · 2022-01-04T17:32:47Z

@rafikiassumani-msft could you please review or have someone review this PR❔

src/Tools/Shared/TestHelpers/TemporaryCSharpProject.cs

...xtensions.ApiDescription.Client/test/Microsoft.Extensions.ApiDescription.Client.Tests.csproj

captainsafia · 2022-01-04T23:59:37Z

...xtensions.ApiDescription.Client/src/build/Microsoft.Extensions.ApiDescription.Client.targets

-          Condition="$([System.IO.File]::Exists('%(OpenApiReference.OutputPath)'))">
-        <OutputPathExtension>$([System.IO.Path]::GetExtension('%(OpenApiReference.OutputPath)'))</OutputPathExtension>
-      </_Files>
+          Condition="$([System.IO.File]::Exists('%(OpenApiReference.OutputPath)'))" />


Great cleanup here!

...xtensions.ApiDescription.Client/src/build/Microsoft.Extensions.ApiDescription.Client.targets

captainsafia · 2022-01-05T00:07:09Z

...xtensions.ApiDescription.Client/src/build/Microsoft.Extensions.ApiDescription.Client.targets

-      <!-- Otherwise, add all descendant files with the expected extension. -->
-      <TypeScriptCompile Include="@(_Directories -> '%(Identity)/**/*.ts;%(Identity)/**/*.tsx')"
-          Exclude="@(TypeScriptCompile)">
-        <SourceDocument>%(_Directories.FullPath)</SourceDocument>
-      </TypeScriptCompile>
+    <!--
+      Otherwise, add all descendant files with the expected extension. Glob into _Directories before updating
+      TypeScriptCompile or Compile items. See <https://github.com/dotnet/msbuild/issues/3274> and workaround in
+      <https://stackoverflow.com/questions/48868060/can-a-task-itemgroup-glob-files>. Unfortunately, this workaround
+      loses SourceDocument and other metadata.
+    -->
+    <PropertyGroup>
+      <_TypeScriptCompileItemsFromDirectories>@(_Directories -> '%(Identity)/**/*.ts;%(Identity)/**/*.tsx')</_TypeScriptCompileItemsFromDirectories>
+      <_CompileItemsFromDirectories>@(_Directories -> '%(Identity)/**/*$(DefaultLanguageSourceExtension)')</_CompileItemsFromDirectories>
+    </PropertyGroup>

-      <Compile Include="@(_Directories -> '%(Identity)/**/*.$(DefaultLanguageSourceExtension)')"
+    <ItemGroup>
+      <TypeScriptCompile Include="$(_TypeScriptCompileItemsFromDirectories)" Exclude="@(TypeScriptCompile)" />
+
+      <Compile Include="$(_CompileItemsFromDirectories)"
          Exclude="@(Compile)"
-          Condition="'$(DefaultLanguageSourceExtension)' != '.ts'">
-        <SourceDocument>%(_Directories.FullPath)</SourceDocument>
-      </Compile>
+          Condition="'$(DefaultLanguageSourceExtension)' != '.ts'" />

-      <FileWrites Exclude="@(FileWrites)"
-          Include="@(_Files);@(_Directories -> '%(Identity)/**/*.ts;%(Identity)/**/*.tsx;%(Identity)/**/*.$(DefaultLanguageSourceExtension)')" />
+      <FileWrites Include="@(_Files);$(_TypeScriptCompileItemsFromDirectories);$(_CompileItemsFromDirectories)"
+          Exclude="@(FileWrites)" />


I'm cooling with keeping the nits. I think they make this code a little clearer.

dougbu added the area-commandlinetools Includes: Command line tools, dotnet-dev-certs, dotnet-user-jwts, and OpenAPI label Jan 4, 2022

dougbu requested review from captainsafia, halter73 and rafikiassumani-msft January 4, 2022 01:02

dougbu requested a review from Pilchie as a code owner January 4, 2022 01:02

ghost added this to the 6.0.x milestone Jan 4, 2022

dougbu commented Jan 4, 2022

View reviewed changes

dougbu added the Servicing-consider Shiproom approval is required for the issue label Jan 4, 2022

dougbu mentioned this pull request Jan 4, 2022

OpenAPIReference does not work for SDK generators that create C# output in a folder #38547

Closed

dougbu added Servicing-approved Shiproom has approved the issue and removed Servicing-consider Shiproom approval is required for the issue labels Jan 4, 2022

captainsafia reviewed Jan 5, 2022

View reviewed changes

!fixup! Align indentation per review comments

78dbd96

captainsafia approved these changes Jan 5, 2022

View reviewed changes

dougbu merged commit e47af40 into dotnet:release/6.0 Jan 5, 2022

dougbu deleted the dougbu/api.decription.client.38547/6.0 branch January 5, 2022 22:59

ghost modified the milestones: 6.0.x, 6.0.2 Jan 5, 2022

p2pbsh mentioned this pull request Feb 21, 2022

SDK 6.0.200 breaks Blazor projects in Azure DevOps Visual Studio build task #40333

Closed

captainsafia mentioned this pull request Mar 29, 2022

Update to latest GA of M.E.ApiDescription.Server RicoSuter/NSwag#3951

Merged

captainsafia mentioned this pull request Jun 6, 2022

Update M.E.ApiDescription.Server version to 6.x release domaindrivendev/Swashbuckle.AspNetCore#2433

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comments

[release/6.0] Work around dotnet/msbuild#3274#39293

[release/6.0] Work around dotnet/msbuild#3274#39293
dougbu merged 2 commits intodotnet:release/6.0from
dougbu:dougbu/api.decription.client.38547/6.0

dougbu commented Jan 4, 2022 •

edited

Loading

Uh oh!

ghost commented Jan 4, 2022

Uh oh!

dougbu Jan 4, 2022

Uh oh!

captainsafia Jan 5, 2022

Uh oh!

dougbu commented Jan 4, 2022

Uh oh!

ghost commented Jan 4, 2022

Uh oh!

dougbu commented Jan 4, 2022

Uh oh!

Uh oh!

Uh oh!

captainsafia Jan 4, 2022

Uh oh!

Uh oh!

captainsafia Jan 5, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Comments

Conversation

dougbu commented Jan 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

[release/6.0] Work around dotnet/msbuild#3274

Description

Customer impact

Regression

Risk

Verification

Packaging changes reviewed?

Uh oh!

ghost commented Jan 4, 2022

Uh oh!

dougbu Jan 4, 2022

Choose a reason for hiding this comment

Uh oh!

captainsafia Jan 5, 2022

Choose a reason for hiding this comment

Uh oh!

dougbu commented Jan 4, 2022

Uh oh!

ghost commented Jan 4, 2022

Uh oh!

dougbu commented Jan 4, 2022

Uh oh!

Uh oh!

Uh oh!

captainsafia Jan 4, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

captainsafia Jan 5, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dougbu commented Jan 4, 2022 •

edited

Loading