Implementation of implicit imports during evaluation instead of parsing #1492

jeffkl · 2016-12-16T21:41:56Z

Had to do some hacky stuff in the preprocessor but all of that code is private and we can make it better in the future.

jeffkl · 2016-12-16T22:09:22Z

@dotnet-bot test Windows_NT Build for Desktop please

AndyGerlicher · 2016-12-18T00:22:42Z

src/XMakeBuildEngine/Construction/ProjectElementContainer.cs

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


This looks like a change in behavior. Does it need to happen for Remove just above this? Or others?

This is removed because I'm reverting the original implementation where the stuff is added to the XML DOM. There is no IsImplicit property anymore so the if is removed.

The change above this is removed for the same reason.

AndyGerlicher · 2016-12-18T00:24:19Z

src/XMakeBuildEngine/Construction/ProjectRootElement.cs

@@ -655,6 +650,25 @@ public string InitialTargets
        }

        /// <summary>
+        /// 


Fill in the xmldoc

AndyGerlicher · 2016-12-18T00:24:33Z

src/XMakeBuildEngine/Construction/ProjectRootElement.cs

@@ -849,6 +861,14 @@ public ElementLocation InitialTargetsLocation
        }

        /// <summary>
+        /// 


dsplaisted

Looks good, but will this affect CPS and its understanding of provenance? Does the object model expose enough information for CPS to see the implicit imports and to know whether they are at the top or bottom of the file?CPS needs this to be able to correctly manipulate project files. For example, if the implicitly imported props have <Compile Include="**\*.cs" />, then CPS needs to understand this if an item is excluded from the project, renamed, etc.

Also, I'd suggest updating Evaluator.GetCurrentDirectoryForConditionEvaluation to return the Sdk directory if there is an Sdk attribute on an import, so that you can have something like Condition="Exists('Sdk\Sdk.props')" on an Sdk import.

jeffkl · 2016-12-20T21:10:50Z

@dsplaisted They imports show up in the Project but not in the ProjectRootElement. When you new up a Project, the evaluation occurs and the imports are added without a location. The properties and items do show they are "imported" which I'm hoping is enough for CPS. I'm not familiar enough with CPS to know all of the implications, do you know if them showing up in the Project but not the XML representation will break CPS?

The Sdk attribute on the ProjectRootElement is how they can be controlled but that's the only thing exposed.

cdmihai · 2016-12-20T23:10:49Z

@jviau, is this OK for CPS (see last two comments)?

jviau · 2016-12-21T00:43:23Z

@jeffkl @cdmihai CPS uses ElementLocation at the ProjectRootElement location for determining how to modify the project file. So this will be a breaking behavior change for us. With this change, for a ProjectItemElement at the ProjectRootElement level, how would we tell if it is from an SDK import, and if it is the top import or the bottom import?

cdmihai · 2016-12-21T00:47:07Z

src/XMakeBuildEngine/Construction/ProjectRootElement.cs

