-
Notifications
You must be signed in to change notification settings - Fork 4
Conversation
To ensure that this works on Linux, I propose we set up a Travis build. I can do a PR for that and rebase once it's merged. Sound good, @CharliePoole? |
@asbjornu, a Travis build would be good. I will review this when I am at a computer. |
Failure because nuget packages are not installed. We would normally do this in the InitializeBuild task of the build.cake script. I generally do most things in scripts rather than relying on msbuild magic. :-) This build previously had no package dependencies so we should add a NuGetRestore call at the start of InitializeBuild. I'm not seeing where we tell GitVersion how to create version numbers. Isn't there a config file? WRT travis, we should definitely set that up at some point. We have it for all our other builds but this one is purely internal and we only make use of the packages. We will need it for the Debian packagers though, since they require building everything from source. |
@rprouse I'd like to use this project to set the "style" for how we use GitVersion. I'm not entirely convinced that GitVersionTask is the way to go but I figured that once @asbjornu got it set up, we could review and talk about alternatives. The choices seem to be GitVersion.exe, the cake GitVersion tool and GitVersionTask. Of course, we could try different approaches on different projects too. |
@CharliePoole I've fixed the build problem, but as I want to use GitVersion to give AppVeyor its build number, I'm trying to work out the details in Cake's channel on Gitter. I'll keep on adding commits to this as I go and can rebase and squash in the end, if necessary. With regards to how GitVersion creates version numbers; it does so by convention. If we need to deviate from the defaults, we can add a |
// INITIALIZE BUILD | ||
////////////////////////////////////////////////////////////////////// | ||
|
||
Setup(context => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any particular reason to prefer SetUp to a Task here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a Cake expert, but it seems like this is something you'd want to explicitly run no matter what target you choose or how you set the different target dependencies up, for which Setup()
fits the bill quite nicely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, cake is a general scripting language and the Tasks could do anything. For example, we have targets in nunit that just run the tests, without building. That's why we created InitializeBuild. In fact, it doesn't matter much, because (1) everything is conditioned by IsRunningOnAppveyor and (2) there are no targets like that in this project. I think it's mostly a style thing - no need to change what you have done.
@asbjornu Once you get this working and we understand what it does, we can figure out further steps to take. This is a good project to start with because we are the only consumers of the packages. The assembly is included in our public NUnit package rather than depending on this package. |
@asbjornu Of course, we already set AppVeyor's build number in the script. That will have to be removed or replaced. |
I've merged the Travis build PR. You should pull in latest from master to get Travis building here too. |
version = context.EnvironmentVariable("GitVersion_MajorMinorPatch"); | ||
var nugetVersion = context.EnvironmentVariable("GitVersion_NuGetVersion"); | ||
var preReleaseTag = context.EnvironmentVariable("GitVersion_PreReleaseTag"); | ||
var semVersion = context.EnvironmentVariable("GitVersion_LegacySemVerPadded"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
None of this is having any effect so far. I think you need to get these variables in the Package task.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, this is not done. I have not gotten GitVersion working correctly in Cake yet.
Can you explain what is happening to AssemblyInfo.cs and when? Will it happen differently when somebody builds using VS versus doing it on AppVeyor? |
526e2e6
to
a020303
Compare
@CharliePoole Nothing is happening with the checked-in
With GitVersionTask automatically generating the version information compile-time, all of the above questions are moot:
I hope this clarifies things. If not, please let me know! 😃 |
…since Cake will download everything it needs anyway
a020303
to
a8e7b1c
Compare
254e162
to
b0ab67d
Compare
b0ab67d
to
cea174a
Compare
… debug why it doesn't work on AppVeyor.
The Travis build is failing due to GitTools/GitVersion#890 as well as cake-build/cake#1021. We can either wait for those pull requests to be merged and released or we can exclude GitVersion in the Cake script (but not in MSBuild) on Unix for now. I'm trying to figure out why AppVeyor is failing and will continue that quest tomorrow. |
Yes, I was trying to suggest earlier that we not do this for this PR. We have no immediate need to build this project on Linux so we would only be doing it to prove it's possible. OTOH all our other projects actually have to build on Linux. |
@CharliePoole Yea, I'll ignore the Travis failures for now. Did my description of GitVersion above make sense? Questions? 😃 |
8dbdccc
to
ed88802
Compare
@asbjornu Yes, your explanations make sense. I can see the advantage of not having any version info in the source code. Looks like you are building now and did a rebase. I'll go through and re-review the code. |
@@ -3,7 +3,7 @@ configuration: Release | |||
platform: Any CPU | |||
|
|||
build_script: | |||
- ps: .\build.ps1 -Target "Appveyor" | |||
- ps: .\build.ps1 -Target "Appveyor" -Verbosity Diagnostic |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should drop this once we're satisfied it works correctly.
@CharliePoole Thanks for the review. Now that cake-build/cake#1021 is merged and GitTools/GitVersion#890 will hopefully be fixed by next week, it looks like we can have a working Unix build in not too long. Otherwise, I consider this PR done. |
I'll merge in the morning. I think I still had an outstanding question to you about how to tailor this to our own work practices, packaging standards, etc. but i can do that separately after the merge. |
@CharliePoole Awesome! 😃 I'm happy to answer any question anywhere you'd like. If there's anything you'd like to change about the current version number, please let me know and I'll add configuration to customize it. |
I guess the problem I see is that we no longer have a simple statement of how versions are generated and packages are labeled. Previously, you could look at the cake file and see code how we generate the code for version number and package label depending on whether there is a tag, whether it's a PR or not, whether the name of the branch starts with "release",... Now that's all up to internal settings rather than being visible, which means nobody can work on the script without a knowledge of GitVersion internal settings. So I think we want to make the configuration explicit by generating the file, make it match our existing standards and then figure out how we may want to change it. We do want to change it and it's OK if this particular repo is non-standard for a short while - that's why I picked it as the one to start with. So I'll get the process going by merging this and then create a new issue for tailoring it. |
This PR adds GitVersionTask to version the assembly. Fixes #4.