Updated return of matching TF and RID instructions

[rainersigwald]

Instead of returning a semicolon-delimited string, which must be parsed
on the receiving end and cannot be easily converted to an item (with
relevant metadata like which project it's associated with), return
structured data in the only way MSBuild can: as an item with metadata.

Metadata includes the closest matching TargetFramework and whether or
not the project should have TargetFramework and/or RuntimeIdentifier
stripped before calling it.

Consumed by Microsoft.Common.CurrentVersion.targets in the coordinated
change dotnet/msbuild#1866.

[rainersigwald]

cc @nguerrera, @dotnet/msbuild

[nguerrera]

Since we're changing the protocol anyway, this name is misleading. It will be false for a project with <TargetFrameworks>xyz1.0</TargetFrameworks>, i.e. using TargetFrameworks plural but only specifying one value.

Should we generalize and just give the callee project the ability to specify properties to remove to avoid having to deal with these case-by-case?

[rainersigwald]

Yes, that's a really good idea.

[rainersigwald]

Ok, I really like this in theory but I'm worried it breaks when Project A cross-targets and depends on Project B (not an Sdk project). If Project B just has the common-targets placeholder for GetTargetFrameworkProperties, it won't return back that we should strip the TF and RID global variables--but we should. Right? Or is that a scenario we don't care about?

[nguerrera]

Maybe I'm missing something, but that seems orthogonal. Today in this case we go to empty GetTargetFrameworkProperties, which won't set HasSingleTargetFramework or IsRidAgnostic.

Generally, it's not as bad to leave TargetFramework in place for non-SDK projects because they don't do anything with it. RuntimeIdentifier could be more problematic, but we have this problem today.

Instead of defaults for HasSingleTargetFramework and IsRidAgnostic, can we have a default set of properties to remove?

[rainersigwald]

Ok, that's possible, but I don't think I can get through implementing it today.

It's easy enough to make GetDesiredProperties return the right set of properties to set and to remove.

Where it's hard is removing the right properties for projects that want to short-circuit by saying "I don't cross-target, just strip the properties"--and not reevaluate with ReferringTF. We could change the protocol on ShouldQueryForProperties to instead get back an item with the project name and metadata for removed properties, basically the same way GetDesiredProperties does now. That's a harder thing to implement (especially in the default case) but not too bad.

@AndyGerlicher does that description make enough sense for you to pick this up?

[rainersigwald]

@AndyGerlicher I pushed https://github.com/rainersigwald/msbuild/tree/general-prop-set-and-unset-through-refs and https://github.com/rainersigwald/sdk/tree/general-prop-set-and-unset-through-refs with a WIP of going down this road. It almost works--but only for cross-targeting projects. It doesn't strip TF/RID from refs to single-targeting projects.

What should be done next to go down this road is to change ShouldQueryForProperties to return the same type of thing that GetDesiredProperties, except no SetDesiredProperties--we only care about UndefineProperties, which should be set for all future calls to that ProjectReferenceExistent.

[nguerrera]

I believe this will not deal well with the case where RuntimeIdentifier is specified as a global property (-r from CLI). We removed it from global properties for GetTargetFrameworkProperties, but not from ShouldQueryForProperties :(

EDIT: Nevermind, they are removed from this evaluation as well.

[rainersigwald]

Latest commit makes this compatible with old common targets, so it's not a breaking change. This can be merged, then an updated MSBuild adopted, then b0cf048 reverted.

[nguerrera]

If we're renaming the target, shouldn't we also remove TargetFramework from this part of the contract?

[nguerrera]

I want to understand more about why we can't use a generic set of properties to remove.

[rainersigwald]

Latest iteration (here and in dotnet/msbuild#1866) changes the protocol again so that a referenced project can unset properties in the initial ShouldQueryForProperties request, allowing single-targeting projects to a) build after a single query and b) specify the global properties that should be removed explicitly.

@nguerrera @livarcocc I'd love to test the coordinated change between the MSBuild repo and the SDK repo, but I can't figure out how to get the private common targets into the sdk repo for testing. I'm stuck at the moment because the common targets come in through the CLI and not a direct ref to MSBuild so I can't just update that. Can one of y'all help me work that out?

[rainersigwald]

Thanks to @livarcocc for helping me test the new targets in the SDK repo (by manually patching the downloaded CLI with the new targets after download, so it gets used for all the tests).

That revealed that this does have one major change from the current always-query approach: there's no longer an easy way to determine that the remote project's single TF is not compatible with a referencing project. I filed #1122 so we can talk about that without the legacy of design iterations in this PR.

[rainersigwald]

MSBuild triage: not pursuing this for now due to the TF-compat check and other complexity concerns.