Project properties simplification #136

andrew-boyarshin · 2018-07-16T13:48:32Z

Import and adapt parts of Microsoft.Build.Evaluation for precise Condition attribute parsing [MIT]
Create ConditionEvaluator to glue Project2015To2017 and MSBuild expression trees
Add sophisticated simplification routine to ProjectPropertiesReader to reduce duplication of defaults (handling variety of cases, including non-default project-wide property override and default conditionalpropertygroup-wide override)
ProjectPropertiesReader: replace hacky configuration list retrieval routine with a better one using ConditionEvaluator
Add Project.Platforms list property retrieval to match Project.Configurations
[breaking change] Real support for multiple TargetFrameworks (Project.TargetFrameworks: IReadOnlyList -> IList, now always non-null, set accessor removed)
Do not write default values of $(Configurations) and $(Platforms) in ProjectWriter
Update tests to reflect changes (e.g. changed type of Project.TargetFrameworks)
Create a number of tests to showcase and verify simplification behavior

andrew-boyarshin · 2018-07-16T13:55:35Z

Progress for issue #113

andrew-boyarshin · 2018-07-22T16:20:49Z

I have reimplemented ConditionEvaluator and some simplification parts.
I use simplified Microsoft.Build.Evaluation sources (MIT, yay!) to provide perfect Condition property parsing (matching real MSBuild of course). It can also provide proper condition evaluation for possible future needs. Shunting-Yard and RPN stack evaluator are no longer needed (it is far outmatched by real expression parsing).
I have also improved property simplification routine, targeting code clarity and handling additional cases.
I expect the code to be ready for review in 1 day. I am going to separate everything in multiple PRs this time to simplify reviewing.

andrew-boyarshin · 2018-07-23T15:41:29Z

Project2015To2017Tests/ProjectPropertiesReadTest.cs

@@ -596,5 +793,15 @@ public class ProjectPropertiesReadTest

 			return project;
 		}
+
+		internal static bool ValidateChildren(IEnumerable<XElement> value, params string[] expected)


Intentionally internal. Used in another PR's tests.

Now it is used in this PR (XamlTransformationTest).

andrew-boyarshin · 2018-07-23T15:57:36Z

@hvanbakel @mungojam All 3 PRs are ready to be reviewed and merged. They can work separately, order of merging is not fixed (although natural is #136 -> #138 -> #139).

hvanbakel · 2018-07-23T16:26:10Z

I'll have a look, soon. Thanks for the work.

…

On Mon, Jul 23, 2018 at 8:57 AM Andrew Boyarshin ***@***.***> wrote: @hvanbakel <https://github.com/hvanbakel> @mungojam <https://github.com/mungojam> All 3 PRs are ready to be reviewed and merged. They can work separately, order of merging is not fixed. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#136 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AD8FPCMCXyXY-lbH7lnxT_vA2hhZHmNPks5uJfJwgaJpZM4VRKQa> .

hvanbakel · 2018-07-25T03:01:18Z

Project2015To2017/Reading/Conditionals/CharacterUtilities.cs

+
+        static internal bool IsHexAlphabetic(char candidate)
+        {
+            return (candidate == 'a' || candidate == 'b' || candidate == 'c' || candidate == 'd' || candidate == 'e' || candidate == 'f' ||


return (candidate >= 97 && candidate <= 102) || (candidate >= 65 && candidate <= 70)
would work too and is probably a lot faster

Well, that's MSBuild. :) I really don't think we should bring in our own modifications except those to reduce dependencies on MSBuild code. We might later want to update code from upstream so such changes might introduce confusion.

Interesting, can we include a link to the source somewhere here in case things get outdated?

I have introduced upstream.md in *Reading* directory.

I think the answer is no, but would https://github.com/daveaglick/Buildalyzer be of any use here to avoid copying the code in? It would be a bit too hefty a dependency probably and may not actually help. Or are git sub-modules another possibility to directly bring in the code from MS Build? I've never actually used them before.

@mungojam unfortunately, we cannot bring in the code directly. It requires removal of specific code, non-trivial replacements in a couple of places so that there are no dependencies introduced. And since we don't want our code to bring in the whole enormous dependency on MSBuild and its dependencies and .NET Core SDK and Microsoft SDKs and NuGet, Buildalyzer (that does exactly the thing) can be of no use to us.

hvanbakel · 2018-07-25T03:02:51Z

Project2015To2017/Definition/Project.cs

 		public IReadOnlyList<XElement> OtherPropertyGroups { get; set; }

-		public IReadOnlyList<string> TargetFrameworks { get; set; }
+		public IList<string> TargetFrameworks { get; } = new List<string>();


This doesn't need to be mutable, right? You read it once and from that point on it's immutable.

But I do add new items there. First, it is used in ProjectReader in 2nd PR. then, it is used in ProjectPropertiesReader for TargetFrameworkVersion. I believe mutability is nothing compared to how comparatively ugly things would be with constructing new colection, preserving old ones and adding new.

I see what you mean, I didn't see that change in this PR, that's where my comment came from. Looking at your other PR it makes sense.

hvanbakel · 2018-07-25T03:04:22Z

Project2015To2017/Reading/ConditionEvaluator.cs

+	{
+		#region MSBuild Conditional routine
+
+		private static readonly Lazy<Regex> SinglePropertyRegex = new Lazy<Regex>(


The cost of initializing this is not worth the Lazy construct?

MSBuild code again (probably to make no-op actions faster). But this is indeed sth I could change. Will do.

hvanbakel · 2018-07-25T03:57:49Z

Looks good in general, a few comments I don't know if you want to update your PR to reflect those or if you want me to fix those when I merge it down?

I'll also be doing a few style changes (I very much dislike if blocks without braces) but that's taste.

hvanbakel · 2018-07-25T04:13:02Z

Now that the other one is merged you should be able to simplify https://github.com/hvanbakel/CsprojToVs2017/blob/master/Project2015To2017/Reading/ProjectReader.cs?#L104

andrew-boyarshin · 2018-07-25T04:35:17Z

OK, I am working on this PR (expected timeframe — 4 hours). Will also rebase on latest HEAD. I will also include XamlTransformationTest since 2nd PR is merged now.

* Import and adapt parts of Microsoft.Build.Evaluation for precise Condition attribute parsing [MIT] * Create ConditionEvaluator to glue Project2015To2017 and MSBuild expression trees * Add sophisticated simplification routine to ProjectPropertiesReader * ProjectPropertiesReader: replace hacky configuration list retrieval routine with a better one using ConditionEvaluator * Add Project.Platforms list property retrieval to match Project.Configurations * Real support for multiple TargetFrameworks (Project.TargetFrameworks: IReadOnlyList<T> -> IList<T>) * Do not write default values of $(Configurations) and $(Platforms) in ProjectWriter * Update tests to reflect changes (e.g. changed type of Project.TargetFrameworks) * Create a number of tests to showcase and verify simplification behavior

andrew-boyarshin · 2018-07-25T06:02:56Z

@hvanbakel done! Also added upstream.md to highlight the essentials of MSBuild code import.

andrew-boyarshin · 2018-07-25T06:05:45Z

Project2015To2017/Reading/ProjectPropertiesReader.cs

+						}
+
+						// Default configuration-specific paths
+						// todo: move duplicated paths from conditional groups to non-conditional one


That is a serious matter for different PR. I have a good idea how to implement this one though.

andrew-boyarshin · 2018-07-25T06:24:42Z

Forgot to enable conditional block in ProjectReader you've mentioned. Will do in next PR.

andrew-boyarshin force-pushed the project_simplification_and_wpf branch from 43d38c3 to 99a330d Compare July 23, 2018 15:36

andrew-boyarshin changed the title ~~Property simplification + use MSBuild.Sdks.Extras for WPF & WinForms~~ Project properties simplification Jul 23, 2018

andrew-boyarshin commented Jul 23, 2018

View reviewed changes

hvanbakel reviewed Jul 25, 2018

View reviewed changes

andrew-boyarshin force-pushed the project_simplification_and_wpf branch from 99a330d to fb12198 Compare July 25, 2018 06:01

andrew-boyarshin commented Jul 25, 2018

View reviewed changes

This was referenced Jul 25, 2018

Debug/Release sections that are probably covered by Sdk are left in project file #113

Closed

Evaluate (and perform) csproj format update rubberduck-vba/Rubberduck#3944

Closed

hvanbakel merged commit fb12198 into hvanbakel:master Jul 26, 2018

andrew-boyarshin deleted the project_simplification_and_wpf branch July 26, 2018 06:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Project properties simplification #136

Project properties simplification #136

andrew-boyarshin commented Jul 16, 2018 •

edited

andrew-boyarshin commented Jul 16, 2018 •

edited

andrew-boyarshin commented Jul 22, 2018

andrew-boyarshin Jul 23, 2018

andrew-boyarshin Jul 25, 2018

andrew-boyarshin commented Jul 23, 2018 •

edited

hvanbakel commented Jul 23, 2018 via email

hvanbakel Jul 25, 2018

andrew-boyarshin Jul 25, 2018

hvanbakel Jul 25, 2018

andrew-boyarshin Jul 25, 2018 •

edited

mungojam Jul 25, 2018

andrew-boyarshin Jul 25, 2018

hvanbakel Jul 25, 2018

andrew-boyarshin Jul 25, 2018

hvanbakel Jul 25, 2018

hvanbakel Jul 25, 2018

andrew-boyarshin Jul 25, 2018

hvanbakel commented Jul 25, 2018

hvanbakel commented Jul 25, 2018

andrew-boyarshin commented Jul 25, 2018

andrew-boyarshin commented Jul 25, 2018 •

edited

andrew-boyarshin Jul 25, 2018

andrew-boyarshin commented Jul 25, 2018

Project properties simplification #136

Project properties simplification #136

Conversation

andrew-boyarshin commented Jul 16, 2018 • edited

andrew-boyarshin commented Jul 16, 2018 • edited

andrew-boyarshin commented Jul 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrew-boyarshin commented Jul 23, 2018 • edited

hvanbakel commented Jul 23, 2018 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrew-boyarshin Jul 25, 2018 • edited

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

hvanbakel commented Jul 25, 2018

hvanbakel commented Jul 25, 2018

andrew-boyarshin commented Jul 25, 2018

andrew-boyarshin commented Jul 25, 2018 • edited

Choose a reason for hiding this comment

andrew-boyarshin commented Jul 25, 2018

andrew-boyarshin commented Jul 16, 2018 •

edited

andrew-boyarshin commented Jul 16, 2018 •

edited

andrew-boyarshin commented Jul 23, 2018 •

edited

andrew-boyarshin Jul 25, 2018 •

edited

andrew-boyarshin commented Jul 25, 2018 •

edited