Add aggregatePackage module by kingerja · Pull Request #80 · CommonBuildToolset/CBT.Modules

kingerja · 2017-04-27T01:53:40Z

Create a basic aggregate package option where the user can set a property like.
<CBTAggregatePackage>AAG_TEST=d:\tmp\aag\src*d:\tmp\aag\src2|d:\tmp\aag\rem1</CBTAggregatePackage>

And it will use copy d:\tmp\agg\src then overlay d:\tmp\aag\src2 then remove from that overlay any files with names that match in d:\tmp\aag\rem1.

Pay attention to the comments there are many issues we need to consider with this approach.

To address Issue #60

jeffkl

Overall this looks great

jeffkl · 2017-05-01T20:04:38Z

+<?xml version="1.0" encoding="utf-8"?>
+<Project ToolsVersion="14.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
+  <PropertyGroup Label="Configuration">
+    <Description>Provides Global Package expansion for CBT.Nuget.</Description>


Update this description.

jeffkl · 2017-05-01T20:38:51Z

+
+  <PropertyGroup>
+    <RestoreNuGetPackagesDependsOn>$(RestoreNuGetPackagesDependsOn);AggregateNugetPackages</RestoreNuGetPackagesDependsOn>
+    <RestoreDependsOn>$(RestoreDependsOn);AggregateNugetPackages</RestoreDependsOn>


You shouldn't need this if you're setting RestoreNuGetPackagesDependsOn. If it needs to be set, I'd say that's a bug.

This appears to be an issue in the globalpackages as well.
....\src\CBT.NuGet.GlobalRestore\CBT\Module\After.Microsoft.Common.targets: RestoreGlobalNuGetPackages;$(RestoreDependsOn)

I have filed an issue.

jeffkl · 2017-05-01T20:40:51Z

+
+  <PropertyGroup Condition=" '$(CBTEnableAggregatePackageRestore)' == 'true' "> 
+    <CBTNuGetAggregatePackagePropertyFile Condition=" '$(CBTNuGetAggregatePackagePropertyFile)' == '' ">$(IntermediateOutputPath)\AggregatePackages.props</CBTNuGetAggregatePackagePropertyFile> 
+    <CBTNuGetAggregatePackagePropertiesCreated>$(CBTNuGetTasksAssemblyPath.GetType().Assembly.GetType('System.AppDomain').GetProperty('CurrentDomain').GetValue(null).CreateInstanceFromAndUnwrap($(CBTNuGetTasksAssemblyPath), 'CBT.NuGet.Tasks.AggregatePackages').Execute('$(CBTAggregateDestPackageRoot)', '$(CBTAggregatePackage)', '$(CBTNuGetAggregatePackagePropertyFile)'))</CBTNuGetAggregatePackagePropertiesCreated> 


We'll need to figure out how to make this work in Visual Studio...

It would be neat if we could come up with a generic pattern or solution for these hacks something where it would do a popup in vs saying doing custom cbt action'somecustomerrormessageforaction'.

jeffkl · 2017-05-01T20:42:36Z

+            Remove,
+        }
+
+        public AggregatePackage(string outPropertyId, List<KeyValuePair<AggregateOption, string>> packagesToAggregate, string destinationRoot)


Is packagesToAggregate a List<T> because it can contain duplicates or can it be an IDictionary<TKey, TValue>?

I was just creating an ordered list of tuple pairs with <Action, Folder> action being add or remove from aggregate and folder being the folder that should be added to the aggregate or have it's contents removed from the aggregate.

Instead of using Tuple I just used a KVP which technically isn't correct. If I were to make the folder path a key then that could be unique but I didn't feel the need to remove duplicates as I didn't want to have to reason about which was the right one to remove. And I didn't feel the need to throw an error on a duplicate entry as it would be a harmless authoring error that could be intentional, (though wasted time) due to prior constraints of the project.

Anyway let me know if you have a better object structure in mind.

jeffkl · 2017-05-01T20:43:41Z

+        {
+            OutPropertyId = outPropertyId;
+            PackagesToAggregate = packagesToAggregate;
+            OutPropertyValue = Path.Combine(destinationRoot, string.Format("{0}.{1}", outPropertyId, GetOutputPropertyHash()));


No more String.Format(), long live string interpolation

OutPropertyValue = Path.Combine(destinationRoot, $"{outPropertyId}.{GetOutputPropertyHash()}");

jeffkl · 2017-05-01T20:49:32Z

+            }
+        }
+
+        public bool CreateAggregatePackage()


This method needs some logging.

Yea I don't disagree with that. Is there a clean pattern you can think of to do logging with out having to make this AggregatePackage object aware of the msbuild logger by passing it in. Perhaps in this case that is a little over engineering.

We should probably just move this method to the AggregatePackages task and leave the AggregatePackage class as just a data structure.

jeffkl · 2017-05-01T20:52:01Z

+
+        public override bool Execute()
+        {
+            BuildEngine = new CBTBuildEngine();


You shouldn't need to create a new CBTBuildEngine in this case since this code is running when the task is running as a real task. Set this on line 91 instead

not sure why I had that there. Thanks

jeffkl · 2017-05-01T20:52:18Z

+            BuildEngine = new CBTBuildEngine();
+            Dictionary<string, string> propertiesToCreate = new Dictionary<string, string>();
+
+            foreach (var aggPkg in PackagesToAggregate.Split(';'))


Be sure to ignore empty entries and trim here

the string is nullorwhitespace should handle the null,empty,and whitespace scenario all of which I am not interested in. However I have added a splitoptions to remoteemptyentries as well.

jeffkl · 2017-05-01T20:52:57Z

+                {
+                    continue;
+                }
+                var pkg = AggregatePackage.ParseIntoPackage(aggPkg, AggregateDestRoot);


I like the TryParse pattern since its a little more standard. What do you think?

standards are good. ;)

jeffkl · 2017-05-01T20:53:26Z

+                catch (DirectoryNotFoundException exp)
+                {
+                    Log.LogError(exp.Message);
+                    throw;


This should probably not throw right?

… and detect content changes.

…iss60

kingerja · 2017-05-03T01:02:23Z

+                // We don't care about directories in aggregation since removing directories is not possible given current implementation.
+                // This is in the scenario where the user aggregates against a nuget package and a checked in folder under source control that contains a config file or whatever.
+                // Concern on aggregating very large folders as this is disk IO.
+                foreach (var f in Directory.GetFiles(pkg.Folder, "*.*", SearchOption.AllDirectories))


Perhaps instead of by default computing the last write time on the files being aggregated we put this behind a flag that is set to off unless they enable it to true or we detect a folder to aggregate is not under the package root.

Because imagine if every project had to parse over the msbuild.corext package to compute it's hash folder.

The scenario I am getting at is to avoid incorrect hard to debug build errors.

At the very least we should use EnumerateFiles instead of GetFiles. I definitely would like to find a more efficient way of hashing the aggregated packages. Could it just be a hash of the package names?

Actually I have an idea. I will send an update. Basically we only care about non immutable folders. So we can pass a list of immutable roots and that can be a ; delimited list of paths and by default that can be the nuget root. This would resolve the issue in the 99% case. In the 1% case the user has the escape hatch to say their immutable folder is really a non-immutable one.

kingerja · 2017-05-03T01:08:00Z

I believe I have addressed your comments. There is one perf concern I have out to you for input. So please review again and take a look.

jeffkl · 2017-05-03T14:34:53Z

+                // We don't care about directories in aggregation since removing directories is not possible given current implementation.
+                // This is in the scenario where the user aggregates against a nuget package and a checked in folder under source control that contains a config file or whatever.
+                // Concern on aggregating very large folders as this is disk IO.
+                foreach (var f in Directory.GetFiles(pkg.Folder, "*.*", SearchOption.AllDirectories))


At the very least we should use EnumerateFiles instead of GetFiles. I definitely would like to find a more efficient way of hashing the aggregated packages. Could it just be a hash of the package names?

jeffkl · 2017-05-03T14:36:11Z

+                throw new DirectoryNotFoundException("Source Directory does not exist or could not be found: " + sourceDirName);
+            }
+            DirectoryInfo dir = new DirectoryInfo(sourceDirName);
+            DirectoryInfo[] dirs = dir.GetDirectories();


Use EnumerateDirectories instead and EnumerateFiles instead of GetFiles below. We should probably revisit this method and see if Async copies or parallelism would help here.

jeffkl · 2017-05-03T14:37:11Z

+            project.Save(propsFile);
+        }
+
+        internal IEnumerable<AggregatePackage> ParsePackagesToAggregate()


You should include unit tests with this pull request or open an issue to write some for this sprint.

Add aggregatePackage module

0be07e8

kingerja requested a review from josemesona April 27, 2017 01:53

jeffkl suggested changes May 1, 2017

View reviewed changes

Josh Kingery and others added 5 commits May 2, 2017 13:51

Refactor aggregate package options

9b32d99

Refactor and change hash function for folder name to avoid collisions…

6a0e8b5

… and detect content changes.

Merge branch 'master' into iss60

b1059a4

Merge branch 'master' into iss60

bcfcaa4

Merge branch 'iss60' of https://github.com/kingerja/CBT.Modules into …

1bd3381

…iss60

kingerja commented May 3, 2017

View reviewed changes

Josh Kingery added 3 commits May 2, 2017 18:10

Merge branch 'master' into iss60

3eca220

Merge branch 'master' into iss60

663f955

wire up CBTParseError

f8f4426

jeffkl approved these changes May 3, 2017

View reviewed changes

Do not date hash known immutable roots.

e9eea63

kingerja merged commit f98f09a into CommonBuildToolset:master May 3, 2017

kingerja deleted the iss60 branch May 3, 2017 19:03

Conversation

kingerja commented Apr 27, 2017

Uh oh!

jeffkl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kingerja commented May 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants