Rework buildscripts for easier management #749

zvirja · 2017-04-07T22:28:33Z

I've reworked build scripts and CI configuration to simplify release management. Now current package version is calculated automatically based on the last tag and number of commits since tag. Also I've configured both CI and our build.fsx to automatically push packages to appropriate NuGet feeds.

The matrix is following (0 means any digit):

Trigger	Result Version	Action
Add tag `v0.0.0` or `v.0.0.0-RC0`	`0.0.0` or `0.0.0-RC0`	Build, push to NuGet and MyGet feeds
Add tag `v0.0.0-aplha0` (any pre suffix)	`0.0.0-aplha0`	Build, push to MyGet feed
Push 2 commits after tag `v0.0.0` to master/v4	`0.0.0.2`	Build, push to MyGet feed
Create a PR	-	Build

Other changes:

Update FAKE to latest version. Switch to FAKE.Core package usage as it takes less space.
Add tasks to Fake script to calculate Git version, patch assembly version attributes and push to NuGet.
Add AssemblyInformationalVersion to each assembly.
Move CI configuration to appveyor.yml
Add production keys to appveyor.yml
Recreated project in AppVeyor, because old project didn't work with appveyor.yml. Likely, that is because of this.

After I merge this PR to master, I'll merge it to v4 and will tune it a bit for .NET Core projects (e.g. change VS image version).

P.S. Large number of changes is caused by FAKE update. The only interesting changes might be build.fsx, appveyor.yml and AssemblyInfo.cs files.

moodmosaic · 2017-04-08T03:31:30Z

Now current package version is calculated automatically based on the last tag and number of commits since tag.

What does this mean regarding Semantic Versioning? (AutoFixture follows Semantic Versioning 2.0.0)

zvirja · 2017-04-08T05:47:13Z

@moodmosaic A good question. Likely, it was too wide statement so it caused some confusion :) Let me describe why I added it.

I wanted to simplify version management, so it's controlled by tags only. Idea is that if you create a tag, it's used as a version for your assemblies. Additionally I wanted to add support for the Dev feed to have a place which we could consume before a new release is tagged and published to NuGet. Feed might be more suitable as CI artifacts as you can consume it by VS easier.
Both this things required a mechanism to calculate the current assembly version based on repo status, so I added this feature.

For the Production feed (NuGet) the SemVer is never violated - we push on tags only. For such cases number of commits since last tag is always zero. Release manager will manage tags according to SemVer, so it will be honored.

For the Dev feed (MyGet) SemVer will not be violated for the v4 branch, as I'm going to push a tag v4.0.0-alpha at the beginning of our work (or just hardcode version on CI).
However it might be violated for the Dev feed and the master branch after we push commits since last tag. Example: we create a tag v3.7.1 and merge a PR to master after that. The next artifact on Dev feed will have version e.g. 3.7.1.2- this violates SemVer as bugfix or a new feature might appear. However I'd say this is fine as the Dev feed is not designed for the public usage. Rather, it's a feed where you can find the latest artifacts and it will be used in rare cases only. In this case you are aware that you are using something internal.

It would be very redundant to track SemVer for the Dev feed, so we 2 options only - disable Dev feed for the Master branch or have it knowning that this is a private feed with latest artifacts. I'd vote for the last option as such a feed might be very useful sometimes.

zvirja · 2017-04-09T14:05:46Z

I've just rebased my code to remove doc for FAKE from repo - it takes about 10MB :)

Again, most changes were done for a few files only, so likely you could focus on the following files only:

appveyor.yml
build.fsx
Any AssemblyInfo.cs file change (e.g. this)

ecampidoglio

Thank you for contributing this! I support the idea of automating the release process by baking it into the build script 👍

However, there are a few issues that need to be addressed before we can move forward.

ecampidoglio · 2017-04-10T08:37:56Z

Build.fsx

@@ -173,6 +173,7 @@ Target "PublishNuGetRelease" (fun _ -> publishPackagesToNuGet
                                            (getBuildParam "NuGetReleaseKey"))

 Target "CompleteBuild" (fun _ -> ())
+Target "PublishNuGetAll" (fun _ -> ()) 


How are the changes in this file related to the commit message? Shouldn't this be part of another commit or even be committed separately?

Done - moved this change to the previous commit.

ecampidoglio · 2017-04-10T08:41:37Z

Build.fsx

