-
Notifications
You must be signed in to change notification settings - Fork 654
Support for Visual Studio Online Build vNext - VSO Build Step #562
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…lorer without a VSIX
…on't show up as a remote branch
Not sure about that build failure- - it's an access denied error on the copy of GitVersion.exe as part of the build? Anyway, @JakeGinnivan @pascalberger, I think this is ready for you to take a look at. I could not get a git fetch working from within GitVersion due to not knowing the right temp session creds, but I was able to powershell my way around it, I think... I've tested this with regular CI builds, pull requests and manual queue builds. You'll need to use the tfx tools to upload the |
The docs/video on how to install TFX are here: https://github.com/Microsoft/tfs-cli/blob/master/docs/buildtasks.md. Basically npm install tfx-cli -g, then you can do tfx login and then tfx upload and point to the dir. Delete doesn't seem to work well due to agent caching of the tasks, so I've just incremented the patch version every time I make a change and re-upload it. That won't be necessary for the real package, just for dev. |
Thanks! Why all the handling of git branches in the PowerShell script? As mentioned by @JakeGinnivan in this comment I understand this should already happen inside gitversion.exe. Do you have deliberately added all the changes from #560 again? Doesn't make much sense IMHO. I would keep this separated (#560 with the necessary changes in gitversion and this PR with only the VSO build task) |
## Basic Usage | ||
In [Visual Studio Online](https://www.visualstudio.com/) build vNext (the web based build system) you can add a build step as follows: | ||
|
||
* **Build Step:** Command Line |
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.
Why command line when this PR is about a custom build step?
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.
That's old and needs to change. That refers to using GitVersion.exe as a standalone. That won't get past the detached head issues that the VSO build step does. I'll update the docs.
So the reason all of the changes are there is that it's a branch off of a branch. I needed my fixes there to continue my work here. This is the only PR that should be needed; the other one can die. The reason I had to do it all in PowerShell is that the C# code relies on authentication and VSO needs per-session tokens. I've pinged the team but have not gotten any response yet if any usable username/password is even available for use by the task to do the fetch/pull. This powershell seems to work so the fetch isn't needed. VSO does a full clone but does the checkout of a commit leaving the detached head. My script recreates the local branches and then sets the selected branch at the designated commit. |
* **Tool:** `GitVersion.exe` | ||
* **Arguments:** `/output buildserver /updateassemblyinfo true` | ||
|
||
Then in your build parameters simply [add a placeholder](#nuget-in-teamcity) of the GitVersion variables you would like to use. |
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.
This seems like copy/past from TeamCity documentation and not accurate for this PR
OK, I see the point about the authentication. But this would also mean that the NuGet package cannot be used successfully with VSO? Maybe this would also be useful information to add to the docs. |
Correct.... for now, this VSO build step is the only way to get it working.... unless someone else runs a similar script before running the NuGet package version. The GitVersionTask would work too, long as this build step runs first as it'll prep the local clone. |
Just thinking if it maybe would make sense to split the task into two tasks:
Like this for NuGet or GitVersionTask scenarios only the branch handling task can be included. Not that executing gitversion twice will do much harm, but it might clutter the logs. What do you think? |
Maybe... but then people would need to remember to add two tasks/build steps to their build definition. There's something to be said by keeping it simple. Just drop this one step in and it'll do whatever it needs to just work. |
OK, thats a point :) |
This is awesome work guys, I dock my hat to you all! What's the word Jake ? Is this good to go :-) |
Nearly, have a branch at https://github.com/JakeGinnivan/GitVersion/commits/vso-build-step which builds on this PR. I want to figure out how I can get PR's detected properly in GitVersions branch normalisation. Tomorrow. But for now I am heading to bed :) |
Cool, I can't wait, I'll be a test subject if you want @JakeGinnivan when your ready as I'm working on CI over the next couple of days. |
Feel free to pull down my branch and give it a spin. I reckon I'll get this merged tomorrow and released on the weekend. Was great work by Oren to get it to this point so don't want to delay it. Sent from my Windows Phone From: John Kattenhornmailto:[email protected] Cool, I can't wait, I'll be a test subject if you want @JakeGinnivanhttps://github.com/JakeGinnivan when your ready as I'm working on CI over the next couple of days. — |
Ok, so there is a slight issue with the integration. VSO does not force merge commits. Meaning the reason pull requests do not detect is because the branch is fast forwarded so the HEAD which is building matches an actual branch, so we never go searching for a pull request ref. The fact that VSO doesn't force merge commits might have some other issues (multiple heads pointing at the same commit etc). I need to investigate. I have also reached out to some MS people to see if it's planned to allow forcing merge commits on branches. |
Could we add some logic to check for multiple |
I do have some logic, but it is still non deterministic. There is enough logged that when it blows up, it gives us enough context of the scenario that we can figure how to improve it. Sent from my Windows Phone From: Darrellmailto:[email protected] Could we add some logic to check for multiple heads pointing to the same commit, in order to check for these fast forward merges? It would be nice to handle fast forward merges - for the instances where people (or VSO) don't use no-ff — |
I think I have a solution but it's VSO specific. VSO passes the branch name in BUILD_SOURCEBRANCH and the commit id in BUILD_SOURCEVERSION. The source branch is passed in the form The workaround is to see if the specified source branch matches any actual branches and create it if not. I did have that part in my script but it could be done in code too. Whether the branch exists or not depends on if there are multiple merge builds and whether "clean" is set in the VSO repo options. Clean for Git nukes the .git directory, not-clean preserves it but nukes all the files and just re-checks-out. In the case there was already a branch, I did a Maybe the |
See #571 |
So not off a great start, I can't even get tfx-cli to run at all. I assume the I needed npm installed in order to grab the tfx-cli package as I found the installation instructions https://github.com/Microsoft/tfs-cli. I've tried running npm install from command prompt as both administrator and non-administrator and the install runs but then running tfx after the install gets me 'tfx' is not recognised. I'll log an issue at the tfs-cli repo and see what I've missed. |
@johnkattenhorn Have you installed tfx-cli globally: |
Yep, tried global, non-global all the same from Windows 10 Command Prompt and Admin, Non-Admin, Powershell. I had this type of problem before with node and npm but don't know enough about npm to debug. I can see a node_module in my roaming profile but nothing in the path and no .exe / .cmd etc. |
Someone helping me on tfx-cli, I'll report back shortly. I can pull the latest straight from master now can't I as the latest bits are merged right ? |
Strange. Works fine for me on Windows 10. After global install of the tfx-cli module you should see the module installed in \npm\node_modules\tfx-cli\ (it contains no exe or cmd) You can download the latest version from the master branch from the AppVeyor build. |
Bingo, seems that the installation of node from a full VS2015 install doesn't add the NPM directory to the path. Doing a Does the trick, thanks @pascalberger, i'll pull from there. |
Investigating now, I think I remember @JakeGinnivan post something about the libraries. |
Maybe this is related to #573. I have only tested yet with earlier versions of the VSO task where I haven't experienced this error. |
Looks like it could be @pascalberger, good find! @SimonCropp, @JakeGinnivan what do you think ? Should we be modifying the VSO BuildTask to include the external dependencies ? |
Yep, if I rebuild from source then I get a Nativebinaries directory under the GitVersionVsoTaskBuild so I'm going to attempt to upload this custom task over the top of the existing. |
Yep that seems to be working fine, now to put together a custom build step for Octopus Packing .... |
@johnkattenhorn yeah i think i fixed that on a later commit. the native binaries should now be shipped with all the packaged things. let me know if any cant find the dependencies |
@SimonCropp I get the same exception with current head version. I created #589 for this. |
@pascalberger not the same exception as in 589. the one here is about not being bale to find the libgit native files. 589 is about git.exe |
@pascalberger sorry i stand corrected. i see u have two errors there |
This is an implementation for #561. It should be done and has been tested with CI builds, manual builds and pull request builds.
Needs code review.