Skip to content
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.

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

Support gathering additional arbitrary properties from referenced projects #5994

Merged

Conversation

@dsplaisted
Copy link
Member

@dsplaisted dsplaisted commented Dec 31, 2020

This allows additional properties to be gathered from project references via the project reference protocol. This is in order to support generating an error when a self-contained Executable references a non-SelfContained Executable, or vice versa, as described in dotnet/sdk#15117.

The referenced project can declare what additional properties should be gathered with AdditionalTargetFrameworkInfoProperty items:

  <ItemGroup>
    <AdditionalTargetFrameworkInfoProperty Include="SelfContained"/>
    <AdditionalTargetFrameworkInfoProperty Include="_IsExecutable"/>
  </ItemGroup>

These items will then be included in the AdditionalPropertiesFromProject metadata on the _MSBuildProjectReferenceExistent items. This value will have PropertyName=PropertyValue combinations joined by semicolons, and those sets of properties for the different TargetFramework values will be joined by double semicolons. For example, a project multitargeted to two TargetFrameworks may have the following for the AdditionalPropertiesFromProject metadata:

SelfContained=true;_IsExecutable=true;;SelfContained=false;_IsExecutable=true

Finding the right set of properties from the referenced project will required looking up the index of the NearestTargetFramework in the TargetFrameworks, and using that index to select the set of properties in the AdditionalPropertiesFromProject metadata. For example:

string nearestTargetFramework = project.GetMetadata("NearestTargetFramework");
int targetFrameworkIndex = project.GetMetadata("TargetFrameworks").Split(';').ToList().IndexOf(nearestTargetFramework);
string projectAdditionalPropertiesMetadata = project.GetMetadata("AdditionalPropertiesFromProject").Split(new[] { ";;" }, StringSplitOptions.None)[targetFrameworkIndex];
Dictionary<string, string> projectAdditionalProperties = new(StringComparer.OrdinalIgnoreCase);
foreach (var propAndValue in projectAdditionalPropertiesMetadata.Split(';'))
{
    var split = propAndValue.Split('=');
    projectAdditionalProperties[split[0]] = split[1];
}

This is implemented in dotnet/sdk#15134.

If anyone has suggestions for a better separator than a double semicolon to separate the list of property/values for each TargetFramework, let me know.

Copy link
Member

@Forgind Forgind left a comment

I'm not a fan of relying on ;; as a separator, since a lot of people add ; to the beginning or end of lists if they aren't sure whether a property is empty or not like $(P);$(Q);(R) if P and Q happen to be empty, and that would confuse parsing it. Perhaps something a little more complicated like (TF1);;(TF2);;(TF3)? It's also a bit annoying to have to do so much parsing. Makes me wish we had item metadata metadata 😉
How annoying would it be to have a collection of Items like:

<ItemGroup>
<Net472AdditionalPropertiesFromProject>
<...>
</Net472AdditionalPropertiesFromProject>
<Netcoreapp3.1AdditionalPropertiesFromProject>
<...>
</Netcoreapp3.1AdditionalPropertiesFromProject>
</ItemGroup>

? Is it even possible to pass that through a ProjectReference?

src/Tasks/Microsoft.Common.CurrentVersion.targets Outdated Show resolved Hide resolved
@dsplaisted
Copy link
Member Author

@dsplaisted dsplaisted commented Dec 31, 2020

@Forgind I think it will handle empty properties and empty lists of properties correctly, though I'm not 100% sure.

Something like Net472AdditionalPropertiesFromProject won't work, as it needs to collect properties from whatever target frameworks the referenced project defines, which is an open set. See dotnet/sdk#15134 for how this would be used.

@Forgind
Copy link
Member

@Forgind Forgind commented Dec 31, 2020

This was my case:

<PropertyGroup>
<EmptyProperty Condition="false">x</EmptyProperty>
<NotEmpty Condition="true">value</NotEmpty>
<AlsoNotEmpty Condition="true">otherVal</AlsoNotEmpty>
</PropertyGroup>
...
<PropertyGroup>
<UsesProperties>$(AlsoNotEmpty);$(EmptyProperty);$(NotEmpty)</UsesProperties>
</PropertyGroup>
...

and then trying to pass that to another project. I'm not sure either, mostly because I get confused sometimes by item transforms and such. As I understand it, it would accept UsesProperties as a valid property (which it is) and add it to AdditionalPropertiesFromProject, then parsing would break the property in two, get confused, and silently use it incorrectly. I could be wrong.

@dsplaisted dsplaisted requested a review from cdmihai Jan 6, 2021
@dsplaisted
Copy link
Member Author

@dsplaisted dsplaisted commented Jan 6, 2021

@cdmihai How would this interact with static graph?

@rainersigwald
Copy link
Contributor

@rainersigwald rainersigwald commented Jan 6, 2021

High-level questions (this implementation looks ok but I wonder if we can improve the overall design).

  1. Would this be better as one-metadatum-per-TF? With names like AdditionalPropertiesFromProject_net472? That'd be somewhat harder to assemble but avoids (one layer of) the delimiting problem.
  2. Another option would be to pass it back as an XML blob. That's what solutions do for similar reasons.

Those are both harder to implement with MSBuild logic and would suggest writing a new AggregateMultitargetingInformation task, but that'd probably be easy to implement and understand.

If we stick with delimited, maybe consider using one of the fancier unicode characters like INFORMATION SEPARATOR TWO?

@cdmihai
Copy link
Contributor

@cdmihai cdmihai commented Jan 6, 2021

Looks fine for static graph, as the msbuild task calling patterns haven't changed, just the data ferried by the calls, which is opaque to static graph builds.

@cdmihai
Copy link
Contributor

@cdmihai cdmihai commented Jan 6, 2021

Can you also update the protocol doc in this PR?

@Forgind
Copy link
Member

@Forgind Forgind commented Jan 22, 2021

Team triage:
@dsplaisted
Just checking whether the feedback on this was clear—we don't think this is mergeable as-is without one of the changes rainersigwald proposed. If it's just that there are other things higher on your priority list, carry on.

@dsplaisted
Copy link
Member Author

@dsplaisted dsplaisted commented Jan 26, 2021

@Forgind I've updated this to use Xml to pass the data around

Copy link
Member

@Forgind Forgind left a comment

Some nits, but this looks good, thanks!

@@ -0,0 +1,34 @@
using Microsoft.Build.Framework;

This comment has been minimized.

@Forgind

Forgind Jan 26, 2021
Member

copyright header

@@ -0,0 +1,34 @@
using Microsoft.Build.Framework;

This comment has been minimized.

@Forgind

Forgind Jan 26, 2021
Member

copyright header


Result = root.ToString();

return true;

This comment has been minimized.

@Forgind

Forgind Jan 26, 2021
Member

return !HasLoggedErrors? Same below.

{
public class CombineXmlElements : TaskExtension
{
public string RootElementName { get; set; }

This comment has been minimized.

@Forgind

Forgind Jan 26, 2021
Member

Required annotations for these?

@@ -1743,12 +1751,26 @@ Copyright (C) Microsoft Corporation. All rights reserved.
<Target Name="GetTargetFrameworksWithPlatformForSingleTargetFramework"
Returns="@(_TargetFrameworkInfo)">

<ItemGroup>
<_AdditionalTargetFrameworkInfoPropertyWithValue Include="@(AdditionalTargetFrameworkInfoProperty)">
<Value>$(%(AdditionalTargetFrameworkInfoProperty.Identity))</Value>

This comment has been minimized.

@Forgind

Forgind Jan 26, 2021
Member

I didn't realize you could have a metadata reference as an identifier for a property. Neat!

@cdmihai
Copy link
Contributor

@cdmihai cdmihai commented Jan 27, 2021

Are the tests for this new flow in the sdk repo? If not, add some here?

@dsplaisted
Copy link
Member Author

@dsplaisted dsplaisted commented Jan 27, 2021

Are the tests for this new flow in the sdk repo? If not, add some here?

Yes, tests for this are in dotnet/sdk#15134

@dsplaisted dsplaisted changed the base branch from vs16.9 to master Jan 29, 2021
@dsplaisted
Copy link
Member Author

@dsplaisted dsplaisted commented Jan 29, 2021

I've applied the code review feedback and retargeted this to the master branch for VS 16.10.

@Forgind Forgind merged commit 83cd7d4 into dotnet:master Feb 3, 2021
8 checks passed
8 checks passed
license/cla All CLA requirements met.
Details
@azure-pipelines
msbuild-pr Build #20210129.9 succeeded
Details
@azure-pipelines
msbuild-pr (Linux Core) Linux Core succeeded
Details
@azure-pipelines
msbuild-pr (Windows Core) Windows Core succeeded
Details
@azure-pipelines
msbuild-pr (Windows Full Release (no bootstrap)) Windows Full Release (no bootstrap) succeeded
Details
@azure-pipelines
msbuild-pr (Windows Full) Windows Full succeeded
Details
@azure-pipelines
msbuild-pr (macOS Core) macOS Core succeeded
Details
@azure-pipelines
msbuild-pr (macOS Mono) macOS Mono succeeded
Details
@ericstj
Copy link
Member

@ericstj ericstj commented Jun 21, 2021

Looks like this broke WCF as it doesn't handle cases like portable-net45+win8+wp8

@dsplaisted
Copy link
Member Author

@dsplaisted dsplaisted commented Jun 22, 2021

Looks like this broke WCF as it doesn't handle cases like portable-net45+win8+wp8

@ericstj Do you have more details on how it broke? Is it something we need to fix? I don't know if we support the old PCL TFMs anymore.

@ericstj
Copy link
Member

@ericstj ericstj commented Jun 22, 2021

I filed #6603 to track the regression. It was due to an invalid XML character in the TargetFramework. I imagine a fix would be to make sure you escape the strings which seems like something you might want to do regardless.

I was able to help WCF find a workaround since they aren't actually building for PCLs (they are just trying to use CSProj + pack to correctly represent a package which overlaps with the PCL targeting packs). ericstj/wcf@66e68f2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants