Node execution engine and build tooling #1139
Conversation
|
There is still a few things to be done especially (geeklearningio/gl-vsts-tasks-build-scripts#1), but I'm creating the PR so progress can be monitored. |
There was a problem hiding this comment.
@sandorfr Thank for this PR! Very much appreciated. I added some comments inline.
I think the biggest open issue is the integration in to the current build pipeline and the already mentioned required npm package for replacing the submodule.
Regarding the integration in the build pipeline: We currently use a Cake script for building the solution. This is the script which is triggered on AppVeyor. The VSTS/TFS task is also part of the Visual Studio Solution. If you rename files, this also needs to be reflected there. As part of the AfterBuild target of the GitVersionExe project we currently copy around the files for the VSTS/TFS task and call a PowerShell script for patching version and one for creating the VSIX. If we switch the build for the VSTS/TFS task to npm scripts this needs also to be cleaned up.
IMHO using npm for the build part of the VSTS/TFS task could make sense. But in this case the npm build script should be called from the Cake script, where also the version information is available.
Maybe other @GitTools/developers have another opinion about the build pipeline :) So maybe it would be good to have a quick discussion about it here.
|
|
||
| if (!gitVersionPath) { | ||
| gitVersionPath = tl.which("GitVersion.exe"); | ||
| gitVersionPath = path.join(currentDirectory, "GitVersion.exe"); |
There was a problem hiding this comment.
This doesn't make sense. I assume the second assignment should be the fallback in case which couldn't find the tool?
There was a problem hiding this comment.
There is an if missing indeed.
| "/nofetch" | ||
| ]); | ||
|
|
||
|
|
| if (result) { | ||
| tl.setResult(tl.TaskResult.Failed, "An error occured during GitVersion execution") | ||
| } else { | ||
|
|
| tl.setResult(tl.TaskResult.Failed, "An error occured during GitVersion execution") | ||
| } else { | ||
|
|
||
| // workaround gitversion to make sure variable are availabel in all vsts build contexts |
| } | ||
| })(); | ||
|
|
||
|
|
| @@ -0,0 +1,99 @@ | |||
| import tl = require('vsts-task-lib/task'); | |||
| import path = require('path'); | |||
| import fs = require('fs-extra'); | |||
There was a problem hiding this comment.
fs-extra is actually not used, I'll remove the dep
| @@ -0,0 +1,99 @@ | |||
| import tl = require('vsts-task-lib/task'); | |||
| import path = require('path'); | |||
There was a problem hiding this comment.
tl and path are used almost everywhere. ;)
| "author": "", | ||
| "license": "ISC", | ||
| "dependencies": { | ||
| "fs-extra": "0.30.0", |
| "dependencies": { | ||
| "fs-extra": "0.30.0", | ||
| "vsts-task-lib": "0.9.20", | ||
| "q" : "1.4.1" |
.gitmodules
Outdated
| @@ -0,0 +1,3 @@ | |||
| [submodule "src/GitVersionTfsTask/BuildScripts"] | |||
There was a problem hiding this comment.
As already mentioned instead of a submodule this should be included by npm (see geeklearningio/gl-vsts-tasks-build-scripts#1).
The whole build pipeline needs generally a little bit more work. It needs to be integrated into the existing build pipeline.
There was a problem hiding this comment.
I'll focus on fixing everything on the task itself. Once we have something working and satisfying and depending on your preference I will either publish our tooling on npm or drop our tooling from the PR. Meanwhile it allows me to publish private extension for testing purpose more easily.
There was a problem hiding this comment.
We've decided to go forward with the npm package. So I've updated this PR accordingly. I'll investigate the vsts beta feature.
src/GitVersionTfsTask/typings.json
Outdated
| @@ -0,0 +1,12 @@ | |||
| { | |||
There was a problem hiding this comment.
I would prefer we use typescript 2 and @types rather than typings
src/GitVersionTfsTask/package.json
Outdated
| "private": true, | ||
| "main": "index.js", | ||
| "author": "", | ||
| "license": "ISC", |
There was a problem hiding this comment.
Should use MIT to match GitVersion
| } | ||
| }, | ||
| { | ||
| "Name": "dev", |
There was a problem hiding this comment.
Is this a requirement, I would prefer prerelease or something?
There was a problem hiding this comment.
we can change it or remove it.
| @@ -1,9 +1,8 @@ | |||
| { | |||
| "manifestVersion": 1, | |||
| "id": "gitversion", | |||
| "id": "gl-gitversion", | |||
There was a problem hiding this comment.
Please revert this change
There was a problem hiding this comment.
I'll revert it when it's ready, meanwhile it allows me to publish a prerelease version of it without any conflicts :)
| "name": "GitVersion", | ||
| "publisher": "gittools", | ||
| "public": true, | ||
| "publisher": "geeklearningio", |
| "name": "GitVersion", | ||
| "publisher": "gittools", | ||
| "public": true, |
There was a problem hiding this comment.
Our tooling allows you to build different version of the same extension. In order to do that we use the extension flags instead and they are configured in configuration.json.
| } | ||
| ], | ||
| "content": { | ||
| "details": { | ||
| "path": "overview.md" | ||
| } | ||
| }, | ||
| "contributions": [ |
There was a problem hiding this comment.
Have they moved more towards convention over configuration?
There was a problem hiding this comment.
no this just filled automatically by our tooling, if you decided to keep the tooling, they are no longer needed.
| @@ -0,0 +1,16 @@ | |||
| { | |||
| "name": "git-version", | |||
There was a problem hiding this comment.
remove the -, should be gitversion
There was a problem hiding this comment.
changing it but it does not really matter as this file is just here to indicate the dependency to be bundled with the task.
| { | ||
| "name": "git-version", | ||
| "version": "1.0.0", | ||
| "private": true, |
There was a problem hiding this comment.
this is not meant to be published to npm, It allows some of the usually fields required by package.json to be missing.
| "license": "MIT", | ||
| "dependencies": { | ||
| "vsts-task-lib": "0.9.20", | ||
| "q" : "1.4.1" |
There was a problem hiding this comment.
Why not native promises?
There was a problem hiding this comment.
vsts-task-lib depends on q. I guess this is related to the fact that some agents are running older versions of node which won't provide native Promises.
src/GitVersionTfsTask/tsconfig.json
Outdated
| "./Tasks/*.ts", | ||
| "./Tests/*.ts", | ||
| "./typings/index.d.ts", | ||
| "./Commom/Node/*.d.ts" |
There was a problem hiding this comment.
Spelling, should be Common?
|
Hi @JakeGinnivan @pascalberger, thanks for taking the time to review this. Just mentioning if it was not clear that this PR is more to show the work in progress, and comment on the core of it ( I know that our tooling might be confusing for you, but I need it to be able to use our continuous delivery pipeline which I use to actually build publish and test this extension. But when my work will be done, and the tasks will be 100% functional, I can make a new PR without any of our tooling. We think it's great but don't want to be pushy about it. Meanwhile the real issue I'm facing is that the current portable version crashes on the Mac. The good news is this is probably a gitversion bug which has been fixed in libgit2/libgit2sharp#1409. The bad news is a version of gitversion including this fix hasn't been released yet :( |
|
I would love to get this in for personal reasons :) Would we be able to drop the initial cut without the shared tooling then switch over to the reusable tooling once it is ready? |
|
Once we merge this into master I will release it soon after that and you will not need your own GitVersion pipeline |
|
Would also allow us to release pre-release packages to VSTS and also close issues like #1150 |
|
Ok, @JakeGinnivan I try to find some time to make the new minimal PR very soon. libgit2sharp apparently released a pre-release with the fix: https://www.nuget.org/packages/LibGit2Sharp/0.24.0-pre20170124092542. If someone can see on your side if there is a way for you to update the lib so it really works on macosx, that would be awesome :) |
Addresses #909