Skip to content

Conversation

@dfederm
Copy link
Contributor

@dfederm dfederm commented Aug 14, 2023

Fixes #9117

For project cache plugins to only partially handle a build request, it makes sense that it proxy some targets back to the original targets. For example, in VS the build request has:

"Build"
"BuiltProjectOutputGroup"
"BuiltProjectOutputGroupDependencies"
"DebugSymbolsProjectOutputGroup"
"DebugSymbolsProjectOutputGroupDependencies"
"DocumentationProjectOutputGroup"
"DocumentationProjectOutputGroupDependencies"
"SatelliteDllsProjectOutputGroup"
"SatelliteDllsProjectOutputGroupDependencies"
"SGenFilesOutputGroup"
"SGenFilesOutputGroupDependencies"

"Build" is the only relevant one that a plugin would want to handle, while the rest are "information gathering" targets which should just be passed through.

This change fixes an exception that gets thrown when attempting to proxy targets back to themselves.

@dfederm
Copy link
Contributor Author

dfederm commented Aug 14, 2023

An alternative idea would actually be to change the scenario a little bit. Instead of the plugin being expected to provide proxy targets for all targets in the request, it could just return the ones it did something about and the rest are implicitly "passed through". Today the proxy targets replace the targets in the request, so this would be more of a merge.

Example:
Original request: Build, BuiltProjectOutputGroup, BuiltProjectOutputGroupDependencies, ...
Cache plugin returns proxy targets Build -> GetTargetPath, and nothing more.
New request: GetTargetPath, BuiltProjectOutputGroup, BuiltProjectOutputGroupDependencies, ...

Here's what the alternate implementation would look like: main...dfederm:msbuild:proxy-targets-fill-missing

I actually kinda like that a bit better since the plugin cannot drop targets entirely, making the caller confused about results for a target they requested being just plain missing.

Thoughts?

@JanKrivanek
Copy link
Member

The second approach (relacing just the proxied targets) looks more mentaly digestable to me :-) - so from maintainability point of view I'd vote for replacing the PR with that one

@dfederm
Copy link
Contributor Author

dfederm commented Aug 15, 2023

Superseded by #9130

@dfederm dfederm closed this Aug 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Msbuild crashes when using cache extension with proxy targets pointing to original targets

2 participants