Adds Generators solution to build #448

jakauppila · 2017-10-01T22:28:34Z

Added the Generators solution to build.cake. Includes downloading Nuget.exe to build.ps1.

The only thing I'm not sure of at this point is how to to perform the NuGetRestore on Linux for Travis; I assume this will involve utilizing Mono.

Fixes #441

jpreese · 2017-10-01T22:38:46Z

build.cake

+    .IsDependentOn("CopyGenerators")
+    .Does(() => {
+        var generatorsProject = GetFiles(generatorsBuildDir + "/*.csproj");
+        Parallel.ForEach(generatorsProject, parallelOptions, (project) => NuGetRestore(project.FullPath));


We shouldn't need to call NuGetResore() explicitly since DotNetCoreBuild() will do an implicit restore for us.

jpreese · 2017-10-01T22:39:05Z

build.cake

+    .Does(() => {
+        var generatorsProject = GetFiles(generatorsBuildDir + "/*.csproj");
+        Parallel.ForEach(generatorsProject, parallelOptions, (project) => NuGetRestore(project.FullPath));
+        Parallel.ForEach(generatorsProject, parallelOptions, (project) => DotNetBuild(project.FullPath));


This should be DotNetCoreBuild as this is a .NET Core 2 project.

The generators solution contains one project, so we shouldn't need to run this build step in parallel.

jpreese · 2017-10-01T22:41:11Z

build.cake

+    .Does(() => {
+        CopyDirectory($"{generatorsSourceDir}/", $"{generatorsBuildDir}");
+    });
+


We don't perform any file replacements in the generators project like we do in the exercises (replacing the example with the stub). There shouldn't be any need to copy the generators directory.

jpreese · 2017-10-01T22:45:58Z

build.ps1

+
+(New-Object System.Net.WebClient).DownloadFile($NuGetExeUri, $NuGetExePath);
+
+###########################################################################


Travis already has NuGet on the CI server, so we shouldn't need to download it.

jpreese · 2017-10-01T22:46:37Z

build.ps1

+
+(New-Object System.Net.WebClient).DownloadFile($NuGetExeUri, $NuGetExePath);
+
+###########################################################################


Travis already has NuGet, so we shouldn't need to install it.

jpreese · 2017-10-01T22:47:44Z

Thanks for working on this, @jakauppila ! I've requested some changes for your PR. If you have any questions, please do not hesitate to ask.

jakauppila · 2017-10-02T02:05:29Z

@jpreese Made the requested changes, Travis is now green!

jpreese · 2017-10-02T02:36:45Z

Excellent, thanks!

ErikSchierboom

Great work! I've requested a couple of small changes. Once those are in, this will be ready to merge!

ErikSchierboom · 2017-10-02T05:59:07Z

build.cake

+var exercisesSourceDir = "./exercises";
+var exercisesBuildDir  = "./build/exercises";
+
+var generatorsSourceDir = "./generators";


This variable is only ever used in a single place, so I think it would be better to remove this variable and just use "./generators/Generators.csproj" in the BuildGeneratorsSolution step.

ErikSchierboom · 2017-10-02T06:03:46Z

build.cake

@@ -5,7 +5,11 @@ using System.Threading.Tasks;
 var target = Argument("target", "Default");
 var exercise = Argument<string>("exercise", null);

-var sourceDir = "./exercises";
+var exercisesSourceDir = "./exercises";


I don't think we need both an exercisesSourceDir and buildDir variable, as the only thing that is built there are the exercises. I think we could just revert to the old situation here, where the exercise directory are directly placed and built in the ./build directory. I do think I like your naming better, so I would like to see something like this:

var exercisesSourceDir = "./exercises"; var exercisesBuildDir = "./build";

And then you could drop the buildDir parameter and replace it with exercisesBuildDir wherever it is used.

ErikSchierboom · 2017-10-02T06:04:22Z

build.cake

        Parallel.ForEach(allProjects, parallelOptions, (project) => DotNetCoreTest(project.FullPath));
    });

+Task("BuildGeneratorsSolution")


Could you change this to BuildGenerators? The solution part is just an implementation detail.

jakauppila · 2017-10-03T05:22:10Z

@ErikSchierboom Requested changes have been made.

ErikSchierboom · 2017-10-03T07:07:41Z

@jakauppila Looking great! Merged. Thanks a lot. 🎉

jpreese suggested changes Oct 1, 2017

View reviewed changes

jakauppila force-pushed the fix_441 branch from e4423a7 to 12e57c6 Compare October 2, 2017 01:47

jpreese approved these changes Oct 2, 2017

View reviewed changes

ErikSchierboom requested changes Oct 2, 2017

View reviewed changes

Adds building of the Generators solution

83b321d

jakauppila force-pushed the fix_441 branch from 12e57c6 to 83b321d Compare October 3, 2017 05:21

ErikSchierboom merged commit 3db8306 into exercism:master Oct 3, 2017


		(New-Object System.Net.WebClient).DownloadFile($NuGetExeUri, $NuGetExePath);

		###########################################################################

Uh oh!

Adds Generators solution to build #448

Adds Generators solution to build #448

Uh oh!

Conversation

jakauppila commented Oct 1, 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!

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!

jpreese commented Oct 1, 2017

Uh oh!

jakauppila commented Oct 2, 2017

Uh oh!

jpreese commented Oct 2, 2017

Uh oh!

ErikSchierboom 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!

jakauppila commented Oct 3, 2017

Uh oh!

ErikSchierboom commented Oct 3, 2017

Uh oh!

Uh oh!