Skip to content

Fix a typo and address remaining service reference TODO items #13364

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Aug 23, 2019
Merged

Fix a typo and address remaining service reference TODO items #13364

merged 2 commits into from
Aug 23, 2019

Conversation

dougbu
Copy link
Contributor

@dougbu dougbu commented Aug 23, 2019

  • Clean up TODOs and duplicated code in client code generation #4923
  • typo caused problems when cleaning files
  • add %(OpenApiProjectReference.GlobalPropertiesToRemove) metadata
  • address timing issues cropping up occasionally in builds using service ref features
    • avoid AfterTargets="Build"; referencing projects sometimes continue while post-build work is done
      • run after CoreBuild in inner builds and after DispatchToInnerBuilds in outer builds
      • do same in GetDocumentInsider.csproj
  • set only properties in buildMultiTargeting\Microsoft.Extensions.ApiDescription.Server.targets
    • items not evaluated early enough to reference in all cases
  • rename Microsoft.Extensions.ApiDescription.Client tasks
  • remove net461 task assembly

@dougbu dougbu requested review from pranavkm and rynowak August 23, 2019 04:51
dougbu
dougbu commented Aug 23, 2019
View reviewed changes
Copy link
Contributor Author

@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.

@rainersigwald please let us know if you have additional suggestions

src/Tools/Extensions.ApiDescription.Client/src/GetCurrentOpenApiReference.cs
@@ -9,7 +9,7 @@ namespace Microsoft.Extensions.ApiDescription.Client
/// <summary>
/// Restore <see cref="ITaskItem"/>s from given property value.
/// </summary>
public class GetCurrentItems : Task
public class GetCurrentOpenApiReference : Task
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one's for you @rynowak 😺

...xtensions.ApiDescription.Server/src/build/Microsoft.Extensions.ApiDescription.Server.targets Outdated
@@ -65,7 +65,7 @@
<!-- Unless this is an inner build or default timing is disabled, tie document retrieval into the build. -->

<Target Name="_GenerateOpenApiDocuments"
AfterTargets="Build"
AfterTargets="CoreBuild"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rainersigwald is it expected that <MSBuild/> tasks calling the Build target can return to the project (and continue) before the referenced project does AfterBuild="Build" work?

Copy link
Member

@rainersigwald rainersigwald Aug 23, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Kind of! The semantics of "what does it mean to hook onto a project's entry-point target with AfterBuild" are very muddy.

Were you seeing execution return, or was it just continuing past an error in AfterTargets="Build"? If so, that's dotnet/msbuild#3345, which also goes into some of why the meaning of this is muddy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The occasional failures looked like a race condition between referenced projects building (including document generation) and the client project starting on its BeforeTargets="BeforeCompile" work (getting filenames of generated documents). To see the output I recall, the server project must have been running two targets simultaneously -- GenerateOpenApiDocuments and OpenApiGetDocuments. In particular, I saw a <Message/> about tool execution just before or just after an <Error/> about a missing file that the command writes.

Put another way, I was seeing the occasional "client" (project with an @(OpenApiProjectReference) item) build continue before the "server" (referenced project using this code) build completed. No errors until the client called the GetOpenApiDocuments target when the server had not finished running GenerateOpenApiDocuments.

Copy link
Member

Choose a reason for hiding this comment

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

Interesting. I think something deeper is wrong in this case. Any chance you have an MSBuild binlog of a build like this? Even a build that didn't fail due to the race would be instructive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll get something to you early next week

@Pilchie Pilchie added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Aug 23, 2019
dougbu added 2 commits August 23, 2019 14:21
@dougbu
Fix a typo and address remaining service reference TODO items
9e0d87b 
- #4923
- typo caused problems when cleaning files
- add `%(OpenApiProjectReference.GlobalPropertiesToRemove)` metadata
- address timing issues cropping up occasionally in builds using service ref features
  - avoid `AfterTargets="Build"`; referencing projects sometimes continue while post-build work is done
    - run after `CoreBuild` in inner builds and after `DispatchToInnerBuilds` in outer builds
    - do same in GetDocumentInsider.csproj
- set only properties in buildMultiTargeting\Microsoft.Extensions.ApiDescription.Server.targets
  - items not evaluated early enough to reference in all cases
- rename Microsoft.Extensions.ApiDescription.Client tasks
- remove net461 task assembly
@dougbu
PR comments
c76266f 
- don't remove `$(Configuration)` or `$(Platform)` global properties
- move completely away from `AfterTargets="[Core]Build"` in these .targets and projects
@dougbu dougbu force-pushed the dougbu/service.ref.todos.4923.III branch from 48f7a01 to c76266f Compare August 23, 2019 21:21
@dougbu
Copy link
Contributor Author

dougbu commented Aug 23, 2019

Getting this in. Only failures in required checks were either flaky-but-still-running-in-main or completely unrelated to what's changing here.

@dougbu dougbu merged commit 8417429 into release/3.0 Aug 23, 2019
@ghost ghost deleted the dougbu/service.ref.todos.4923.III branch August 23, 2019 23:00
@dougbu dougbu added the tell-mode Indicates a PR which is being merged during tell-mode label Aug 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rainersigwald rainersigwald rainersigwald approved these changes

@pranavkm pranavkm Awaiting requested review from pranavkm

@rynowak rynowak Awaiting requested review from rynowak

Assignees
No one assigned
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates tell-mode Indicates a PR which is being merged during tell-mode
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dougbu @rainersigwald @Pilchie