Initial use of GitVersion #226

CharliePoole · 2017-09-01T21:11:10Z

Fixes #206

This is initial implementation of the use of GitVersion to determine the correct version for each build. When complete, it will no longer be necessary to track and update the version of the GUI in the source code or scripts.

The initial commit calculates and displays the version info that would be used but doesn't actually use it. By creating a PR, I'll be able to see how the pre-release tag portion of the version is set for PRs, but the PR should not be merged by anyone but me, after it's working correctly.

Please review and comment on the versioning scheme anyway!

mikkelbu

I'm not sure that I get all the implications of using GitVersion (actually, I'm quite sure that I do not all of it). But so I sounds quite reasonable to me.

mikkelbu · 2017-09-03T18:50:20Z

build.cake

@@ -2,6 +2,8 @@
 #tool nuget:?package=GitVersion.CommandLine
 #addin "Cake.Incubator"

+using System.Text.RegularExpressions;


Why is this necessary ?

I had a regular expression at one point but it's gone now.

mikkelbu · 2017-09-03T18:51:38Z

build.cake

@@ -1,4 +1,8 @@
 #tool nuget:?package=NUnit.ConsoleRunner&version=3.6.1


We are back to using both tabs and spaces for indention in this file.

Yes. I should try to tell VS how to handle this file.

CharliePoole · 2017-09-06T19:41:09Z

@NUnitSoftware/gui-team @ChrisMaddock @rprouse
I think this is in it's final form as far as coding. If you reviewed it earlier, it may be worth another look.

That said, it's still not doing anything but merely displaying what it would do. Next commits will actually begin to impact the packaging.

I'm saving modification of the AssemblyInfo for last. 😃

immeraufdemhund · 2017-09-06T20:01:28Z

pardon if this is a dumb question but is this changing how we do our branching and do our pull requests??? or just how the CI's handle our PR's and branches?

CharliePoole · 2017-09-06T20:13:51Z

It's changing how we version. Normally, when you do a branch and PR, you don't set the version. Our script handles it. But the script depends on the version being coded in it somewhere - as well as in the AssemblyInfo files.

Once we implement this, I'd hope to end up where we no longer have the version in any of our source code and therefore don't need to update it. GitVersion can examine the repository history and decide what version we want, based on some conventions and the settings we establish.

Currently, we create automatic pre-release suffixes for our versions on AppVeyor only, using environment variables they provide. Once this is in place, we'll have a consistent automatic method that works on AppVeyor, Travis and locally.

Initially, final (non-preview) releases will continue to be done manually. But this will enable a later change to do them automatically as well. All that adds up to more frequent automated releases, which has to be a good thing!

CharliePoole · 2017-09-06T23:10:58Z

With the latest commits, we are now using the info from GitVersion to do packaging. The hard-coded version in the script is no longer used.

There is still quite a lot of debug info in the script, which I will remove when it's no longer needed. I'll reformat using spaces everywhere before merge as well.

Note that we can't do a chocolatey package on Linux without a bunch of work, so I'm simply not creating one.

Next step is to deal with the AssemblyInfo files.

CharliePoole · 2017-09-07T12:09:57Z

This is ready for review and merge once CI passes.

mikkelbu · 2017-09-07T15:23:34Z

I'll take a look at it tonight (in approx. 3 hours)

mikkelbu

I only have very minor nitpick and some comments.

mikkelbu · 2017-09-07T19:27:41Z

.vscode/tasks.json

+ {
+    // See https://go.microsoft.com/fwlink/?LinkId=733558
+    // for the documentation about the tasks.json format
+    "version": "2.0.0",


Is this file included on purpose? I can google that it defines a vscode task, but I cannot find the script Package that is mentioned.

That's because I executed tasks in the script under VsCode. File should be in .gitignore.

The script is build.cake, Package is a target.

Ok. The "script": "Package" was a bit misleading 😄

mikkelbu · 2017-09-07T19:54:00Z

build.cake

+#tool nuget:?package=GitVersion.CommandLine
+#addin "Cake.Incubator"
+
+using System.Text.RegularExpressions;


The regex is back for parsing whether it is a PR or not, so we should keep this.

mikkelbu · 2017-09-07T19:59:02Z

build.cake

@@ -167,9 +144,7 @@ Task("PackageZip")
            BIN_DIR + "CHANGES.txt",
            BIN_DIR + "nunit-gui.exe",
            BIN_DIR + "nunit-gui.exe.config",
-            BIN_DIR + "nunit-gui.pdb",


What is the motivation for not providing pdbs ?

They break the build under Mono 4.6.2, which doesn't produce them. We could put them back if we used logic to include them only when they actually are present.

Ok. Let us just leave them out to begin with. Then we can always include them later.

…into issue-206

mikkelbu · 2017-09-08T07:09:22Z

.travis.yml

-  - ./build.sh -target="Travis" -configuration="Release" -verbosity="verbose"
-    
+  - git fetch --unshallow
+  - ./build.sh --target "Travis"


I've pushed a commit b758c79 to correct the error from the merge (the error made Travis fail every time).

mikkelbu · 2017-09-08T07:35:51Z

@CharliePoole .tavis.yml should contain something like
- ./build.sh -target="Travis" -configuration="Release" -verbosity="verbose"
See e.g. b758c79 that made Travis succeed (and sorry for stepping on your toes in this PR, but I did not think you were online)

CharliePoole · 2017-09-08T07:38:39Z

Can you explain further? Is the problem the dash, the missing config, the need for verbosity? Is this because of merging the new Cake bootstrap script? Seems like, if it messes us up, we might not want it.

CharliePoole · 2017-09-08T07:39:41Z

Going to bed... I'll look at this in the morning.

mikkelbu · 2017-09-08T08:38:47Z

When you merged master to this branch no change was made to .tavis.yml (as far as I can tell), even though it was changed on master (I don't know much about GIT merges, but I guess that there must have been a conflict since you had made changes to the file on this branch). So we lost the changes done to .tavis.yml. So I made the commit b758c79 (on this branch) to solve the problem, but I think that you concurrently also tried to solve the problem (see ae9e7a3) and as a result overwrote my change when the to commits were merged in b813c78.

.tavis.yml on master
- ./build.sh -target="Travis" -configuration="Release" -verbosity="verbose"
.tavis.yml on this branch
- ./build.sh -t "Travis"
So -t should be replaced with -target=. The part about configuration and verbosity is just to keep the old behaviour as these are not the default anymore.

You can also see the diff between master and this branch here
https://github.com/NUnitSoftware/nunit-gui/pull/226/files#diff-354f30a63fb0907d4ad57269548329e3

And yes, it is because of the new bootstrap script, but I don't think that it is more problematic than other changes we do (and since we change .travis.yml quite rarely I don't think that it will effect us much).

CharliePoole · 2017-09-08T18:44:28Z

I understand that it would probably work if I put it back to how it was in master. It would also probably work if I backed out the upgraded build.sh script. I'm trying to figure out which is the best course of action.

I take it you are saying you merely used the args from master, and they work. But those args are wrong in several senses. We don't want verbose output. We shouldn't have to specify the configuration, since it is supposed to use our default in build.cake - although specifying it here is no big deal.

@jnm2 Is this related to the changes you made to Cake? If so can you give us info about exactly what changed in the command-line args or point to same?

jnm2 · 2017-09-08T19:19:59Z

@CharliePoole I put all the details in cake-build/resources#33. I was just on Gitter earlier bugging them again to get cake-build/resources#34 validated and merged.

CharliePoole · 2017-09-08T19:49:50Z

@jnm2 The command that was failing for me most recently was

./build.sh -t "Travis"

before that this was failing:

./build.sh --target "Travis"

Is it the quotation marks?

jnm2 · 2017-09-08T20:15:23Z

@CharliePoole nothing has changed WRT quotation marks, thankfully! The issue is that right now the only format build.sh understands is the format Cake.exe understands, which is --target="Foo" and --target=Foo. Note the =.

From the page I linked, this is what you can do right now:

build.sh

-t foo ❌

-t=foo ❌

-t:foo ❌

-target foo ❌

-target:foo ❌

--target foo ❌

--target=foo ✔️

This is what you used to be able to do:

build.sh

-t foo ✔️

-t=foo ❌

-t:foo ❌

-target foo ❌

-target:foo ❌

--target foo ✔️

--target=foo ✔️

Hopefully the PR is merged soon and you can go back to separating with a space. :-/

CharliePoole · 2017-09-08T21:32:30Z

It's the =! Now I get it. 😀

mikkelbu · 2017-09-09T19:35:17Z

I take it you are saying you merely used the args from master, and they work. But those args are wrong in several senses. We don't want verbose output. We shouldn't have to specify the configuration, since it is supposed to use our default in build.cake - although specifying it here is no big deal.

The reason for why I chose set -configuration="Release" -verbosity="verbose" was to keep the old behaviour. The old version of build.sh contained defaults which set configuration=Release and verbosity=verbose, if no arguments were given to build.sh (and we only supplied target).
(see e.g. here https://github.com/NUnitSoftware/nunit-gui/blob/a78c5bf8d240214b55283e25edc5b11f139b4313/build.sh#L13-L19)

jnm2 · 2017-09-09T19:58:23Z

This is the best place to keep defaults:
https://github.com/NUnitSoftware/nunit-gui/blob/e0699e256f7ce72b23c4cb2561122f2ce7199654/build.cake#L10-L11

Cake's template has Release which means someone changed it, even though the change never worked due to the design fault in the the bootstrappers. If you ever customize these, rather than editing in each bootstrapper, it would seem to make the most sense to put your argument defaults in the build script.
Not in the bootstrappers and not in CI server config files. I'd only put argument overrides in CI server configs where the overrides would be relevant and keep defaults in build.cake where they are most relevant.

CharliePoole · 2017-09-09T20:59:42Z

@mikkelbu Yes, I now understand that. It's a breaking change on the part of the Cake folks. I agree with how you compensated and put back in the configuration arg. Problem is, these updates come out without any release info on the part of Cake.

@jnm2 Wrong. 😄 We got Debug as a default when we ran Cake directly and Release when we ran build.ps1 or build.sh. I worked for quite a while using Cake directly locally and only using the bootstrap scripts in CI. I always thought it was weird that we defaulted to Release in one case and Debug in the other, but I got used to it and our CI scripts came to depend on it.

I agree it makes the most sense for the bootstrap scripts to be neutral in terms of defaults that are passed to Cake. It's just a change from the past and not one that's easy to notice in all cases.

jnm2 · 2017-09-09T21:54:11Z

My bad. Thanks for filling me in on the history behind it.

CharliePoole force-pushed the issue-206 branch 3 times, most recently from b9d9052 to 5576afa Compare September 2, 2017 17:40

Initial use of GitVersion

aa54405

CharliePoole force-pushed the issue-206 branch 4 times, most recently from 76aafa6 to c6dbe5c Compare September 3, 2017 02:59

Extract PR number

975875c

CharliePoole force-pushed the issue-206 branch from c6dbe5c to 975875c Compare September 3, 2017 04:34

mikkelbu reviewed Sep 3, 2017

View reviewed changes

CharliePoole added 2 commits September 5, 2017 09:50

Add branch name to ci builds

b486c27

Fix errors and refactor so properties are all set in constructor

10abbd8

CharliePoole force-pushed the issue-206 branch from 0bd7763 to 10abbd8 Compare September 6, 2017 19:30

CharliePoole added 2 commits September 6, 2017 14:19

Use GitVer version for packaging locally and on Travis

d261fec

Use GitVer version for packaging on AppVeyor

87bb6ca

CharliePoole force-pushed the issue-206 branch from 94ec480 to 87bb6ca Compare September 6, 2017 22:33

Exclude pdb files from zip for now

a0ef16a

CharliePoole force-pushed the issue-206 branch from aa1a7c1 to 5312802 Compare September 7, 2017 11:15

Update CommonAssemblyInfo for CI builds

d9606c0

CharliePoole force-pushed the issue-206 branch from 4f5c353 to d9606c0 Compare September 7, 2017 11:42

Remove Debugging info from script

e08bac4

CharliePoole force-pushed the issue-206 branch from 4538d04 to e08bac4 Compare September 7, 2017 12:33

mikkelbu approved these changes Sep 7, 2017

View reviewed changes

CharliePoole added 3 commits September 7, 2017 16:56

Merge branch 'master' into issue-206

c3e8757

Ignore files in .vscode dir

68781f8

Merge branch 'issue-206' of https://github.com/CharliePoole/nunit-gui …

924e2f5

…into issue-206

mikkelbu reviewed Sep 8, 2017

View reviewed changes

Fix build.sh arguments

62a1cfd

CharliePoole force-pushed the issue-206 branch from b813c78 to 62a1cfd Compare September 8, 2017 21:40

CharliePoole merged commit e0699e2 into master Sep 8, 2017

CharliePoole deleted the issue-206 branch September 8, 2017 22:36

		@@ -1,4 +1,8 @@
		#tool nuget:?package=NUnit.ConsoleRunner&version=3.6.1

Initial use of GitVersion #226

Initial use of GitVersion #226

Conversation

CharliePoole commented Sep 1, 2017 • edited Loading

mikkelbu 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

CharliePoole commented Sep 6, 2017

immeraufdemhund commented Sep 6, 2017

CharliePoole commented Sep 6, 2017

CharliePoole commented Sep 6, 2017

CharliePoole commented Sep 7, 2017

mikkelbu commented Sep 7, 2017

mikkelbu 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

mikkelbu commented Sep 8, 2017 • edited Loading

CharliePoole commented Sep 8, 2017

CharliePoole commented Sep 8, 2017

mikkelbu commented Sep 8, 2017 • edited Loading

CharliePoole commented Sep 8, 2017

jnm2 commented Sep 8, 2017 • edited Loading

CharliePoole commented Sep 8, 2017

jnm2 commented Sep 8, 2017 • edited Loading

CharliePoole commented Sep 8, 2017

mikkelbu commented Sep 9, 2017

jnm2 commented Sep 9, 2017 • edited Loading

CharliePoole commented Sep 9, 2017

jnm2 commented Sep 9, 2017

CharliePoole commented Sep 1, 2017 •

edited

Loading

mikkelbu commented Sep 8, 2017 •

edited

Loading

mikkelbu commented Sep 8, 2017 •

edited

Loading

jnm2 commented Sep 8, 2017 •

edited

Loading

jnm2 commented Sep 8, 2017 •

edited

Loading

jnm2 commented Sep 9, 2017 •

edited

Loading