Remove xmlns from props/targets and UTs #7169

dfederm · 2021-12-22T22:02:07Z

xmlns hasn't been requires in project files for a while now (v15?), so this change removes it from all props/targets as well as all UTs (minus the ones explicitly testing the xmlns stuff).

dfederm · 2021-12-22T22:02:40Z

FYI @Nirmal4G since you were interested in this

Nirmal4G · 2021-12-22T22:16:36Z

Nice... Upon reviewing, I see <?xml?> tags too. We can remove them too!

Contributes to #4779 and #6645

Nirmal4G · 2021-12-22T22:25:49Z

Also, if it's not too much trouble, can you separate your changes into specific commits. Like separate the formatting and xmlns removals (even this can be further separated into removals in shipping files, project files, docs and unit tests).

Totally optional but it's just good practice.

Forgind

I'm not going to pretend I legitimately reviewed this, but the part I did review looked good! xmlns and ToolsVersion confused me before I learned that I can pretty much ignore them.

Forgind · 2021-12-22T22:39:32Z

src/Build.OM.UnitTests/Construction/ElementLocationPublic_Tests.cs

@@ -82,7 +82,7 @@ public void XmlLocationsAreCached()
        public void LocationStringsMedley()
        {
            string content = @"
-            <Project ToolsVersion=`msbuilddefaulttoolsversion` xmlns=`http://schemas.microsoft.com/developer/msbuild/2003`>
+            <Project ToolsVersion=`msbuilddefaulttoolsversion`>


You can also remove the ToolsVersion, right?

Yup, although I figured this PR was large enough as it was, so probably will follow up with another PR.

dfederm · 2021-12-22T23:04:24Z

Also, if it's not too much trouble, can you separate your changes into specific commits. Like separate the formatting and xmlns removals (even this can be further separated into removals in shipping files, project files, docs and unit tests).

Totally optional but it's just good practice.

The formatting was automatic by vscoed and unintentional :(. But figured I'd leave them in place. I should look into turning that off by default...

Nirmal4G · 2021-12-23T02:53:58Z

No problem. It's good that you format the files that is touched but bad that every change is in a single commit, that's all!

Nirmal4G

Took a long time but it's looking good! 👌

sharwell · 2022-01-03T00:17:51Z

src/Build.OM.UnitTests/Construction/ProjectChooseElement_Tests.cs

@@ -268,13 +268,13 @@ public void ExcessivelyNestedChoose()
        public void SettingWhenConditionDirties()
        {
            string content = @"
-                    <Project xmlns='http://schemas.microsoft.com/developer/msbuild/2003' >
+                    <Project>


💡 Make sure to verify there is still adequate coverage for cases where xmlns is included, since both approaches are supported

Yup, I left it in a bunch of UTs, mostly ones which dealt with evaluation. It probably could be cleaned from even more UTs (many use xmlns="msbuildnamespace" instead of xmlns="http://schemas.microsoft.com/developer/msbuild/2003">), but whatever this change is large enough as-is.

Comparing repo-wide searches of "xmlns=" (which admittedly covers other namespaces eg in loc xml files):
Before: 2641 results in 377 files
After: 1693 results in 277 files

This also had me worried but by inspection it looks ok (this concern is why I split out the "remove from unit tests" part of the change).

src/Utilities.UnitTests/ToolLocationHelper_Tests.cs

src/Build.OM.UnitTests/Construction/ProjectItemElement_Tests.cs

benvillalobos · 2022-01-13T19:26:27Z

src/Deprecated/Conversion.UnitTests/XMakeConversionUnitTests.csproj

@@ -1,5 +1,5 @@
 <?xml version="1.0" encoding="utf-8"?>
-<Project ToolsVersion="dogfood" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">


is dogfood relevant here? it's deprecated so...meh

Do deprecated unit tests even run? Like ever? Should we just delete this?

they don't at the moment but I hate to delete them because we should turn them back on (but maybe we can just delete the code first?

More than happy to delete deprecated UTs if that's the decision. But for another PR, as this one is big enough :)

benvillalobos

Checked everything but the .cs files and it looks fine, minus my 1 question on compat. If we won't have any compat issues, I approve.

benvillalobos · 2022-01-13T19:27:57Z

src/Tasks/Microsoft.CSharp.CrossTargeting.targets

@@ -10,7 +10,7 @@ Copyright (C) Microsoft Corporation. All rights reserved.
 ***********************************************************************************************
 -->

-<Project xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
+<Project>


This won't break compatibility with some super old MSBuild that's still in support will it?

As with #7165, the assertion is that these targets are never used with a super-old MSBuild.

I left it in a bunch of UTs, mostly ones which dealt with evaluation. It probably could be cleaned from even more UTs (many use `xmlns="msbuildnamespace"` instead of `xmlns="http://schemas.microsoft.com/developer/msbuild/2003">`), but whatever this change is large enough as-is. Comparing repo-wide searches of "xmlns=" (which admittedly covers other namespaces eg in loc xml files): Before: 2641 results in 377 files After: 1693 results in 277 files

rainersigwald · 2022-01-13T21:41:00Z

I just pushed a change that a) splits into two commits, one for "MSBuild logic in/shipped from this repo" and one for tests, and b) drops the changes in Deprecated.

Forgind approved these changes Dec 22, 2021

View reviewed changes

Nirmal4G approved these changes Dec 24, 2021

View reviewed changes

sharwell reviewed Jan 3, 2022

View reviewed changes

Forgind reviewed Jan 12, 2022

View reviewed changes

src/Utilities.UnitTests/ToolLocationHelper_Tests.cs Outdated Show resolved Hide resolved

Forgind reviewed Jan 13, 2022

View reviewed changes

src/Build.OM.UnitTests/Construction/ProjectItemElement_Tests.cs Outdated Show resolved Hide resolved

benvillalobos reviewed Jan 13, 2022

View reviewed changes

benvillalobos approved these changes Jan 13, 2022

View reviewed changes

dfederm added 2 commits January 13, 2022 15:34

Remove xmlns from props/targets and projects

0bef5b1

rainersigwald force-pushed the remove-xmlns branch from 348b71f to af7c186 Compare January 13, 2022 21:40

rainersigwald approved these changes Jan 13, 2022

View reviewed changes

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 Jan 19, 2022

ladipro merged commit 8872ed6 into dotnet:main Jan 21, 2022

This was referenced Jan 21, 2022

Adjust Indentation in XAML Rules and Common props/targets/tasks #7168

Merged

Remove XML meta tag (<?xml?>) from all XML-derived files #7313

Closed

dfederm deleted the remove-xmlns branch July 29, 2022 18:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove xmlns from props/targets and UTs #7169

Remove xmlns from props/targets and UTs #7169

dfederm commented Dec 22, 2021

dfederm commented Dec 22, 2021

Nirmal4G commented Dec 22, 2021 •

edited

Loading

Nirmal4G commented Dec 22, 2021

Forgind left a comment

Forgind Dec 22, 2021

dfederm Dec 22, 2021

dfederm commented Dec 22, 2021

Nirmal4G commented Dec 23, 2021

Nirmal4G left a comment

sharwell Jan 3, 2022

dfederm Jan 3, 2022

rainersigwald Jan 13, 2022

benvillalobos Jan 13, 2022

Forgind Jan 13, 2022

rainersigwald Jan 13, 2022

dfederm Jan 13, 2022

benvillalobos left a comment

benvillalobos Jan 13, 2022

dfederm Jan 13, 2022

rainersigwald commented Jan 13, 2022

		@@ -1,5 +1,5 @@
		<?xml version="1.0" encoding="utf-8"?>
		<Project ToolsVersion="dogfood" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">

Remove xmlns from props/targets and UTs #7169

Remove xmlns from props/targets and UTs #7169

Conversation

dfederm commented Dec 22, 2021

dfederm commented Dec 22, 2021

Nirmal4G commented Dec 22, 2021 • edited Loading

Nirmal4G commented Dec 22, 2021

Forgind left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dfederm commented Dec 22, 2021

Nirmal4G commented Dec 23, 2021

Nirmal4G left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benvillalobos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rainersigwald commented Jan 13, 2022

Nirmal4G commented Dec 22, 2021 •

edited

Loading