Implicit Sdk imports #1403

rainersigwald · 2016-11-28T21:00:06Z

First iteration of #1392.

Known issues:

Doesn't define a default location for the SDKs. MUST FIX
Allows Sdk attribute on any <Project> element, even ones that are (recursively) imported. That's confusing and wrong.
Preprocessed output is not buildable (will double-import SDK imports)
Not enough tests (I'll start working on this next, wanted to get this out)

Example of use:
test.proj

<Project Sdk="TestSdk">
  <PropertyGroup>
    <PropertyDefinedInProjectFromImportedProperty>Local/$(PropertyFromSdkImport)</PropertyDefinedInProjectFromImportedProperty>
  </PropertyGroup>

  <Target Name="Build">
    <Message Importance="High"
             Text="Build target. Prop: $(PropertyFromSdkImport) LocalProp: $(PropertyDefinedInProjectFromImportedProperty)" />
  </Target>
</Project>

TestSdk\Sdk\Sdk.props

<Project>
  <PropertyGroup>
    <PropertyFromSdkImport>Imported Property!</PropertyFromSdkImport>
  </PropertyGroup>
</Project>

TestSdk\Sdk\Sdk.targets

<Project>
  <Target Name="SdkTarget"
          BeforeTargets="Build">
    <Message Importance="High"
             Text="Target from the SDK" />
  </Target>
</Project>

S:\work\autoimports>s:\msbuild\bin\Bootstrap\15.0\Bin\MSBuild.exe test.proj
Microsoft (R) Build Engine version 15.1.425.23802
Copyright (C) Microsoft Corporation. All rights reserved.

Build started 11/28/2016 2:58:47 PM.
Project "S:\work\autoimports\test.proj" on node 1 (default targets).
SdkTarget:
  Target from the SDK
Build:
  Build target. Prop: Imported Property! LocalProp: Local/Imported Property!
Done Building Project "S:\work\autoimports\test.proj" (default targets).


Build succeeded.
    0 Warning(s)
    0 Error(s)

Time Elapsed 00:00:00.55

preprocessor output:

<?xml version="1.0" encoding="utf-8"?>
<!--
============================================================================================================================================
S:\work\autoimports\test.proj
============================================================================================================================================
-->
<Project Sdk="TestSdk">
  <!--
============================================================================================================================================
  Import of "S:\work\autoimports\TestSdk\Sdk\Sdk.props" from Sdk "TestSdk" was implied by the Project element's Sdk attribute.

S:\work\autoimports\TestSdk\Sdk\Sdk.props
============================================================================================================================================
-->
  <PropertyGroup>
    <PropertyFromSdkImport>Imported Property!</PropertyFromSdkImport>
  </PropertyGroup>
  <!--
============================================================================================================================================
  </Import>

S:\work\autoimports\test.proj
============================================================================================================================================
-->
  <PropertyGroup>
    <PropertyDefinedInProjectFromImportedProperty>Local/$(PropertyFromSdkImport)</PropertyDefinedInProjectFromImportedProperty>
  </PropertyGroup>
  <Target Name="Build">
    <Message Importance="High" Text="Build target. Prop: $(PropertyFromSdkImport) LocalProp: $(PropertyDefinedInProjectFromImportedProperty)" />
  </Target>
  <!--
============================================================================================================================================
  Import of "S:\work\autoimports\TestSdk\Sdk\Sdk.targets" from Sdk "TestSdk" was implied by the Project element's Sdk attribute.

S:\work\autoimports\TestSdk\Sdk\Sdk.targets
============================================================================================================================================
-->
  <Target Name="SdkTarget" BeforeTargets="Build">
    <Message Importance="High" Text="Target from the SDK" />
  </Target>
  <!--
============================================================================================================================================
  </Import>

S:\work\autoimports\test.proj
============================================================================================================================================
-->
</Project>

jeffkl · 2016-11-28T21:06:28Z

src/XMakeBuildEngine/Construction/ProjectElementContainer.cs

@@ -540,6 +545,15 @@ internal void AddToXml(ProjectElement child)
            }
        }

+        private static bool HasXmlRepresentation(ProjectElement element)


I think the IsImplicit property should be enough, do you think we need a method to call instead of checking the property?

It wasn't enough in the original PR because I only put Implicit on ProjectImportElement. But you're right: if I just extend it to all ProjectElements this becomes much cleaner.

jeffkl · 2016-11-28T21:06:34Z

src/Shared/UnitTests/ObjectModelHelpers.cs

+            private readonly string _name;
+            private readonly string _originalValue;
+
+            public TemporaryEnvironment(string name, string value)


Would it make sense for this to take an IDictionary<string, string> so you could have one or more variables set?

I envisioned a cascading set of usings but yeah, that's more elegant. Mind if we wait until after RC2?

I'll leave it up to you

jeffkl · 2016-11-28T21:06:37Z

ref/net46/Microsoft.Build/Microsoft.Build.cs

@@ -83,6 +83,7 @@ public partial class ProjectExtensionsElement : Microsoft.Build.Construction.Pro
    public partial class ProjectImportElement : Microsoft.Build.Construction.ProjectElement
    {
        internal ProjectImportElement() { }
+        public bool Implicit { get { throw null; } }


IsImplicit sounds better to me but I'll leave it up to you.

Yeah, looks like that matches existing ones better.

jeffkl · 2016-11-28T21:11:02Z

src/XMakeBuildEngine/Evaluation/ProjectParser.cs

+
+                var sdksString = element.GetAttribute(XMakeAttributes.sdk);
+
+                var sdks = sdksString.Contains(";") ? sdksString.Split(';') : new[] {sdksString};


I'd use .Split(';', RemoveEmptyEntries).Select(i => i.Trim()).Where(i => !String.IsNullOrWhiteSpace()) to be as robust as possible.

Yes, absolutely! This file didn't use LINQ so I did it manually.

jeffkl · 2016-11-28T21:13:46Z

src/XMakeBuildEngine/Construction/ProjectImportElement.cs

        /// </summary>
-        public ElementLocation ProjectLocation
+        // TODO: *should* this be public? if it's not, you can't determine if an import is implicit from the public OM.


I think it should be public. With no public setter, it seems safe.

jeffkl · 2016-11-28T21:16:38Z

src/XMakeBuildEngine/Construction/ProjectRootElement.cs

+                        var implicitElements =
+                            xmlWithNoImplicits.SelectNodes($"//*[@{XMakeAttributes.@implicit}]");
+
+                        if (implicitElements == null)


Would this ever happen? I think if the document is parsed and successfully cloned, this would always return a value?

I can't think of a reason for this to fail either, but it seemed worth checking and throwing. Would you rather I just let a NRE happen later?

I did some searching and couldn't find an instance where it would return null unless the document couldn't be parsed. I think its safe to remove the check personally.

jeffkl · 2016-11-28T21:17:22Z

src/XMakeBuildEngine/Evaluation/Preprocessor.cs

+                        if (((XmlElement)child).GetAttribute(XMakeAttributes.@implicit).Length > 0)
+                        {
+                            importTag =
+                                $"  Import of \"{importProject}\" from Sdk \"{importSdk}\" was implied by the {XMakeElements.project} element's {XMakeAttributes.sdk} attribute.";


dsplaisted

LGTM, the only thing I really want changed is for you to get a build error if you specify an SDK that doesn't exist, or is misspelled. I also think we may need to switch to handling the Sdk attribute in the evaluator rather than in the parser (for example, to handle dependencies between Sdks and importing the props/targets in the right order). However, let's go with what works for now for the first preview with this feature.

Doesn't define a default location for the SDKs. MUST FIX

Do we actually need to fix this? My understanding is that we were going to set this for full framework MSBuild in its app.config file, and the .NET CLI would set the environment variable when it launched an MSBuild process.

Allows Sdk attribute on any <Project> element, even ones that are (recursively) imported. That's confusing and wrong.

Agreed that this should be disabled now, but I'm noodling on a design for handling Sdk dependencies where an Sdk's Sdk.props file would list it's dependencies using the Sdk attribute of the Project element.

dsplaisted · 2016-11-29T09:14:11Z

src/XMakeBuildEngine/Construction/ProjectElementContainer.cs

@@ -619,7 +633,11 @@ private void AddInitialChild(ProjectElement child)
            AddToXml(child);

            _count++;
-            MarkDirty("Add child element named '{0}'", child.ElementName);
+
+            if (HasXmlRepresentation(child))


Wouldn't you also want to suppress marking the tree as dirty if the child is implicit in InsertBeforeChild and InsertAfterChild?

dsplaisted · 2016-11-29T09:16:36Z

src/XMakeBuildEngine/Construction/ProjectImportElement.cs

+        /// Location of the project attribute
+        /// </summary>
+        /// <remarks>
+        /// For an implicit import, the location points to the Sdk attribute on the Project element.


This line of the comment doesn't seem to match the behavior in the code.

Good catch--remnant of a previous attempt.

dsplaisted · 2016-11-29T09:19:43Z

src/XMakeBuildEngine/Construction/ProjectImportElement.cs

        {
-            get { return XmlElement.GetAttributeLocation(XMakeAttributes.project); }
+            get { return XmlElement.HasAttribute(XMakeAttributes.@implicit); }


Could we represent whether an import is implicit or not with a field in this class instead of a separate attribute? What happens if someone actually uses the attribute?

<Import Project="foo.targets" _Implicit="Foo" />

It throws an error: https://github.com/Microsoft/msbuild/pull/1403/files#diff-a9d9d41d4ebe72ee0285850966f63d3fR576

Test TK.

Also, I tried using a field in this class, but it caused problems with the preprocessor, since it walks only the XML tree.

dsplaisted · 2016-11-29T09:24:41Z

src/XMakeBuildEngine/Construction/ProjectRootElement.cs

@@ -1714,8 +1714,23 @@ public void Save(Encoding saveEncoding)
                {
                    using (ProjectWriter projectWriter = new ProjectWriter(_projectFileLocation.File, saveEncoding))
                    {
-                        projectWriter.Initialize(XmlDocument);
-                        XmlDocument.Save(projectWriter);
+                        var xmlWithNoImplicits = (XmlDocument)XmlDocument.Clone();


At first glance the variable name led me to believe that it wouldn't have any implicit imports when it was cloned, not that you were going to remove them afterwards. It might be possible to make this clearer with a better name.

dsplaisted · 2016-11-29T09:25:10Z

src/XMakeBuildEngine/Evaluation/Evaluator.cs

@@ -1081,7 +1081,6 @@ private void PerformDepthFirstPass(ProjectRootElement currentProjectOrImport)
                }

                ProjectImportElement import = element as ProjectImportElement;
-


No-op whitespace change here.

dsplaisted · 2016-11-29T09:37:20Z

src/XMakeBuildEngine/Evaluation/ProjectParser.cs

+                    var finalImportPath = Path.Combine(Environment.GetEnvironmentVariable("MSBuildSDKsPath"),
+                        sdkName, "Sdk", "Sdk.targets");
+
+                    if (File.Exists(initialImportPath))


I think we need to generate an error if an Sdk is specified that is not found. However, that should be an error at evaluation, not parse time. To get that behavior for now, I think we should always add both implicit import elements whether the files exist or not, which will mean that all of our Sdks will need to have both Sdk.props and Sdk.targets.

When we add support for an Sdk attribute on an Import and implement the Project-level Sdk attribute in terms of Sdk imports, we may be able to change how this works.

dsplaisted · 2016-11-29T09:39:47Z

src/XMakeBuildEngine/Microsoft.Build.csproj

@@ -1,4 +1,4 @@
-<?xml version="1.0" encoding="utf-8"?>
+<?xml version="1.0" encoding="utf-8"?>


No-op changes (EOL probably) here and at end of file.

dsplaisted · 2016-11-29T09:42:23Z

src/XMakeBuildEngine/UnitTestsPublicOM/Construction/ConstructionEditing_Tests.cs

+            {
+                Directory.CreateDirectory(testSdkDirectory);
+
+                string sdkPropsPath = Path.Combine(testSdkDirectory, "Sdk.props");


I think this needs to be Path.Combine(testSdkDirectory, "Sdk", "Sdk.props") to match the path where the parser is going to look for the Sdk imports.

dsplaisted · 2016-11-29T09:43:26Z

src/XMakeBuildEngine/UnitTestsPublicOM/Construction/ProjectImportElement_Tests.cs

@@ -45,53 +45,56 @@ public void ReadNone()
        public void ReadInvalidMissingProject()
        {
            Assert.Throws<InvalidProjectFileException>(() =>
-            {
-                string content = @"
+                {


Lots of whitespace changes in this file.

dsplaisted · 2016-11-29T09:46:30Z

src/XMakeBuildEngine/UnitTestsPublicOM/Construction/ProjectImportElement_Tests.cs

+                File.WriteAllText(sdkPropsPath, string.Empty);
+                File.WriteAllText(sdkTargetsPath, string.Empty);
+
+                using (new Helpers.TemporaryEnvironment("MSBUILDMAGICIMPORTDIRECTORY", testSdkRoot))


Change to MSBuildSDKsPath environment variable.

rainersigwald · 2016-11-29T21:07:10Z

src/XMakeBuildEngine/Evaluation/ProjectParser.cs

+                    if (sdkName.Contains("/"))
+                    {
+                        ProjectErrorUtilities.ThrowInvalidProject(element.GetAttributeLocation(XMakeAttributes.sdk),
+                            "InvalidSdkFormatTooManySlashes");


Self, actually add this string to the resx.

dsplaisted · 2016-11-30T01:14:49Z

src/XMakeBuildEngine/Construction/ProjectRootElement.cs

@@ -1714,8 +1714,18 @@ public void Save(Encoding saveEncoding)
                {
                    using (ProjectWriter projectWriter = new ProjectWriter(_projectFileLocation.File, saveEncoding))
                    {
-                        projectWriter.Initialize(XmlDocument);
-                        XmlDocument.Save(projectWriter);
+                        var xmlWithNoImplicits = (XmlDocument)XmlDocument.Clone();


The Clone() method doesn't seem to be available in the .NET Core build. Probably the fix is to switch to CloneNode() instead.

ah, I apparently hadn't committed and pushed that fix when I made it. You're right--the replacement is CloneNode(true).

rainersigwald · 2016-11-30T12:58:48Z

Consensus has emerged that the default value of the SDK location should be {MSBuild directory}\Sdks. See dotnet/sdk#444 for example.

I need to:

Create an internal variable for that
Make a property function to expose it
Make a property with the same name in toolsets
- Full Framework
- .NET Core

rainersigwald · 2016-11-30T13:46:36Z

@dotnet-bot test Windows_NT Build for Desktop please

rainersigwald · 2016-11-30T15:57:26Z

I'm going to rebase -i to a semi-coherent commit structure to resolve the merge to rc2

… the XML tree

…orts (it currently does)

jeffkl · 2016-11-30T16:49:25Z

src/Shared/XMakeAttributes.cs

@@ -40,9 +40,11 @@ internal static class XMakeAttributes
        internal const string taskName = "TaskName";
        internal const string continueOnError = "ContinueOnError";
        internal const string project = "Project";
+        internal const string @implicit = "_Implicit";


Can this be isImplicit so you can get rid of the @? Should the attribute name just be IsImplicit?

We expect to treat versions specified after a slash in the Sdk attribute as a "minimum supported version", but that is not fully designed yet. For the first iteration of Sdk support, ignore the version entirely, but allow it to be specified.

This better matches the MSBuild convention.

cdmihai · 2016-11-30T18:14:42Z

src/Shared/UnitTests/ObjectModelHelpers.cs

@@ -1264,6 +1264,11 @@ internal static string[] CreateFiles(params string[] files)
        /// </summary>
        internal static string[] CreateFilesInDirectory(string rootDirectory, params string[] files)
        {
+            if (files == null)
+            {
+                return null;


return null; [](start = 16, length = 12)

Return empty array so the upstream code can work without checking for null?

…ent folder

cdmihai · 2016-11-30T18:58:37Z

src/XMakeBuildEngine/Definition/ToolsetLocalReader.cs

@@ -57,6 +57,7 @@ protected override IEnumerable<ToolsetPropertyDefinition> ToolsVersions
        protected override IEnumerable<ToolsetPropertyDefinition> GetPropertyDefinitions(string toolsVersion)
        {
            yield return new ToolsetPropertyDefinition(MSBuildConstants.ToolsPath, BuildEnvironmentHelper.Instance.CurrentMSBuildToolsDirectory, _sourceLocation);
+            yield return new ToolsetPropertyDefinition(MSBuildConstants.SdksPath, BuildEnvironmentHelper.Instance.MSBuildSDKsPath, _sourceLocation);


Is this new ToolsetProperty read from anywhere? If it is, it looks like ToolsetConfigurationReader needs to be updated as well to support reading the SDK from the app.config too.
Apparently the local reader is only used on xplat as hardcoded values because there are no app configs on .net core.

Not by MSBuild itself, but the first iteration of some of the magic SDKs plan to do SDK-to-SDK references using the property. It's set here for .NET Core and in app.config for full.

I think you may also have to change ToolsetConfigurationReader to make the property flow from the app config too.

Oh, wait, no, that happens automatically, no need to change anything :)

cdmihai · 2016-11-30T19:05:35Z

src/XMakeBuildEngine/Evaluation/ProjectParser.cs

+
+                var sdksString = element.GetAttribute(XMakeAttributes.sdk);
+
+                var sdks = sdksString.Contains(";") ? sdksString.Split(';') : new[] {sdksString};


sdksString.Split(';') [](start = 54, length = 21)

You could also look into using ExpressionShredder.SplitSemiColonSeparatedList. Though it might be overkill for this situation.

worth thinking about for vnext

The if-exists checks meant that if you had a badly defined MSBuildSDKsPath or an SDK name that didn't exist, you would get no error and just silently get no imports. That's no good. This adds the implicit import unconditionally, requiring all SDKs to have both an Sdk.props and an Sdk.targets for the moment.

Addressed the describe "only blocking issue"

dnfclas added the cla-already-signed label Nov 28, 2016

rainersigwald added the Needs Review label Nov 28, 2016

rainersigwald force-pushed the sdk-imports-virtual-elements branch from fa5cd51 to e6d4397 Compare November 28, 2016 21:16

jeffkl reviewed Nov 28, 2016

View reviewed changes

dsplaisted previously requested changes Nov 29, 2016

View reviewed changes

rainersigwald commented Nov 29, 2016

View reviewed changes

dsplaisted reviewed Nov 30, 2016

View reviewed changes

rainersigwald changed the base branch from xplat to vs15-rc2 November 30, 2016 13:25

rainersigwald force-pushed the sdk-imports-virtual-elements branch 3 times, most recently from b3f2666 to fb6455d Compare November 30, 2016 14:06

rainersigwald and others added 13 commits November 30, 2016 10:28

POC of parser-injection approach to imports

2e08446

Implicit import element

f869b51

make implicitness a property of the import element--PP does not work

b71b619

back to implicit xml element--fixes preprocessor that walks only over…

b107746

… the XML tree

Make Implicit field public so it can be observed from the public OM

d408ca3

Disposable TemporaryEnvironment

7b81fdf

What? you want a TEST?

ef4b933

Do not require additional files in CreateFilesInDirectory

769ce6b

Test to verify that editing via the OM doesn't serialize implicit imp…

0aa0ef0

…orts (it currently does)

Use MSBuildSDKsPath environment variable for path to Sdks

96fd4df

Remove implicit elements from XML at Save() time

e5c9de9

Multiple ;-delimited Sdks in Project attribute

291254d

Use named constant for Sdk attribute

7419c05

rainersigwald force-pushed the sdk-imports-virtual-elements branch from 5b5283b to 7303fcd Compare November 30, 2016 16:40

jeffkl reviewed Nov 30, 2016

View reviewed changes

rainersigwald force-pushed the sdk-imports-virtual-elements branch from 7303fcd to 681b98a Compare November 30, 2016 16:57

rainersigwald added 13 commits November 30, 2016 11:14

[Test Needed] Error on file containing _Implicit attr

e30f3d3

Sdk Preprocessor output test

350e21b

Move ProjectSdkImplicitImport tests to own file

9f11ecf

Use Implicit instead of HasXmlRepresentation

2fa8e4e

Implicit -> IsImplicit

c085463

This better matches the MSBuild convention.

Remove SelectNodes null check (just fail if it goes that wrong)

4dd1af7

Remove misleading ProjectLocation remark

61e14a8

Preprocess Sdk attr for conditioned imports too

f9e09eb

Add string error for badly-formed Sdk attribute

b47e0e0

Clone() isn't available in Core

efdb487

Disable GitVersioning in CI to work around dotnet#1409

d83721a

Default MSBuildSDKsPath location to {tools}\Sdks

9801c6d

rainersigwald force-pushed the sdk-imports-virtual-elements branch from 681b98a to 9801c6d Compare November 30, 2016 17:14

cdmihai reviewed Nov 30, 2016

View reviewed changes

Use MSBuildExtensionsPath-equivalent instead of ToolsPath for Sdk par…

94214d3

…ent folder

rainersigwald force-pushed the sdk-imports-virtual-elements branch from cc2122a to 94214d3 Compare November 30, 2016 18:57

cdmihai reviewed Nov 30, 2016

View reviewed changes

AndyGerlicher approved these changes Nov 30, 2016

View reviewed changes

cdmihai reviewed Nov 30, 2016

View reviewed changes

rainersigwald added 2 commits November 30, 2016 13:10

Use only 2-element Path.Combine for downlevel compat

fd8c656

rainersigwald force-pushed the sdk-imports-virtual-elements branch from eec49d9 to b3f22f8 Compare November 30, 2016 19:15

Fix AddProperty_WithSdk_KeepsSdkAndImplicitImports

2b5610e

rainersigwald merged commit 6490226 into dotnet:vs15-rc2 Nov 30, 2016

rainersigwald removed the Needs Review label Nov 30, 2016

rainersigwald modified the milestone: MSBuild Sdks Dec 4, 2016

rainersigwald mentioned this pull request Dec 12, 2016

Use default imports for Microsoft.Common.props and Microsoft.CSharp.targets dotnet/project-system#628

Closed


		var sdksString = element.GetAttribute(XMakeAttributes.sdk);

		var sdks = sdksString.Contains(";") ? sdksString.Split(';') : new[] {sdksString};

		@@ -1081,7 +1081,6 @@ private void PerformDepthFirstPass(ProjectRootElement currentProjectOrImport)
		}

		ProjectImportElement import = element as ProjectImportElement;

		@@ -1,4 +1,4 @@
		<?xml version="1.0" encoding="utf-8"?>
		<?xml version="1.0" encoding="utf-8"?>

Implicit Sdk imports #1403

Implicit Sdk imports #1403

Conversation

rainersigwald commented Nov 28, 2016

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

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

dsplaisted 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

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

Choose a reason for hiding this comment

rainersigwald commented Nov 30, 2016 • edited Loading

rainersigwald commented Nov 30, 2016

rainersigwald commented Nov 30, 2016

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

rainersigwald commented Nov 30, 2016 •

edited

Loading