Update schema for combining TargetFramework info to allow for invalid xml names such as including '+' #6699

Forgind · 2021-07-21T22:43:08Z

Context

The ability to gather extra properties from referenced projects was added in #5994. It used XML transformations to couch that logic in a way that MSBuild could understand.

Unfortunately, the XML-based logic assumed everything was a valid XML element. Ultimately, the goal of the change, at least as I understand it, was just to pass information.

Changes Made

TargetFrameworks had been XML elements, but some are not valid XML names. This was changed to being a property on the more generically named TargetFramework element. This also allows for escaping.

Testing

Verified that a simple case that had not previously worked now works and looked at what it produced in a debugger.

Notes

After this PR, the SDK can opt in. Then MSBuild can turn it on by default, and the SDK can stop specifying the property.

benvillalobos

LGTM. I'd be fine with a simple local test of comparing the strings between the way this was done previously, and the new way. Then verify that a stage1 build succesfully builds the repro listed in the original bug. Or just the latter.

benvillalobos · 2021-07-22T18:45:21Z

src/Tasks/CombineTargetFrameworkInfoProperties.cs

                foreach (var item in PropertiesAndValues)
                {
-                    root.Add(new XElement(item.ItemSpec, item.GetMetadata("Value")));
+                    sb.AppendLine($"  <{item.ItemSpec}>{item.GetMetadata("value")}</{item.ItemSpec}>");


Is the getmetadata search case sensitive? And should there be a tab in the append line?

It should be case-insensitive, but there's no reason to test that here...

I had two spaces. That seemed to be how it was with XElement rather than the tab when I was inspecting its escaped form.

Also, I verified that this can build the repro but not that it actually produces correct results, which is why I said it isn't properly tested yet 🙂 In my list of things to do. A test is a good plan.

dsplaisted · 2021-07-23T02:33:14Z

I think this data is parsed as XML elsewhere, so I don't think this will fix the problem. Instead of allowing invalid XML, I think we should escape characters that aren't valid.

Forgind · 2021-07-23T02:37:17Z

I think this data is parsed as XML elsewhere, so I don't think this will fix the problem. Instead of allowing invalid XML, I think we should escape characters that aren't valid.

I just went with this because it made the repro not work anymore.

As far as escaping invalid characters, I tried replacing + with %2b, &2b;, or +, and all of those failed. Is there some other escaping standard I should be using? Should I be replacing it with something like 'literalPlus' in CombineTargetFrameworkInfoProperties and looking for that in CombineXmlElements and replacing it again with +?

dsplaisted · 2021-07-26T16:04:48Z

Looks like there's not much we can do to escape characters in an XML name: https://stackoverflow.com/questions/19049954/escape-in-a-element-name-in-xml-file

I think what we should do is that if the ItemSpec isn't a valid XML name, then we should come up with a name that is valid (perhaps by removing invalid characters), and put the real name in an attribute value.

Forgind · 2021-07-26T23:28:52Z

So were you thinking something more like this:
Forgind@2525a15
?

(Cleaned up a bit)

rainersigwald · 2021-07-28T03:54:10Z

I think what we should do is that if the ItemSpec isn't a valid XML name, then we should come up with a name that is valid (perhaps by removing invalid characters), and put the real name in an attribute value.

Can we instead change the schema to be something like

<Property Name="..." Value="..." />

And then use standard XML escaping to represent anything?

dsplaisted · 2021-07-28T04:20:13Z

I think what we should do is that if the ItemSpec isn't a valid XML name, then we should come up with a name that is valid (perhaps by removing invalid characters), and put the real name in an attribute value.

Can we instead change the schema to be something like
<Property Name="..." Value="..." />
And then use standard XML escaping to represent anything?

That would be better, but there's the ValidateExecutableReferences task in dotnet/sdk consumes this XML. So we'd have to figure out how to change the format in a compatible way. It's also possible (though probably unlikely, as this is new code) that there are other consumers of this XML.

…operties-with-plus

Forgind · 2021-08-04T22:08:18Z

Hopefully you're fine with how I did this—I made the new schema only activate if you set the UseNewSchema input to be true (it defaults to false), so this should have no impact unless you ask it to.

In SDK-land, then, we can opt into using that and make the relevant changes to ValidateExecutableReferences.

Then MSBuild can remove the UseNewSchema flag and just always use it, and the SDK can stop setting it.

If we don't document it, and we keep it all within one release, hopefully it's fine to tempt users with an extra flag then take it away.

Any other comments or concerns?

They didn't autogenerate for some reason with build.cmd. Had to use the CIBuild.

dsplaisted · 2021-08-05T17:44:05Z

I think this is a good idea, but we need a better name for UseNewSchema. For the property, it should probably start with an underscore, and be something like _UseAttributeForTargetFrameworkInfoPropertyNames. Also, the example XML in the documentation should also be updated: https://github.com/dotnet/msbuild/blob/main/documentation/ProjectReference-Protocol.md

Forgind · 2021-08-05T19:39:00Z

I can fix that. I was thinking it doesn't matter too much if we want it taken out later, but we definitely don't want someone making explicit use of it before that happens 🙂

I think I should hold off on the documentation change until Then MSBuild can remove the UseNewSchema flag and just always use it, and the SDK can stop setting it. because otherwise anyone trying to understand it would be confused. Hopefully that's no one, but not being able to get something to work in the moment can be frustrating. Maybe I should add a comment saying it's changing so don't assume it's static?

dsplaisted

👍

Were you able to do any sanity testing of this?

Forgind · 2021-08-06T15:46:20Z

👍

Were you able to do any sanity testing of this?

I know how to do one kind of sanity testing, that is, checking whether the repro still reproduces, in this case with the property enabled, and I can inspect it in the debugger. Is there a more definitive test I can try?

dsplaisted · 2021-08-06T15:54:05Z

That sounds good

Forgind · 2021-08-06T16:14:50Z

This is what it looks like, by the way:

<Property Name="AdditionalProjectProperties">
  <Property Name="netstandard1.3">
    <SelfContained></SelfContained>
    <_IsExecutable></_IsExecutable>
    <ShouldBeValidatedAsExecutableReference></ShouldBeValidatedAsExecutableReference>
  </Property>
  <Property Name="portable-net45+win8+wp8">
    <SelfContained></SelfContained>
    <_IsExecutable></_IsExecutable>
    <ShouldBeValidatedAsExecutableReference></ShouldBeValidatedAsExecutableReference>
  </Property>
</Property>

I wasn't 100% sure that the second level of escaping was necessary (in CombineXmlElements), but at worst, it shouldn't hurt.

dsplaisted · 2021-08-06T16:19:18Z

Actually, let's not do it in CombineXmlElements, that's probably always just going to be AdditionalProjectProperties, and if it's not we still shouldn't need arbitrary property names there, because it's the root element of the XML document.

Forgind · 2021-08-06T17:18:48Z

Sounds good. I changed it to look like:

<AdditionalProjectProperties>
  <Property Name="netstandard1.3">
    <SelfContained></SelfContained>
    <_IsExecutable></_IsExecutable>
    <ShouldBeValidatedAsExecutableReference></ShouldBeValidatedAsExecutableReference>
  </Property>
  <Property Name="portable-net45+win8+wp8">
    <SelfContained></SelfContained>
    <_IsExecutable></_IsExecutable>
    <ShouldBeValidatedAsExecutableReference></ShouldBeValidatedAsExecutableReference>
  </Property>
</AdditionalProjectProperties>

Forgind · 2021-08-09T15:26:47Z

src/Tasks/CombineTargetFrameworkInfoProperties.cs

@@ -36,9 +37,11 @@ public override bool Execute()
        {
            if (PropertiesAndValues != null)
            {
-                XElement root = new XElement(RootElementName);
+                XElement root = UseAttributeForTargetFrameworkInfoPropertyNames ?
+                    new("Property", new XAttribute("Name", EscapingUtilities.Escape(RootElementName))) :


PR Triage: TargetFramework Name=...

Change title pre-merge

documentation/ProjectReference-Protocol.md

benvillalobos reviewed Jul 22, 2021

View reviewed changes

Forgind requested a review from dsplaisted July 22, 2021 21:22

Call XElements "Property" with escaped names

7b41f2c

Forgind force-pushed the combinetargetframeworkproperties-with-plus branch from d597f16 to 7b41f2c Compare July 30, 2021 18:58

Forgind added 3 commits August 4, 2021 09:01

Merge https://github.com/dotnet/msbuild into combinetargetframeworkpr…

f90bd86

…operties-with-plus

Workaround to prevent SDK breakages

8d833fa

Switch switch to optional input

d0bb260

Forgind added 3 commits August 4, 2021 15:49

Make enableable via property

c51ae03

Add reference assemblies

11d458e

They didn't autogenerate for some reason with build.cmd. Had to use the CIBuild.

One other case

bd7137a

Forgind added 2 commits August 5, 2021 13:28

Feedback

ccc6ded

build

05caac0

dsplaisted approved these changes Aug 6, 2021

View reviewed changes

Remove escaping from CombineXmlElements

3a5f66e

Forgind commented Aug 9, 2021

View reviewed changes

Change name

9db3be1

rainersigwald approved these changes Aug 16, 2021

View reviewed changes

documentation/ProjectReference-Protocol.md Outdated Show resolved Hide resolved

Update documentation/ProjectReference-Protocol.md

d9ee0e5

Forgind changed the title ~~Switch XML manipulations to string manipulations~~ Update schema for combining TargetFramework info to allow for invalid xml names such as including '+' Aug 16, 2021

Forgind added the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label Aug 16, 2021

Forgind merged commit 00166eb into dotnet:main Aug 16, 2021

Forgind deleted the combinetargetframeworkproperties-with-plus branch August 16, 2021 23:17

dsplaisted mentioned this pull request Sep 10, 2021

Update ValidateExecutableReferences with new schema dotnet/sdk#19786

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update schema for combining TargetFramework info to allow for invalid xml names such as including '+' #6699

Update schema for combining TargetFramework info to allow for invalid xml names such as including '+' #6699

Forgind commented Jul 21, 2021 •

edited

benvillalobos left a comment

benvillalobos Jul 22, 2021

Forgind Jul 22, 2021

Forgind Jul 22, 2021 •

edited

dsplaisted commented Jul 23, 2021

Forgind commented Jul 23, 2021

dsplaisted commented Jul 26, 2021

Forgind commented Jul 26, 2021 •

edited

rainersigwald commented Jul 28, 2021

dsplaisted commented Jul 28, 2021

Forgind commented Aug 4, 2021

dsplaisted commented Aug 5, 2021

Forgind commented Aug 5, 2021

dsplaisted left a comment

Forgind commented Aug 6, 2021

dsplaisted commented Aug 6, 2021

Forgind commented Aug 6, 2021

dsplaisted commented Aug 6, 2021

Forgind commented Aug 6, 2021

Forgind Aug 9, 2021

Forgind Aug 16, 2021

Update schema for combining TargetFramework info to allow for invalid xml names such as including '+' #6699

Update schema for combining TargetFramework info to allow for invalid xml names such as including '+' #6699

Conversation

Forgind commented Jul 21, 2021 • edited

Context

Changes Made

Testing

Notes

benvillalobos left a comment

Choose a reason for hiding this comment

benvillalobos Jul 22, 2021

Choose a reason for hiding this comment

Forgind Jul 22, 2021

Choose a reason for hiding this comment

Forgind Jul 22, 2021 • edited

Choose a reason for hiding this comment

dsplaisted commented Jul 23, 2021

Forgind commented Jul 23, 2021

dsplaisted commented Jul 26, 2021

Forgind commented Jul 26, 2021 • edited

rainersigwald commented Jul 28, 2021

dsplaisted commented Jul 28, 2021

Forgind commented Aug 4, 2021

dsplaisted commented Aug 5, 2021

Forgind commented Aug 5, 2021

dsplaisted left a comment

Choose a reason for hiding this comment

Forgind commented Aug 6, 2021

dsplaisted commented Aug 6, 2021

Forgind commented Aug 6, 2021

dsplaisted commented Aug 6, 2021

Forgind commented Aug 6, 2021

Forgind Aug 9, 2021

Choose a reason for hiding this comment

Forgind Aug 16, 2021

Choose a reason for hiding this comment

Forgind commented Jul 21, 2021 •

edited

Forgind Jul 22, 2021 •

edited

Forgind commented Jul 26, 2021 •

edited