Skip to content

Traversal: Have GetTargetPath return collected build outputs#338

Merged
jeffkl merged 3 commits intomicrosoft:mainfrom
ViktorHofer:patch-5
Feb 17, 2022
Merged

Traversal: Have GetTargetPath return collected build outputs#338
jeffkl merged 3 commits intomicrosoft:mainfrom
ViktorHofer:patch-5

Conversation

@ViktorHofer
Copy link
Copy Markdown
Member

1c02e77 I made the GetTargetPath target for traversal projects return nothing. This is actually wrong as msbuild projects are expected to return the same data in the Build and the GetTargetPath target.
If those two targets aren't in sync, a behavior difference is observable when a traversal project is P2Pd and the dependent project reads from the CollectedBuildOutput (which is returned by the ResolveProjectReferences target). If the BuildProjectReferences=true flag is passed in, the output in the ResolveP2P is currently empty vs. not empty during a normal build without the flag set.

I tested this locally and am also submitting a PR into dotnet/runtime right now which depends on this behavior (currently via the AfterTargets extension point of the Traversal project). Would be great if we could get this into main here so that I can remove the custom implementation in dotnet/runtime.

cc @jeffkl

microsoft@1c02e77 I made the GetTargetPath target for traversal projects return nothing. This is actually wrong as msbuild projects are expected to return the same data in the Build and the GetTargetPath target.
If those two targets aren't in sync, a behavior difference is observable when a traversal project is P2Pd and the dependent project reads from the `CollectedBuildOutput` (which is returned by the `ResolveProjectReferences` target). If the `BuildProjectReferences=true` flag is passed in, the output in the ResolveP2P is currently empty vs. not empty during a normal build without the flag set.
@jeffkl
Copy link
Copy Markdown
Contributor

jeffkl commented Feb 15, 2022

@ViktorHofer this is super cool! Any chance you have time to write some unit tests? If not, I should have some time today to do it and push to your branch.

@ViktorHofer
Copy link
Copy Markdown
Member Author

To be honest I still haven't familiarized myself with testing msbuild components in neither this repo nor msbuild nor the sdk. At some I definitely want to close that gap but at the moment I would prefer to not spend time on that as I have little time at the moment. If you don't mind Jeff, it would be great if you could add them.

@jeffkl
Copy link
Copy Markdown
Contributor

jeffkl commented Feb 15, 2022

To be honest I still haven't familiarized myself with testing msbuild components in neither this repo nor msbuild nor the sdk

No worries, this is definitely the kind of area where the unit testing is a bit of a unicorn since we're exercising MSBuild logic. That's why I offered, I'll push a commit later today.

@jeffkl jeffkl added the Feature Request New feature or request label Feb 15, 2022
@jeffkl jeffkl changed the title Traversal: Keep GetTargetPath in sync with Build Traversal: Have GetTargetPath return collected build outputs Feb 15, 2022
@jeffkl jeffkl force-pushed the patch-5 branch 7 times, most recently from b848a40 to e2ea79c Compare February 16, 2022 18:59
@jeffkl jeffkl merged commit ef97ac8 into microsoft:main Feb 17, 2022
@ViktorHofer ViktorHofer deleted the patch-5 branch February 17, 2022 19:42
@ViktorHofer
Copy link
Copy Markdown
Member Author

Awesome. Can you please push a new version to nuget? That would be fantastic.

@jeffkl
Copy link
Copy Markdown
Contributor

jeffkl commented Feb 17, 2022

I am actively working on releasing new bits, the official build is failing with some signing issues so I'm retrying...

https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=5768594&view=logs&j=141f5c4f-a843-5a00-b45d-c47e8c39a157

@ViktorHofer
Copy link
Copy Markdown
Member Author

In case it helps, arcade repos like dotnet/runtime use the version 3 of the MicroBuildSigningPlugin task:https://github.com/dotnet/arcade/blob/78eaf78761027d225030be2b28aaf4e8bf392929/eng/common/templates/job/job.yml#L111

Traversal projects do not build an assembly so dependent projects shouldn't get a path to the target. Override the GetTargetPath to do nothing.
-->
<Target Name="GetTargetPath" />
<Target Name="GetTargetPath"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@ViktorHofer I think GetTargetPath isn't available for a multi targeting build. So if one of the project references is multing-targeting (e.g, <TargetFrameworks>net9.0;net10</TargetFrameworks>), this would fail because the project doesn't have the target defined. What should be the right behavior here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Feature Request New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants