Create Chocolatey packages for console #236

CharliePoole · 2017-06-07T16:16:07Z

Fixes #207

This PR incorporates work from PR #234 by @Roemer and adds the following:

There are now two packages, one for console only and one with extensions
File names and package ids are changed
Creation of shims for the agent exes is suppressed - we don't want users to be executing them directly.

I marked this as "Do Not Merge" because I'd really prefer to see the extensions referenced as packages, the way we do for NuGet, rather than actually copied into the package with extensions. I can see the way to do that with Chocolatey, but it will take some more experimentation. Alternatively, we could release what we have and improve it later.

Improving would mean first trying to use the existing nuget packages for the extensions. I know chocolatey will install them, but I don't know how whether it will do so in a way that is useful for us. The fallback would be to modify the nuget packages, creating chocolatey packages. In either case, there would be a file installed with the engine, similar to nunit.nuget.addins, i.e. nunit.choco.addins, which would allow the engine to find the extensions.

So the question is whether to wait for the better solution or just go with what we have now. @rprouse what do you prefer?

CharliePoole · 2017-06-07T16:18:58Z

Just to be clear... in my preferred solution, the user could type...

choco install nunit-console-runner nunit-teamcity-extension

and get the runner with that single extension installed.

CharliePoole · 2017-06-07T16:20:47Z

Hey @ferventcoder we'd love to have your comments on this as well.

Roemer · 2017-06-08T12:10:34Z

build.cake

+		// Copy the nuspec files
+		CopyFileToDirectory("choco/nunit-console-runner.nuspec", currentImageDir);
+		CopyFileToDirectory("choco/nunit-console-with-extensions.nuspec", currentImageDir);
+		CopyFileToDirectory("choco/nunit-agent.exe.ignore", currentImageDir + "bin/");


Wouldn't it be better to just create those empty files here in cake instead of creating them in the solution and copying them here? Or at least just copy all files from the choco directory.

I think it's clearer if they are in the same place as the agent exe and config files. If we do it all from the cake script, then I would favor what you are suggesting.

Personally, I'd agree it would be nice to create them in cake - purely because I'm not too familiar with choco, and just went to look them up, and find out what was in the .ignore files. 😆 That would be more explicit if they were created right here - I assumed files being copied had content.

I'll look at that change and see if I can fit it in relatively smoothly.

Roemer · 2017-06-08T12:12:35Z

choco/nunit-console-runner.nuspec

+    <tags>nunit console runner test testing tdd</tags>
+    <copyright>Copyright (c) 2017 Charlie Poole</copyright>
+  </metadata>
+  <files>


The more I look at it the more i think it will be more maintainable if we have the entire content of the package build by the cake script. Now we have a mix of solution items, lines in nuspec, things copied and generated by cake...

That's the approach I favor too. 😄 Will do!

Part of the difficulty here is the use of the "Image" directory to build everything. This is a hold-over from when the framework and console were in the same repo and we built a complicated structure with various frameworks for different runtime versions. It serves little use here and was never removed purely out of laziness on my part. I may take a look at that, although it's probably better as a separate PR.

CharliePoole · 2017-06-08T15:20:09Z

@rprouse I know it's easy to miss which issues require attention, so... THIS DOES. Your comment to question above is the only thing that is blocking it right now. 😸

Based on your input, I will either merge what we have or work toward the solution I prefer - described above. I also asked you a while back to tell me how you wanted this to relate to the release you are working on - i.e. does it need to be part of it or can it be done a bit later. The two questions are obviously related.

rprouse

From what I know of Chocolatey, this looks good except for the version in the one file, unless you edit that in Cake and I missed it.

As for going with what you have, I think we should keep it simple for now and iterate if we have to.

rprouse · 2017-06-10T13:30:55Z

choco/nunit-console-with-extensions.nuspec

+<package xmlns="http://schemas.microsoft.com/packaging/2015/06/nuspec.xsd">
+  <metadata>
+    <id>nunit-console-with-extensions</id>
+    <version>0.0.0</version>


Doesn't the version need to be a variable here?

I see it gets injected in the Cake file, just thought it should be $version$ like NuGet does it. I could very easily be wrong though 😄

This is how chocolatey generates templates, so it's at least their preferred approach. Don't know if they accept $version$ as well.

Not yet, but we do accept choco pack --version=1.2.3. See chocolatey/choco#1313

This is how chocolatey generates templates, so it's at least their preferred approach.

I'm not sure what version of Chocolatey you are on, but this hasn't been the way that Chocolatey generates templates for over a year. Current version is 0.10.7. and it separates package specifics from software specifics now in generation.

I'm on the latest... however I copied this from another project. 😊 Anyway, I'm running tests using this package, so we're good for now.

CharliePoole · 2017-06-10T19:50:48Z

I'm taking "keep it simple" to mean "don't make changes" - what we have suffers precisely from not being simple. However, it's no worse than what we do in the distribution project. I'll take care of the review comments and merge.

In the longer (even medium) term, I'm convinced that this is not the right way to use chocolatey with self-contained packages. My experience, their docs and my discussions with @ferventcoder have convinced me that use of package references is a better path. Hopefully, whoever does that next iteration will bear it in mind.

Still waiting to here how you want this to fit in with the release. It makes a difference in how this is eventually merged.

rprouse · 2017-06-10T20:09:00Z

I think we can merge this and we can try to work it into the release process, or at least right after the release. I will likely need your help uploading. We do NUnit distribution just after the initial release and amend the GitHub release, we could do the same with this. Let's see how it goes, document the process and then decide if it needs improvements.

CharliePoole · 2017-06-11T01:38:35Z

Rebased on master

ChrisMaddock

Looks good to me. I agree we should have separate packages with dependencies at some point, as NuGet, but don't see the harm in merging this for now, and releasing the latter later.

ChrisMaddock · 2017-06-11T11:52:02Z

build.cake

+				"1.0.2",
+				"teamcity-event-listener.dll",
+				null
+			)


Hardcoded versions for the addins are an extra maintenance cost - the msi just pulls the latest version of the add-in. This could also mean the msi and choco end up containing different extensions for the same version number release - which I realise is possible, but should be avoided, in my opinion.

If no version is set on nugetInstallSettings, does it just fetch latest? Could we do that?

@ChrisMaddock This came out of my directions to @Roemer. To the extent that we might build and distribute extensions written by other people, I thought we needed to be able to specify the version we have tested. RIght now, the only such extension is teamcity, which we don't test in any version. Now that I write that, it sounds bad, doesn't it. 😟

ChrisMaddock · 2017-06-11T11:54:28Z

build.cake

+			var primaryDllPath = System.IO.Path.Combine(addinToolsPath, addin.Item3);
+			CopyFileToDirectory(primaryDllPath, addinsDir);
+			// Copy additional files
+			if (addin.Item4 != null) {


This could be addin?.Item4 to remove the extra nesting? Only minor though. :-)

That would guard against addin being null, not Item4. Currently, the code assumes that addin is not null, which is valid since we create the list here.

Sorry - you're right.

ChrisMaddock · 2017-06-11T12:03:23Z

build.cake

+		// Copy the nuspec files
+		CopyFileToDirectory("choco/nunit-console-runner.nuspec", currentImageDir);
+		CopyFileToDirectory("choco/nunit-console-with-extensions.nuspec", currentImageDir);
+		CopyFileToDirectory("choco/nunit-agent.exe.ignore", currentImageDir + "bin/");


Personally, I'd agree it would be nice to create them in cake - purely because I'm not too familiar with choco, and just went to look them up, and find out what was in the .ignore files. 😆 That would be more explicit if they were created right here - I assumed files being copied had content.

ChrisMaddock · 2017-06-11T12:09:03Z

build.cake

+			// Write the primary dll to the addins file
+			FileAppendLines(System.IO.Path.Combine(currentImageDir, "nunit.engine.addins"), new[] {
+				System.IO.Path.Combine("addins", addin.Item3)
+			});


This is much better than the nunit-distribution handling. 😄

@Roemer 's concept!

FYI - and not particularly to do with these lines of code - Because I can no longer build nunit-console without commenting out the line that detects .NET Core, I'm actually using CI as my primary test bed and making changes more incrementally than I would normally do. Each new commit gets in a bit more to fix the comments that have been made.

ChrisMaddock · 2017-06-11T12:09:46Z

build.cake

@@ -402,6 +404,143 @@ Task("PackageConsole")
        });
    });

+Task("PackageChocolatey")
+	.Description("Creates chocolate packages of the console runner")


chocolate -> Chocolatey. Although chocolate packages would be nice! 😄

ChrisMaddock · 2017-06-11T12:14:56Z

build.cake

+		EnsureDirectoryExists(PACKAGE_DIR);
+
+		// Note: Since cake does not yet support a working directory and separate output directory for chocolatey, the following copying and hacks are needed.
+


Should this comment be attached to lines 480ish? I'm not sure what it's referring to.

@Roemer This came from your branch and I just kept it. I actually don't understand it either.

CharliePoole · 2017-06-14T23:17:31Z

@Roemer @rprouse I think this is as good as it gets without doing some major reoranization on the build script. I'd actually like to do that, but not in this PR.

I'd also like to see a version with extensions that simply referenced them, but I think we should get this one out first.

There are some comments in the code that point out places where I left in duplication that should eventually be removed. That's because I think the script reorganization is needed first.

Please re-review for merging.

ferventcoder · 2017-06-15T12:37:40Z

Late to the conversation but I will reiterate what I mentioned to @CharliePoole in an email here.

When you implement plugins/exensions, you could do it in one of two ways. Either within Chocolatey's lib directory, or gather it together under $env:ChocolateyToolsLocation\nunit - this allows users to determine where that tools location is (defaults to c:\tools). Using the Tools Location does have some drawbacks and considerations, as you are responsible for setting the path and removing that, etc - much of that is currently documented at https://github.com/chocolatey/choco/issues/1303#issuecomment-303804561

I do prefer each plugin as a separate package, but as mentioned here that could be a major restructuring.

ferventcoder · 2017-06-15T12:39:27Z

build.cake

@@ -554,7 +669,8 @@ Task("Package")
    .IsDependentOn("CheckForError")
    .IsDependentOn("PackageEngine")
    .IsDependentOn("PackageConsole")
-    .IsDependentOn("PackageNetStandardEngine");
+    .IsDependentOn("PackageNetStandardEngine")
+	.IsDependentOn("PackageChocolatey");


Small formatting issue.

CharliePoole · 2017-06-15T14:33:22Z

Thanks @ferventcoder for chiming in. I'm going to continue on this and we'll do it in the preferred way when possible. Meanwhile, there may be a first release using what we have so far. Key question is can we rely on relative paths from our engine to any extensions remaining stable over time as Chocolatey develops. We use such paths extensively.

ferventcoder · 2017-06-15T14:36:42Z

Key question is can we rely on relative paths from our engine to any extensions remaining stable over time as Chocolatey develops. We use such paths extensively.

You can't go wrong with $env:ChocolateyInstall\lib\<id> - we have no plans to change it currently. If we ever did it would be a VERY long transition and deprecation before the change was made (with plenty of warnings). So very unlikely.

CharliePoole · 2017-06-15T14:53:57Z

We can adapt. 😄 Here's how we handle both nuget 2 and 3 for example in our addins file:

../../NUnit.Extension.*/**/tools/     # nuget v2 layout
../../../NUnit.Extension.*/**/tools/  # nuget v3 layout

This works well because we actually don't want a copy of the NUnit engine installed with NuGet to be aware of any extensions except those also installed in the project with NuGet.

Similarly, I think an app installed via Chocolatey should only use extensions installed the same way. Eventually, I'd hope to see all of the following separate...

Console runner
Gui Runner
Engine
Each extension to the engine

That would give the two (or more) runners access to the same engine with that engine automatically using all the extensions. It's not something that's so easy to do otherwise.

CharliePoole requested review from Roemer and rprouse June 7, 2017 16:16

Roemer mentioned this pull request Jun 8, 2017

Build choco package for the console runner #234

Closed

Roemer reviewed Jun 8, 2017

View reviewed changes

rprouse approved these changes Jun 10, 2017

View reviewed changes

CharliePoole added 3 commits June 10, 2017 18:37

Merge changes from Roman's fork

f234a31

Rename files and choco ids; add choco package for runner alone

73382ed

Suppress creation of shims for agent exes

0336544

CharliePoole force-pushed the issue-207 branch from b239e18 to 0336544 Compare June 11, 2017 01:37

Remove file names from chocolatey nuspecs and put in build.cake

39c4f83

ChrisMaddock reviewed Jun 11, 2017

View reviewed changes

CharliePoole added 2 commits June 13, 2017 22:49

Restructure chocolatey packaging

d9fa2f1

Explanatory comments

dd1cf32

Roemer approved these changes Jun 15, 2017

View reviewed changes

CharliePoole merged commit a2c43d7 into master Jun 15, 2017

ferventcoder reviewed Jun 15, 2017

View reviewed changes

CharliePoole deleted the issue-207 branch June 15, 2017 13:59

		EnsureDirectoryExists(PACKAGE_DIR);

		// Note: Since cake does not yet support a working directory and separate output directory for chocolatey, the following copying and hacks are needed.

Create Chocolatey packages for console #236

Create Chocolatey packages for console #236

Conversation

CharliePoole commented Jun 7, 2017 • edited Loading

CharliePoole commented Jun 7, 2017

CharliePoole commented Jun 7, 2017

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

CharliePoole commented Jun 8, 2017

rprouse left a 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

CharliePoole commented Jun 10, 2017

rprouse commented Jun 10, 2017

CharliePoole commented Jun 11, 2017

ChrisMaddock left a 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

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

CharliePoole commented Jun 14, 2017

ferventcoder commented Jun 15, 2017

Choose a reason for hiding this comment

CharliePoole commented Jun 15, 2017

ferventcoder commented Jun 15, 2017

CharliePoole commented Jun 15, 2017

CharliePoole commented Jun 7, 2017 •

edited

Loading