@@ -19,7 +19,9 @@ let build target configuration =
    let properties = [ "Configuration", configuration ] @ keyFile

    solutionsToBuild
-    |> MSBuild "" target properties
+    |> Seq.iter (fun s -> build (fun p -> { p with Verbosity = Some(Minimal)


Why set the verbosity to minimal? Don't we want to have as much information as possible when troubleshooting?

I did that intentionally, as it was impossible to track review the build log on CI - browser hung after the pressure of number of build entries.
However, later (in migration to .NET Core branch I'm currently working on) I found that we perform a lot of redundant rebuilds, so probably we could revert this setting back. Will update soon.

Done - keep normal verbosity

ecampidoglio · 2017-04-10T08:47:16Z

Build.fsx


 let releaseFolder = "Release"
 let nunitToolsFolder = "Packages/NUnit.Runners.2.6.2/tools"
 let nuGetOutputFolder = "NuGetPackages"
 let solutionsToBuild = !! "Src/*.sln"
 let signKeyPath = FullName "Src/AutoFixture.snk"

+type GitVersion = { apiVersion:string; nugetVersion:string; }
+let getGitVersion = 


I like the idea of letting the version number be calculated based on the history. 👍
However, I strongly suggest we use an established tool like GitVersion for that instead of rolling our own.

I do like idea of switching to GitVersion tool, FAKE even have helpers for it, however I don't have experience with that tool. It would take some time for me to read the documentation.

Currently I'm focused on migration to .NET Core, so would like to have this PR merged as soon as possible.
Would it be OK for you if merge this PR as is and switch to GitVersion in v4 branch later?

ecampidoglio · 2017-04-10T08:51:55Z

Build.fsx

@@ -13,7 +13,7 @@ let nuGetOutputFolder = "NuGetPackages"
 let solutionsToBuild = !! "Src/*.sln"
 let signKeyPath = FullName "Src/AutoFixture.snk"

-type GitVersion = { apiVersion:string; nugetVersion:string; }
+type GitVersion = { apiVersion:string; nugetVersion:string }


I think this whole commit should be squashed into 6e6dad9 as it's unclear what it does on its own. What's the "optimization" here?

Done - squashed entire commit with the previous one

ecampidoglio · 2017-04-10T08:55:52Z

Build.fsx

@@ -17,7 +17,7 @@ type GitVersion = { apiVersion:string; nugetVersion:string }
 let getGitVersion = 
    let desc = Git.CommandHelper.runSimpleGitCommand "" "describe --tags --long --match=v*"
    // Example for regular: v3.50.2-288-g64fd5c5b, for prerelease: v3.50.2-alpha1-288-g64fd5c5b
-    let result = Regex.Match(desc, @"^v(?<maj>\d+)\.(?<min>\d+)\.(?<rev>\d+)(?<pre>-(alpha|beta|rc)\d*)?-(?<num>\d+)-g(?<sha>[a-z0-9]+)$", RegexOptions.IgnoreCase).Groups
+    let result = Regex.Match(desc, @"^v(?<maj>\d+)\.(?<min>\d+)\.(?<rev>\d+)(?<pre>-\w+\d*)?-(?<num>\d+)-g(?<sha>[a-z0-9]+)$", RegexOptions.IgnoreCase).Groups


Again, it would be better if we didn't have to support all these use cases on our own. See my previous comment.

See reply below. Let's close this thread ;)

ecampidoglio · 2017-04-10T09:00:41Z

Build.fsx

@@ -1,4 +1,4 @@
-#r @"packages/FAKE.4.19.0/tools/FakeLib.dll"
+#r @"packages/FAKE.Core/tools/FakeLib.dll"


It's hard to get an overview of the changes related to updating FAKE when there are another 150K+ deleted lines. Could you please commit the deletion of the old package separately?

Do you want me to perform the rebase and just separate add/remove as separate commits, or want me to rebase and remove this from PR at all, so it will be fired via separate PR later?

I think a separate commit for deleting the old package (and its related documentation) is enough. No need for a new PR. 😉

Done - separated to 2 standalone commits.

ecampidoglio · 2017-04-10T09:11:17Z

@zvirja Thank you for your contribution! I left a few comments in my review.

Also, I noticed that the build on AppVeyor is currently broken on master. 😢 I understand that you needed to modify the configuration in order to test these changes, but I really think we should get the build up and running as quickly as possible while this PR is being reviewed.

Could you please make a new PR with a new appveyor.yml file that reproduces the old process? Of course, if you don't have time, I can also do it myself.

zvirja · 2017-04-10T09:24:25Z

I do understand your concern regarding the build failing for the master, however there was no other way. I needed to remove the previous AppVeyor project, so currently it relies on appveyor.yml. This file never existed before, therefore now master is broken.

Do you believe that it makes sense to create a temp appveyor.yml? I expect this PR will be merged soon, so everything will become fixed. We don't have too much activities on AF project now, so it might be acceptable to wait for a couple of days. Let me know if you disagree with that and still persist on creation of a temp appveyor.yml via separate PR... :)

ecampidoglio · 2017-04-10T09:40:02Z

@zvirja

I expect this PR will be merged soon

In OSS, "soon" could literally mean anything 😉

I still think we should get the build running on master in the meantime. We might have to fix something in the released version, you never know. Plus, it's unhealthy for the project to have a broken build for any extended period of time.

zvirja · 2017-04-10T10:13:14Z

@ecampidoglio I've followed up all the points above and fixed them (except the GitVersion one). Thank you for such a brilliant code review.

Let's follow this way. If PR is not approved by you and {@moodmosaic or @adamchester} till evening, I'll create this file and add it directly to master - it's pretty simple.

zvirja · 2017-04-10T11:09:22Z

@ecampidoglio I noticed that you liked the reply, but haven't replied here and haven't approved the PR. Does that mean that you don't agree? 😏

FAKE should be updated to support MSBuild from VS 2017. FAKE.Core package is used instead of full FAKE because it's smaller, while all the required functionality is present.

zvirja · 2017-04-10T20:59:28Z

@ecampidoglio I've pushed legacy configuration, so now our CI is green.

I've also spent few hours playing with GitVersion tool and I hate it 😠 While I find it awesome in some aspects, there is one thing which blows my head up. Tool simply doesn't work property with pre-release tags ending with numbers. Consider the following flow:

You release RC1 and add a tag v4.0.0-rc1. Tool version: v4.0.0-rc0001. I understand that padding, because NuGet doesn't support SemVer 2.0 properly.
You add 2 more commits. Tool version: v4.0.0-rc0002. WHAT?
You add 2 more commits and create a tag v4.0.0-rc2. What do you guess is the tool version? It's 4.0.0-rc0002!

The issue is that SemVer tool simply trims numbers at the end of your pre-release tag. Meaning, that you cannot release rc1 and rc2 builds, as you might end up with same version for absolutely different releases.

If you tweak configuration a bit and play with ContinuousDelivery, ContinuousDeployment and branch specific configuration, you might observe different set of behavior (e.g. it simply ignores commits after the tag assigning them the same NuGet version). But none of them is what I expect to see.

I'm too silly to make it work fine 😢 Therefore, if you could find free time, please rework approach to use this tool instead of the regular expressions. In the meanwhile, I'd like to ask to approve as is. I've improved my clutch to format with commits number with padding - I've found that NuGet might not support SemVer 2.0 properly. I'd be really happy it you can adapt GitVersion tool for tags with ending numbers.

Last tag and number of commits since last tag is used to determine version. Resolved version is written to the AssemblyInformationalVersion attribute for each assembly. Later during NuGet pack this version is used as a NuGet version. This approach allows to simplify Release Management by controlling releases via tags creation only.

It appears that NuGet doesn't fully support SemVer 2.0. Format commits number wtih 4 digits so lex comparison will compare values correctly

moodmosaic · 2017-04-11T03:47:54Z

I've been viewing this item on-and-off the past few days and I have to admit that I'm having a hard time understanding what it does 😕 Though, as long as @ecampidoglio is reviewing this, I feel safe 😃

But maybe we should consider splitting this in smaller pull requests?

zvirja · 2017-04-11T07:57:45Z

@moodmosaic Thank you for the time you spent on this. Likely, the most confusion is caused by the FAKE update which brought a huge number of diffs.

Could you clarify which exact parts caused the confusion to you? We could split it to a separate PRs if that is needed, but I need to understand which exact commits you want to group.

moodmosaic · 2017-04-11T08:03:46Z

@zvirja, I just can't understand what happens next if/when this pull request gets merged 😕 (I'm not saying that the pull request is bad, just that I can't understand exactly what it does.)

ecampidoglio · 2017-04-11T08:16:57Z

From what I gather, this PR does a few different things:

Updates to a new version of FAKE
Switches to using the Fake.Core package
Moves the CI configuration to appveyor.yml instead of having it in the AppVeyor portal
Adds automatic versioning based on the state of the history
Adds deployment to NuGet for stable releases
Adds deployment to MyGet for prerelease releases

They all go under the umbrella of automating the release process, but I have to say that multiple PRs for each of those items would be easier to manage.

zvirja · 2017-04-11T08:20:08Z

Ok, let me describe that :) As I mentioned above, the main purpose of these changes is to use repository status to understand current version, rather than need to patch all the AssemblyInfo files instead. We use current/last tag and number of commits since last tag to automatically generate assembly version. Also I added the MyGet feed for the pre-release builds (e.g. for the v4).

Changes by files:

appveyor.yml - Extract all the CI logic to a file. I've added a set of logic to push to different feeds (NuGet and MyGet) depending on different situations. You might find this logic here.
build.fsx - A few changes were applied to this file:
- Add PublishNuGetPreRelease and PublishNuGetRelease tasks to publish produced NuGet artifacts to the feed. I use these tasks in the appveyor.yml file.
- Add getGitVersion function to resolve version based on current repository status.
- Add PatchAssemblyVersions target to automatically patch all the AssemblyInfo.cs files before build using the version provided by the getGitVersion function.
- Given that sign key is now added to repo, I've added parameter to MSBuild to pass that key directly.
Switch to FAKE.Core instead of FAKE.

All this staff just allows you to simplify the release procedure. Now all you need is just to create a SemVer tag - build script will use it as a version for versions and nuget packages.

moodmosaic · 2017-04-11T08:22:58Z

@ecampidoglio, @zvirja, if you agree, based on this and this, it will be faster then to get this done if it gets split into smaller, separate pull requests.

zvirja · 2017-04-11T08:24:07Z

@ecampidoglio appeared to be quicker than me 😉
Agreed! I'll split that to 2 PRs - Update of the FAKE and everything else. Are you fine with that? I don't see a reason to make them smaller.. That should be definitely enough for the better understanding.

moodmosaic · 2017-04-11T08:26:02Z

@zvirja, probably it's just me, but I see each item on this list by @ecampidoglio as a separate pull request.

ecampidoglio · 2017-04-11T08:27:18Z

Actually, I would prefer to have separate PRs for each of the items that I listed here, assuming I'm not missing anything.

The reason is that we might not want to have everything. For example, while the automatic versioning might be good, we might want to hold off on the MyGet feed.

zvirja · 2017-04-11T08:36:48Z

Thanks for the feedback. Likely, that is how review system works in OSS projects - you need to make small granularity changes.

I could rework that to the small sub-sets, the are couple of issues with that:

All these changes are iterative, meaning that I will need to create PRs one by one. That will take a lot of time, before all them are finally merged. It doesn't play well with our plan to release v4 in the nearest future :)
Do you really want to throw something out? If yes - it would make sense to do that. If not - it would be just a process for a process. I do understand that it was my mistake to mix all these changes together and I promise to not do that in future. But this will require me to rework everything just to make granular changes. I've spent almost 1.5 weeks trying to find the working solution for that it it's sad for me to rework everything just for a good history, while the current history is not too bad.

However, by submitting that I'm ready to do that. If you really need that, feel that it would be better for everybody and we cannot accept 2 PRs with FAKE update and everything else - I'll do that! I appreciate the time you spend on this, I want to collaborate and to not make a chaos in our project 😉

ecampidoglio · 2017-04-11T08:49:43Z

@zvirja Thank you for understanding. Your efforts are very appreciated 😃

In the future, it's better to discuss your proposed changes with the team first, before you do any actual work (with the exception of a small POC, if necessary). That way, you'll avoid investing a huge chunk of your time doing something that might not align with the vision the rest of the team has for the project.

The preferred way to do that is to create an issue and have the initial discussion there. Once we agree on the course of action, you can submit one or more PRs in reference to that issue.

Again, I want to empathize that I really appreciate your enthusiasm and your will to do good! We just need to dial it back a little so that we don't rock the boat too hard 😉

zvirja · 2017-04-11T09:41:51Z

@ecampidoglio Thanks for the appreciation 😌

Ok, let's follow this way. I've already created the first PR with FAKE update and will create more after it will be approved.

I close this one as it will not be merged.

zvirja requested review from moodmosaic, adamchester and ecampidoglio April 7, 2017 22:28

zvirja closed this Apr 8, 2017

zvirja reopened this Apr 8, 2017

zvirja force-pushed the rework-buildscripts-for-easier-management branch from d924a61 to f3f92e9 Compare April 9, 2017 14:00

ecampidoglio requested changes Apr 10, 2017

View reviewed changes

zvirja force-pushed the rework-buildscripts-for-easier-management branch from f3f92e9 to 488db65 Compare April 10, 2017 10:02

zvirja added 4 commits April 10, 2017 21:41

Add batch file to update FAKE

6d24da1

Update FAKE to new version and switch to FAKE.Core package instead

6d5c1f6

FAKE should be updated to support MSBuild from VS 2017. FAKE.Core package is used instead of full FAKE because it's smaller, while all the required functionality is present.

Remove old FAKE as it isn't in use more

0cbe416

Extend build script with NuGet push tasks

2ec1202

zvirja force-pushed the rework-buildscripts-for-easier-management branch from 488db65 to de25f50 Compare April 10, 2017 18:43

zvirja added 8 commits April 11, 2017 00:24

Add appveyor.yml file with build configuration

49029ad

Publish sign key to repository

6b2afa6

Delete unused variable

9cb9baf

Tune appveyor.yml to push to prod feed only for release or rc tags

e75097e

Add production NuGet keys to CI script

a5ddabc

Add support any pre-release suffix instead of hardcoded ones only

535f5bf

Format commits number to 4 digits as NuGet doens't support SemVer 2.0

bfb5643

It appears that NuGet doesn't fully support SemVer 2.0. Format commits number wtih 4 digits so lex comparison will compare values correctly

zvirja force-pushed the rework-buildscripts-for-easier-management branch from de25f50 to bfb5643 Compare April 10, 2017 21:26

zvirja mentioned this pull request Apr 11, 2017

Update the FAKE package #751

Merged

zvirja closed this Apr 11, 2017

zvirja deleted the rework-buildscripts-for-easier-management branch April 11, 2017 13:12

zvirja mentioned this pull request Apr 17, 2017

Calculate build version based on repo status #753

Merged

3 tasks

ecampidoglio mentioned this pull request Apr 28, 2017

Add automatic deployment support #755

Merged

		@@ -1,4 +1,4 @@
		#r @"packages/FAKE.4.19.0/tools/FakeLib.dll"
		#r @"packages/FAKE.Core/tools/FakeLib.dll"

Rework buildscripts for easier management #749

Rework buildscripts for easier management #749

Conversation

zvirja commented Apr 7, 2017 • edited

moodmosaic commented Apr 8, 2017

zvirja commented Apr 8, 2017 • edited

zvirja commented Apr 9, 2017 • edited

ecampidoglio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zvirja Apr 10, 2017 • edited

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

zvirja Apr 10, 2017 • edited

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

zvirja Apr 10, 2017 • edited

Choose a reason for hiding this comment

ecampidoglio commented Apr 10, 2017

zvirja commented Apr 10, 2017

ecampidoglio commented Apr 10, 2017

zvirja commented Apr 10, 2017

zvirja commented Apr 10, 2017

zvirja commented Apr 10, 2017 • edited

moodmosaic commented Apr 11, 2017 • edited

zvirja commented Apr 11, 2017

moodmosaic commented Apr 11, 2017

ecampidoglio commented Apr 11, 2017 • edited

zvirja commented Apr 11, 2017

moodmosaic commented Apr 11, 2017

zvirja commented Apr 11, 2017 • edited

moodmosaic commented Apr 11, 2017

ecampidoglio commented Apr 11, 2017

zvirja commented Apr 11, 2017

ecampidoglio commented Apr 11, 2017

zvirja commented Apr 11, 2017

zvirja commented Apr 7, 2017 •

edited

zvirja commented Apr 8, 2017 •

edited

zvirja commented Apr 9, 2017 •

edited

zvirja Apr 10, 2017 •

edited

zvirja Apr 10, 2017 •

edited

zvirja Apr 10, 2017 •

edited

zvirja commented Apr 10, 2017 •

edited

moodmosaic commented Apr 11, 2017 •

edited

ecampidoglio commented Apr 11, 2017 •

edited

zvirja commented Apr 11, 2017 •

edited