+            [DebuggerStepThrough]
+            get
+            {
+                return ProjectXmlUtilities.GetAttributeValue(XmlElement, XMakeAttributes.sdk);


As things get more involved with the SDKs, it might be worth creating a new SDKProjectElement which is represented as an attribute (reusing the way metadata as attributes represent themselves). This type would be a valid child for ProjectRootElements and ProjectImportElements. Then, the new type could gain responsibilities such as parsing the semicolon delimited string into a {name, version} object, doing data validation, etc

I'm in favor of putting duplicated logic in a central place, but here I don't think it's helpful to represent it as a child element expressed as an attribute. That just complicates the object model without any benefit that I see, and we can centralize the logic into utility methods or similar without changing the object model.

cdmihai · 2016-12-21T00:50:12Z

src/XMakeBuildEngine/Evaluation/Evaluator.cs

+            {
+                // SDK imports are added implicitly where they are evaluated at the top and bottom as if they are in the XML
+                //
+                foreach (string sdk in currentProjectOrImport.Sdk.Split(new[] { ';' }, StringSplitOptions.RemoveEmptyEntries)


This splitting logic (and the subsequent name / version parsing) is an example of what could go in a new SDKProjectElement type, since most likely other parts of the codebase may want to parse the SDK string (or third party API users).

cdmihai · 2016-12-21T01:01:59Z

src/XMakeBuildEngine/Evaluation/ProjectParser.cs

@@ -187,61 +187,6 @@ private void ParseProjectElement(XmlElementWithLocation element)
            // so we have to set it now
            _project.SetProjectRootElementFromParser(element, _project);

-            if (element.HasAttribute(XMakeAttributes.sdk))


Shouldn't this method (and the other thing that can have SDKs, ParseImportElement too) do some SDK string validation, such as ensure it's not empty? Better to crash here than when the SDK is used in the evaluator / preprocessor.

cdmihai · 2016-12-21T01:06:14Z

src/XMakeBuildEngine/UnitTestsPublicOM/Construction/ProjectSdkImplicitImport_Tests.cs

@@ -29,24 +30,36 @@ public void SdkImportsAreInImportList()
                string sdkPropsPath = Path.Combine(testSdkDirectory, "Sdk.props");
                string sdkTargetsPath = Path.Combine(testSdkDirectory, "Sdk.targets");

-                File.WriteAllText(sdkPropsPath, "<Project />");


Can you also do a ProjectSdkExplicitImport_Tests class to test out the explicit SDK imports? Or rename this class to refer to both implcit and explicit SDK elements.

cdmihai · 2016-12-21T01:07:07Z

src/XMakeBuildEngine/UnitTestsPublicOM/Construction/ProjectSdkImplicitImport_Tests.cs

@@ -6,6 +6,7 @@
 using System.IO;
 using System.Xml;
 using Microsoft.Build.Construction;
+using Microsoft.Build.Evaluation;


Can you also add some invalid SDK tests, like empty SDKs, or SDK names with slash in them?

cdmihai

LGTM after the ProjectParser validates the SDK strings and there's tests for those situations.

jeffkl · 2017-01-03T17:14:22Z

@dsplaisted

will this affect CPS and its understanding of provenance?

I need to understand CPS more to answer this. If it uses the evaluation APIs first, the imports show up in the graph and everything has the IsImported property flag indicating that it came from an import. If it uses the construction APIs for reading the project, then I might have to rethink more of this. I've pinged Jacob to get more info but perhaps you can fill me in?

Also, I'd suggest updating Evaluator.GetCurrentDirectoryForConditionEvaluation to return the Sdk directory if there is an Sdk attribute on an import, so that you can have something like Condition="Exists('Sdk\Sdk.props')" on an Sdk import.

Good idea, I'll do this as part of the SDK resolver work and I've added your comment as a note on the Design doc #1493

jeffkl · 2017-01-03T17:21:34Z

@cdmihai The ProjectParser does not parse any of the ProjectRootElement attributes (InitialTargets, DefaultTargets, etc). They are all read by the evaluator as raw XML strings. Let's talk in person about your comments when you're back.

…g parsing of projects. Had to do some hacky stuff in the preprocessor but all of that code is private and we can make it better in the future. Closes dotnet#1447

Added a unit test for an invalid SDK name Made ImplicitImportLocation public

… list like "One; ;Two"

jeffkl · 2017-01-09T19:24:59Z

@dotnet-bot test OSX Build for Mono please

rainersigwald · 2017-01-17T19:33:21Z

Related to #1493 and #1392.

dnfclas added the cla-already-signed label Dec 16, 2016

Sarabeth-Jaffe-Microsoft added the Needs Review label Dec 16, 2016

AndyGerlicher reviewed Dec 18, 2016

View reviewed changes

AndyGerlicher approved these changes Dec 20, 2016

View reviewed changes

dsplaisted reviewed Dec 20, 2016

View reviewed changes

jeffkl mentioned this pull request Dec 20, 2016

Import from SDKs #1400

Closed

cdmihai reviewed Dec 21, 2016

View reviewed changes

cdmihai suggested changes Dec 21, 2016

View reviewed changes

jeffkl added 6 commits January 5, 2017 07:51

Implementation of implicit imports during evaluation instead of durin…

13ad21e

…g parsing of projects. Had to do some hacky stuff in the preprocessor but all of that code is private and we can make it better in the future. Closes dotnet#1447

Add missing XML doc comments

c4736ca

Refactor a little

479d25c

Added a unit test for an invalid SDK name Made ImplicitImportLocation public

Fix broken unit test

f6601ed

Thow an exception if the SDK attribute contains an empty value in the…

8b83d81

… list like "One; ;Two"

More tests!

98a3d6f

jeffkl force-pushed the implicitImports branch from 8da92e7 to 98a3d6f Compare January 5, 2017 15:58

AndyGerlicher approved these changes Jan 5, 2017

View reviewed changes

cdmihai approved these changes Jan 6, 2017

View reviewed changes

AndyGerlicher merged commit e8b8f2c into dotnet:xplat Jan 10, 2017

Sarabeth-Jaffe-Microsoft removed the Needs Review label Jan 10, 2017

rainersigwald mentioned this pull request Jan 13, 2017

GetLogicalProject() doesn't return elements imported from Sdks #1570

Closed

jeffkl deleted the implicitImports branch January 17, 2017 21:16

rainersigwald mentioned this pull request Feb 10, 2017

Implicit top & bottom SDK imports #1392

Closed

rainersigwald mentioned this pull request Jul 25, 2017

MSBuild Construction APIs should allow interrogating and modifying SDKs [Implicit Imports] #1429

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementation of implicit imports during evaluation instead of parsing #1492

Implementation of implicit imports during evaluation instead of parsing #1492

jeffkl commented Dec 16, 2016

jeffkl commented Dec 16, 2016

AndyGerlicher Dec 18, 2016

jeffkl Dec 18, 2016

AndyGerlicher Dec 18, 2016

AndyGerlicher Dec 18, 2016

dsplaisted left a comment

jeffkl commented Dec 20, 2016

cdmihai commented Dec 20, 2016

jviau commented Dec 21, 2016

cdmihai Dec 21, 2016

dsplaisted Dec 21, 2016

cdmihai Dec 21, 2016 •

edited

Loading

cdmihai Dec 21, 2016 •

edited

Loading

cdmihai Dec 21, 2016 •

edited

Loading

cdmihai Dec 21, 2016

cdmihai left a comment

jeffkl commented Jan 3, 2017

jeffkl commented Jan 3, 2017

jeffkl commented Jan 9, 2017

rainersigwald commented Jan 17, 2017 •

edited

Loading

Implementation of implicit imports during evaluation instead of parsing #1492

Implementation of implicit imports during evaluation instead of parsing #1492

Conversation

jeffkl commented Dec 16, 2016

jeffkl commented Dec 16, 2016

AndyGerlicher Dec 18, 2016

Choose a reason for hiding this comment

jeffkl Dec 18, 2016

Choose a reason for hiding this comment

AndyGerlicher Dec 18, 2016

Choose a reason for hiding this comment

AndyGerlicher Dec 18, 2016

Choose a reason for hiding this comment

dsplaisted left a comment

Choose a reason for hiding this comment

jeffkl commented Dec 20, 2016

cdmihai commented Dec 20, 2016

jviau commented Dec 21, 2016

cdmihai Dec 21, 2016

Choose a reason for hiding this comment

dsplaisted Dec 21, 2016

Choose a reason for hiding this comment

cdmihai Dec 21, 2016 • edited Loading

Choose a reason for hiding this comment

cdmihai Dec 21, 2016 • edited Loading

Choose a reason for hiding this comment

cdmihai Dec 21, 2016 • edited Loading

Choose a reason for hiding this comment

cdmihai Dec 21, 2016

Choose a reason for hiding this comment

cdmihai left a comment

Choose a reason for hiding this comment

jeffkl commented Jan 3, 2017

jeffkl commented Jan 3, 2017

jeffkl commented Jan 9, 2017

rainersigwald commented Jan 17, 2017 • edited Loading

cdmihai Dec 21, 2016 •

edited

Loading

cdmihai Dec 21, 2016 •

edited

Loading

cdmihai Dec 21, 2016 •

edited

Loading

rainersigwald commented Jan 17, 2017 •

edited

Loading