Add new configuration/platform providers #1771

RaulPerez1 · 2017-03-16T23:21:07Z

Creating a new PR since the original PR was pre dogfood changes and the merge is a mess.

This consumes the latest version of CPS which has the changes that allows us to define configuration and platform via Configurations and Platform properties, e.g.

Debug;Release
AnyCPU

#694

There are few workarounds to juggle around coordinating insertions in 3 repos (this, SDK and MsBuild):

SDK changes: dotnet/sdk#949
MsBuild changes: dotnet/msbuild#1879

This tracks the work to remove the workarounds once the above insertions have happened

davkean · 2017-03-21T01:14:16Z

Is this ready to review? #Resolved

RaulPerez1 · 2017-03-21T02:00:48Z

Yup, this is the PR with the changes post CPS insertion. #Resolved

basoundr · 2017-03-17T17:32:40Z

build/Targets/References.targets

@@ -89,7 +89,7 @@
    <When Condition="'$(IncludeCPSReferences)' == 'true'">
      <ItemGroup>

-        <PackageReference Include="Microsoft.VisualStudio.ProjectSystem.SDK" Version="15.0.737" PrivateAssets="None" />
+        <PackageReference Include="Microsoft.VisualStudio.ProjectSystem.SDK" Version="15.3.5-pre-g4382fbdb8f" PrivateAssets="None" />


This needs a follow-up change to update the dependency in the Nuget and the change the release version to a pre release version for 15.3 Nugets #Resolved

This work is done #1810 #Resolved

I'll merge latest, this is most likely the merge conflict that is now showing. #Resolved

basoundr · 2017-03-21T17:54:39Z

...ystem.Managed.VS/ProjectSystem/VS/Configuration/BaseProjectConfigurationDimensionProvider.cs

+            else
+            {
+                // First value is the default one.
+                var defaultValues = ImmutableArray.CreateBuilder<KeyValuePair<string, string>>();


This method returns the default value which should be just one value. We dont need to return IEnumerable<T>, just T should be enough. #ByDesign

basoundr · 2017-03-21T17:57:13Z

...ystem.Managed.VS/ProjectSystem/VS/Configuration/BaseProjectConfigurationDimensionProvider.cs

+        /// </summary>
+        /// <param name="unconfiguredProject">Unconfigured project.</param>
+        /// <returns>Collection of key/value pairs for the defaults values for the configuration dimensions of this provider for given project.</returns>
+        public virtual async Task<IEnumerable<KeyValuePair<string, string>>> GetDefaultValuesForDimensionsAsync(UnconfiguredProject unconfiguredProject)


Should we return a KeyValuePair here? Instead, we can just return the value and if the consumer is interested then we can expose the dimension through a property on the Provider rather than baking that information everytime the call is made. Your call here. #Resolved

It's an exsiting CPS interface so there's not much we can change there without making a changeo n the interface. I know this is extra confusing though I had to debug through it a few times to understand what it expects. The reason why it's an IEnumerable instead of a single value is that your provider can return multiple dimensions. So for example if instead of having one provider for config and one for platform we could have one that does both and you'd need to return at that point 2 values, one for the default config and one for the default platform. #Resolved

basoundr · 2017-03-21T18:05:04Z

...ystem.Managed.VS/ProjectSystem/VS/Configuration/BaseProjectConfigurationDimensionProvider.cs

+        }
+
+        /// <summary>
+        /// Gets the project configuration dimensions for the given unconfigured project.


The description of this method is confusing. This method returns the value(s) for a given dimension in a project. The existing description, Gets the project configuration dimensions..., and also the method name, seems to suggest that we are providing the different dimensions itself, For eg: Configurations, Platforms, rather than the value for those dimensions. #Resolved

Same explanation as the other one regarding the name, and return values, this is part of the CPS interface (IProjectConfigurationDimensionsProvider). I used the xml doc comments for the interface however I can change ours to be more descriptive if that helps. BTW it does return both the dimension(s) and the value(s), it just happens that we return a single one though. #Resolved

basoundr · 2017-03-21T18:05:28Z

...ystem.Managed.VS/ProjectSystem/VS/Configuration/BaseProjectConfigurationDimensionProvider.cs

+        /// </summary>
+        /// <param name="unconfiguredProject">Unconfigured project.</param>
+        /// <returns>Collection of key/value pairs for the current values for the configuration dimensions of this provider for given project.</returns>
+        public virtual async Task<IEnumerable<KeyValuePair<string, IEnumerable<string>>>> GetProjectConfigurationDimensionsAsync(UnconfiguredProject unconfiguredProject)


The same comment as above, do we need KeyValuePair? #Resolved

See the explanation above. #Resolved

basoundr · 2017-03-21T18:37:58Z

...aged.VS/ProjectSystem/VS/Configuration/ConfigurationProjectConfigurationDimensionProvider.cs

+        /// </summary>
+        /// <param name="unconfiguredProject">Unconfigured project for which the configuration change.</param>
+        /// <param name="configurationName">Name of the new configuration.</param>
+        /// <returns>If the method succeeds, it returns S_OK. If it fails, it returns an error code.</returns>


This method is just returning Task. Please update the description #Resolved

Good catch, one of the initial implementations was using IVsCfgProvider and that was the doc for that interface. I'll go over all the comments for Add/Remove/Rename and make sure they are updated properly. #Resolved

basoundr · 2017-03-21T18:38:04Z

...aged.VS/ProjectSystem/VS/Configuration/ConfigurationProjectConfigurationDimensionProvider.cs

+        /// </summary>
+        /// <param name="unconfiguredProject">Unconfigured project for which the configuration change.</param>
+        /// <param name="configurationName">Name of the deleted configuration.</param>
+        /// <returns>If the method succeeds, it returns S_OK. If it fails, it returns an error code.</returns>


This method is just returning Task. Please update the description #Resolved

basoundr · 2017-03-21T18:39:39Z

src/Microsoft.VisualStudio.ProjectSystem.Managed/MsBuildUtilities.cs

+            StringBuilder value = new StringBuilder();
+            foreach (string propertyValue in GetPropertyValues(evaluatedPropertyValue, delimiter))
+            {
+                value.Append(propertyValue == oldValue ? newValue : propertyValue);


Use string.Compare #Resolved

basoundr · 2017-03-21T18:41:36Z

src/Microsoft.VisualStudio.ProjectSystem.Managed/MsBuildUtilities.cs

+        /// <param name="newValue">New property value.</param>
+        /// <param name="delimiter">Character used to delimit the property values.</param>
+        /// <remarks>
+        /// If the property is not present it will be added. This means that the evaluated property


Is the case of remark, where the old value not being found, handled? #Resolved

I think you're asking about the value not the property itself right? Three options -

Do a no-op, can't rename something that doesn't exist in the first place.

Throw an error so that it can be handled upstream

Treat it as an add and append the value at the end

From a truly generic point of view the second option makes more sense. I went with the first though because this is (currently) used in the limited scope of something that's guaranteed to exist (because the config manager dialog is blocking so you can't edit the file at the same time as you're doing edits). I don't feel strongly about it though and I'd be happy to change it to either of the two other options if that sounds better.
#Resolved

I would prefer option 2 since this indicates data mismatch. Either CPS or us should throw en error when we encounter this.

basoundr · 2017-03-21T18:47:23Z

...m.Managed.VS/ProjectSystem/VS/Configuration/PlatformProjectConfigurationDimensionProvider.cs

+        /// <param name="unconfiguredProject">Unconfigured project for which the configuration change.</param>
+        /// <param name="platformName">Name of the new platform.</param>
+        /// <returns>If the method succeeds, it returns S_OK. If it fails, it returns an error code.</returns>
+        private async Task OnPlatformAdded(UnconfiguredProject unconfiguredProject, string platformName)


Both OnPlatformAdded and OnPlatformDeleted can be abstracted away to the base class and used by Platform and Configuration Provider #Pending

I left it like this with adding telemetry in mind but thinking about it some more that should still be easily doable even with them being in the base class. I'll address this comment when I add the telemetry calls. #Pending

basoundr · 2017-03-21T19:09:52Z

src/Microsoft.VisualStudio.ProjectSystem.Managed.UnitTests/MsBuild/MsBuildUtilitiesTest.cs

+        {
+            using (var project = new MsBuildTempProjectFile())
+            {
+                MsBuildUtilities.GetOrAddProperty(project.Project, "MyProperty");


You also get hold of the ProjectPropertyElement which you could inspect. #ByDesign

That one was intentional. I wanted to validate the project itself rather than the individual element to circle back and make sure that the element did really get added. #ByDesign

basoundr · 2017-03-21T19:11:33Z

src/Microsoft.VisualStudio.ProjectSystem.Managed.UnitTests/MsBuild/MsBuildUtilitiesTest.cs

+                MsBuildUtilities.GetOrAddProperty(project.Project, "MyProperty");
+                Assert.Equal(1, project.Project.Properties.Count);
+                Assert.Equal(1, project.Project.PropertyGroups.Count);
+                var group = project.Project.PropertyGroups.First();


Nit: Line breaks after a bunch of asserts tells the user the rest of the test is testing a different component. Improves readability. #Resolved

basoundr · 2017-03-21T19:17:08Z

src/Microsoft.VisualStudio.ProjectSystem.Managed.UnitTests/MsBuild/MsBuildTempProjectFile.cs

+
+namespace Microsoft.VisualStudio.ProjectSystem
+{
+    public class MsBuildTempProjectFile : IDisposable


Nit: MsBuildProjectFile is fine too since this is already part of the UnitTests #Resolved

basoundr · 2017-03-21T19:19:34Z

...sts/ProjectSystem/VS/Configuration/ConfigurationProjectConfigurationDimensionProviderTest.cs

+        }
+
+        [Fact]
+        public async void ConfigurationProjectConfigurationDimensionProvider_OnDimensionValueChanged_Rename()


Missing case where the oldValue is not present. #Resolved

I didn't add it (or other negative tests for remove/rename) because those operations are currently a no-op and from a full integration point of view you can't get into that state (the provider is the source of truth so the data would have to get corrupted on the consumer before the request to rename comes back). That being said, if we do change the behavior of the utilities functions I'll go ahead and add tests for that. #Resolved

basoundr · 2017-03-21T19:21:04Z

src/Microsoft.VisualStudio.ProjectSystem.Managed/MsBuildUtilities.cs

+        /// <param name="propertyValue">Value to remove from the property.</param>
+        /// <param name="delimiter">Character used to delimit the property values.</param>
+        /// <remarks>
+        /// If the property is not present it will be added. This means that the evaluated property


The remark seems wrong. #Resolved

what is the expected behavior when we dont find the value that we are expected to find? This will represent the case where the data is corrupted. We should throw some exception or log the error to investigate why the data mismatch #Resolved

Look at my explanation on the other method. Same as there I don't feel strongly about it either way so I'm open to changing it to whatever makes the most sense. #Resolved

basoundr · 2017-03-21T19:23:59Z

src/Microsoft.VisualStudio.ProjectSystem.Managed.UnitTests/MsBuild/MsBuildUtilitiesTest.cs

@@ -0,0 +1,362 @@
+// Copyright (c) Microsoft.  All Rights Reserved.  Licensed under the Apache License, Version 2.0.  See License.txt in the project root for license information.


Nit: The test files end with Tests.cs #Resolved

basoundr · 2017-03-21T19:29:26Z

@RaulPerez1 Also a general question. When we get a rename event for Configuration, we modify the value in the Msbuild Construction Model. How would this change affect the conditions that were using the older Configuration value? Wouldn't those conditions break now that the old value does not exist? #Resolved

RaulPerez1 · 2017-03-21T20:27:06Z

@basoundr - Regarding the rename question - There's a LOT of code on the CPS side that handles conditions for all 3 operations (rename/delete/add) #Resolved

RaulPerez1 · 2017-03-22T00:28:43Z

@basoundr - I updated some of the documentation, specially around the CPS interface consumption. Let me know if that makes it clear or if it needs more explanations in there. Also there's a few open questions.

basoundr · 2017-03-22T16:03:56Z

src/Microsoft.VisualStudio.ProjectSystem.Managed/MsBuildUtilities.cs

+namespace Microsoft.VisualStudio.ProjectSystem
+{
+    /// <summary>
+    /// Utitlies class to manipulate MsBuild projects.


basoundr · 2017-03-22T16:09:04Z

src/Microsoft.VisualStudio.ProjectSystem.Managed/MsBuildUtilities.cs

+        /// <param name="newValue">New property value.</param>
+        /// <param name="delimiter">Character used to delimit the property values.</param>
+        /// <remarks>
+        /// If the property is not present it will be added. This means that the evaluated property


I would prefer option 2 since this indicates data mismatch. Either CPS or us should throw en error when we encounter this.

basoundr · 2017-03-22T16:20:49Z

src/Microsoft.VisualStudio.ProjectSystem.Managed/MsBuildUtilities.cs

+
+            return project.Properties
+                .Where(p => string.Equals(p.Name, propertyName, StringComparison.OrdinalIgnoreCase))
+                .FirstOrDefault();


The dimensions that we deal here are Platform and Configuration, which are usually not usually conditioned. But modifying a Msbuild Construction model does not ensure correctness. These are one-time operation or user-triggered operations, for which we can take a perf hit.

To ensure correctness you should be using this instead. This gives the XML element of the project property which is actually part of the Msbuild Evaluation Model rather than the Construction model. If we do this, then we dont have to worry whether the property is conditioned or not. It just works

…issing.

RaulPerez1 · 2017-03-22T20:02:38Z

@nguerrera - Can you take a look at the resource file change? Looks like VS made a bunch of changes on top of my 2 new strings, including some version change. Will that cause any issues? If so I can hand craft the changes instead of letting the designer do it for me.

srivatsn · 2017-03-24T21:46:14Z

I took a brief look and this looks fine to me. @basoundr if you sign off, then @RaulPerez1 go ahead and checkin. If @davkean has any comments we can follow up afterwards.

Add new configuration/platform providers

3dbc4e8

dnfclas added the cla-already-signed label Mar 16, 2017

This was referenced Mar 16, 2017

Remove workarounds on design time targets for configurations #1772

Closed

Config properties #1591

Closed

Add telemetry for configuration/platform changes #1773

Open

Add VSI test for configuration and platform changes #1774

Open

basoundr mentioned this pull request Mar 21, 2017

Move the Nuget Version value from hard coded value to a variable #1810

Merged

basoundr reviewed Mar 21, 2017

View reviewed changes

Raul Perez added 2 commits March 21, 2017 17:16

Clarify documentation and style changes

df0e255

Merge latest

74fc520

basoundr reviewed Mar 22, 2017

View reviewed changes

Add error conditions for Remove/Rename when the property values are m…

d854a8d

…issing.

basoundr approved these changes Mar 24, 2017

View reviewed changes

RaulPerez1 merged commit 3227bf9 into dotnet:master Mar 24, 2017

		@@ -0,0 +1,362 @@
		// Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

Add new configuration/platform providers #1771

Add new configuration/platform providers #1771

Conversation

RaulPerez1 commented Mar 16, 2017 • edited Loading

davkean commented Mar 21, 2017 • edited by RaulPerez1 Loading

RaulPerez1 commented Mar 21, 2017 • edited Loading

basoundr Mar 17, 2017 • edited by RaulPerez1 Loading

Choose a reason for hiding this comment

basoundr Mar 21, 2017 • edited by RaulPerez1 Loading

Choose a reason for hiding this comment

RaulPerez1 Mar 21, 2017 • edited Loading

Choose a reason for hiding this comment

basoundr Mar 21, 2017 • edited by RaulPerez1 Loading

Choose a reason for hiding this comment

basoundr Mar 21, 2017 • edited by RaulPerez1 Loading

Choose a reason for hiding this comment

RaulPerez1 Mar 21, 2017 • edited Loading

Choose a reason for hiding this comment

basoundr Mar 21, 2017 • edited by RaulPerez1 Loading

Choose a reason for hiding this comment

RaulPerez1 Mar 21, 2017 • edited Loading

Choose a reason for hiding this comment

basoundr Mar 21, 2017 • edited by RaulPerez1 Loading

Choose a reason for hiding this comment

RaulPerez1 Mar 21, 2017 • edited Loading

Choose a reason for hiding this comment

basoundr Mar 21, 2017 • edited by RaulPerez1 Loading

Choose a reason for hiding this comment

RaulPerez1 Mar 21, 2017 • edited Loading

Choose a reason for hiding this comment

basoundr Mar 21, 2017 • edited by RaulPerez1 Loading

Choose a reason for hiding this comment

basoundr Mar 21, 2017 • edited by RaulPerez1 Loading

Choose a reason for hiding this comment

basoundr Mar 21, 2017 • edited by RaulPerez1 Loading

Choose a reason for hiding this comment

RaulPerez1 Mar 21, 2017 • edited Loading

Choose a reason for hiding this comment

basoundr Mar 22, 2017

Choose a reason for hiding this comment

basoundr Mar 21, 2017 • edited by RaulPerez1 Loading

Choose a reason for hiding this comment

RaulPerez1 Mar 21, 2017 • edited Loading

Choose a reason for hiding this comment

basoundr Mar 21, 2017 • edited by RaulPerez1 Loading

Choose a reason for hiding this comment

RaulPerez1 Mar 21, 2017 • edited Loading

Choose a reason for hiding this comment

basoundr Mar 21, 2017 • edited by RaulPerez1 Loading

Choose a reason for hiding this comment

basoundr Mar 21, 2017 • edited by RaulPerez1 Loading

Choose a reason for hiding this comment

basoundr Mar 21, 2017 • edited by RaulPerez1 Loading

Choose a reason for hiding this comment

RaulPerez1 Mar 21, 2017 • edited Loading

Choose a reason for hiding this comment

basoundr Mar 21, 2017 • edited by RaulPerez1 Loading

Choose a reason for hiding this comment

basoundr Mar 21, 2017 • edited by RaulPerez1 Loading

Choose a reason for hiding this comment

RaulPerez1 Mar 21, 2017 • edited Loading

Choose a reason for hiding this comment

basoundr Mar 21, 2017 • edited by RaulPerez1 Loading

Choose a reason for hiding this comment

basoundr commented Mar 21, 2017 • edited by RaulPerez1 Loading

RaulPerez1 commented Mar 21, 2017 • edited Loading

RaulPerez1 commented Mar 22, 2017

basoundr Mar 22, 2017

Choose a reason for hiding this comment

basoundr Mar 22, 2017

Choose a reason for hiding this comment

basoundr Mar 22, 2017

Choose a reason for hiding this comment

RaulPerez1 commented Mar 22, 2017

srivatsn commented Mar 24, 2017

RaulPerez1 commented Mar 16, 2017 •

edited

Loading

davkean commented Mar 21, 2017 •

edited by RaulPerez1

Loading

RaulPerez1 commented Mar 21, 2017 •

edited

Loading

basoundr Mar 17, 2017 •

edited by RaulPerez1

Loading

basoundr Mar 21, 2017 •

edited by RaulPerez1

Loading

RaulPerez1 Mar 21, 2017 •

edited

Loading

basoundr Mar 21, 2017 •

edited by RaulPerez1

Loading

basoundr Mar 21, 2017 •

edited by RaulPerez1

Loading

RaulPerez1 Mar 21, 2017 •

edited

Loading

basoundr Mar 21, 2017 •

edited by RaulPerez1

Loading

RaulPerez1 Mar 21, 2017 •

edited

Loading

basoundr Mar 21, 2017 •

edited by RaulPerez1

Loading

RaulPerez1 Mar 21, 2017 •

edited

Loading

basoundr Mar 21, 2017 •

edited by RaulPerez1

Loading

RaulPerez1 Mar 21, 2017 •

edited

Loading

basoundr Mar 21, 2017 •

edited by RaulPerez1

Loading

basoundr Mar 21, 2017 •

edited by RaulPerez1

Loading

basoundr Mar 21, 2017 •

edited by RaulPerez1

Loading

RaulPerez1 Mar 21, 2017 •

edited

Loading

basoundr Mar 21, 2017 •

edited by RaulPerez1

Loading

RaulPerez1 Mar 21, 2017 •

edited

Loading

basoundr Mar 21, 2017 •

edited by RaulPerez1

Loading

RaulPerez1 Mar 21, 2017 •

edited

Loading

basoundr Mar 21, 2017 •

edited by RaulPerez1

Loading

basoundr Mar 21, 2017 •

edited by RaulPerez1

Loading

basoundr Mar 21, 2017 •

edited by RaulPerez1

Loading

RaulPerez1 Mar 21, 2017 •

edited

Loading

basoundr Mar 21, 2017 •

edited by RaulPerez1

Loading

basoundr Mar 21, 2017 •

edited by RaulPerez1

Loading

RaulPerez1 Mar 21, 2017 •

edited

Loading

basoundr Mar 21, 2017 •

edited by RaulPerez1

Loading

basoundr commented Mar 21, 2017 •

edited by RaulPerez1

Loading

RaulPerez1 commented Mar 21, 2017 •

edited

Loading