Add Appveyor CI for Windows #672
Conversation
appveyor.yml
Outdated
| - npm install | ||
| build: off | ||
| test_script: | ||
| - cmd: mocha test/*.test.js |
There was a problem hiding this comment.
Newline at the end of the file please :)
There was a problem hiding this comment.
Also, shouldn't the cmd be npm test?
There was a problem hiding this comment.
npm test runs pretest, then test. pretest currently builds. npm install also builds, so it was doing a redundant build. Do we need pretest to build? I agree it would be nicer to just have npm test.
There was a problem hiding this comment.
Ahh, hmm, I would kind of like to get rid of the pre* as well. I don't think it's the right place to have that responsibility here.
Although, at least on unix, node-gyp build isn't the same as rebuild. Mine just outputs "nothing to be done"...
|
This is awesome, some comments:
|
zbjornson
force-pushed
the
appveyor
branch
26 times, most recently
from
cb65096
to
381e8f9
Compare
appveyor.yml
Outdated
| - 7z x gtk.zip -oGTK > nul | ||
| - mv GTK/ C:/ | ||
| - npm install | ||
| build: off |
There was a problem hiding this comment.
Would it work if removed both of these lines?
- npm install
build: offThere was a problem hiding this comment.
No, the default build command invokes msbuild directly. Getting the configuration right for that looks more verbose/complicated than it currently is.
|
Do you think we should remove all iojs releases from both here and travis? I don't know what the LTS status is on those but I don't think anyone uses them anymore... |
|
From https://github.com/nodejs/LTS/
I don't know if there are any metrics available to see if anyone is using iojs nonetheless. Doesn't hurt to leave them, I think? |
zbjornson
force-pushed
the
appveyor
branch
2 times, most recently
from
dd4a26a
to
6d65cd3
Compare
|
@TooTallNate @domenic @rvagg @kangax Who should I talk to to get a free appveyor account set up for Automattic? I guess it would be good if someone with a somewhat official email address set it up? If this is desirable at all, which I assume it is :) |
|
That would be @TooTallNate or @rvagg (@domenic and I were invited) |
|
Not I, it doesn't show up for me in appveyor so I guess I don't have enough access in the Automattic org, will have to defer to someone more official, @TooTallNate or someone else there. |
|
@TooTallNate Do you have time to take a look at this? I would appreciate it! |
|
This would be nice to have, I don't use node-canvas on Windows anymore, so AppVeyor would have caught #984 breaking the build against the old gtk.zip |
|
I'm personally still open for this. One thing I don't like with AppVeyor though is that they are super super super slow. If anyone knows any other free for open source CI for Windows, I would be very interested 😄 Other than that, I think we are at the same position as before, someone from the automattic org needs to enable this... |
Yeah, so basically the same problem as #796. I wonder how much it would help if we only built with the latest node on macOS and Windows? I could maybe experiment with that a bit. |
|
Just updated this branch since JPEG works on Windows now. The builds themselves aren't slow (they're actually about a minute faster than Travis) but they all run sequentially. So, I've set the matrix to just Node 6 (LTS) and MSVS 2013 and 2015. (2015 is probably sufficient; 2017 I haven't got working yet, although I use it on one of my computers.) ✔️ https://ci.appveyor.com/project/zbjornson/node-canvas/build/0.0.61 <-- PR #987 |
|
What I remember with them being super slow was for them to actually start, my latest build in node-spearker took hours (I think, can't find any numbers now after the fact) just to start 😱 Anyhow, I'm still all for getting this in, just venting my frustation 😂 |
|
I no longer have admin access to this repo either. Not sure who at a8c to ping based on the commit logs. Maybe /cc @dmsnell? I dunno. |
|
Thanks @TooTallNate! I can try and look into this soon. |
|
Hey @dmsnell, any luck finding the admin of this repo? :) |
|
@zbjornson do we need the fix in canvas.cc in order to add Appveyor? Can you provide some context for that fix and why it's there, what it's fixing? Seems like we're changing the line under an existing comment and it's not clear if that comment needs to be altered before merging this in. |
|
That commit was already merged, I just haven't rebased. We need the admin of this repo to enable Appveyor though. |
|
@zbjornson I'm getting the Appveyor account setup. I may need some help in the next few days getting it configured as we need it to be here, but I'll let you know. |
|
@zbjornson in the meantime, would you mind rebasing this branch against the current master? |
|
Although the Appveyor account isn't established yet I'm going to go ahead and merge this in for now. Honestly I don't know how to review this but I can tell it shouldn't cause trouble. Thanks for your contributions! |
This mostly works. VS2015 fails without #670. We need #458 merged for any to fully pass -- see log:
https://ci.appveyor.com/project/zbjornson/node-canvas/build/job/x3domgfjsq77445m After that I need to change the script to install libjpeg-turbo.
For now there's only one node env. Think it's unnecessary to have more than one, but it's easy to add.
(Sorry I've got 83a8b27 upstream from #671 in here.)