Skip to content

AppVeyor CI integration, take 2 #2229

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

Closed
wants to merge 46 commits into from
Closed

Conversation

weltling
Copy link
Contributor

@weltling weltling commented Dec 4, 2016

This is based on #2169. The tests encompass almost all the important bindings including PostgreSQl, MySQL, OpenSSL related. Opcache is not done yet, but can be added later. Quite a few test bugs are fixed in the core, some are still there, plus some possible bugs are to investigate. The build history is here https://ci.appveyor.com/project/weltling/php-src/history .

Thanks.

@weltling weltling mentioned this pull request Dec 4, 2016
@nikic
Copy link
Member

nikic commented Dec 9, 2016

For the enchant dictionary: Can we fetch this from somewhere during the build, instead of including it in the repo? Not sure if we should include a 50k line file for testing purposes only.

@weltling
Copy link
Contributor Author

weltling commented Dec 9, 2016

Indeed. It can be put somewhere under windows.php.net, and then use the Appveyor cache. Leaving it in but compressed would probably work in a pinch, too. But fetch+cache is OFC better, doing that.

Thanks.

* php/master:
  Care about intput and output encoding, as per default encoding RFC
  More fixes for bug #73089
* php/master:
  cleanup temporary data
This reverts commit 20ca98f.
50ms is not a critical time, but will make test not fail on slow vm
* php/master:
  make timing check more forgiving in these tests
  Skip tests when secure_file_priv dir not writable
  Add missing UPGRADING entry. Manual is updated before 7.0 release.
  extend skip section
  Partially fix bug #70492
  Fixed bug #73727
* php/master:
  Add an assertion
  Fixed bug #73746 (Method that returns string returns UNKNOWN:0 instead)
@weltling
Copy link
Contributor Author

Merged into master.

Thanks.

@weltling weltling closed this Dec 16, 2016
@weltling weltling deleted the appveyor3 branch December 17, 2016 17:59
@mlocati
Copy link
Contributor

mlocati commented Dec 20, 2016

Great!

@staabm
Copy link
Contributor

staabm commented Jan 2, 2017

@weltling as @krakjoe reported, the build queue on appveyor gets long and ci feedback is reported very late... just googled a bit and found out that Appveyor supports a "build cache"[1], which might can be used to speedup the builds...?

[1] https://www.appveyor.com/blog/2016/09/28/the-new-build-cache/

@staabm
Copy link
Contributor

staabm commented Jan 2, 2017

latest docs on the feature: https://www.appveyor.com/docs/build-cache/

@weltling
Copy link
Contributor Author

weltling commented Jan 2, 2017

@staabm this cache is already used for the deps and a couple of other things. The most time is spent for tests. Having something like ccache could speedup the compilation, but otherwise - that's it. Not sure, what exactly you wanted to cache.

Indeed, if you compare a single AppVeyor run with Travis - it's not slower, even though some more things are tested. But the fact it doesn't run in parallel is the most crucial in the story. That's something we cannot affect under a free account for OSS. Instead, the option I've turned on for that is called rolling builds - it'll abandon a running build, as soon as there's a new commit in the same branch. I guess, there's some potential for speedup on the process, but without parallel build runs it won't be significant. But actually, I wanted to add some more exts yet, firebird and dba at least :)

@staabm you can experiment on this easily, by logging in to AppVeyor and making changes to .appveyor.yml in a feature branch. If you can get some significant improvement, so it could be PR'd to the mainstream.

Thanks.

@krakjoe
Copy link
Member

krakjoe commented Jan 2, 2017

Earlier on today, I waited three hours for a build to complete; it spent two hours in the queue, and then took an hour to run.

Regardless of the reasons, this doesn't feel like continuous integration to me, and actually does waste a lot of time.

@nikic
Copy link
Member

nikic commented Jan 2, 2017

For the record, I've disabled AppVeyor for 7.0 and 7.1 today to reduce the queue size. Building master and PRs provides most value for us, as anything that goes into 7.0 and 7.1 generally also goes into master. (I don't yet know whether this will still build PRs against 7.0 and 7.1 or not.)

@staabm
Copy link
Contributor

staabm commented Jan 2, 2017

(I don't yet know whether this will still build PRs against 7.0 and 7.1 or not.)

@nikic I can only speak for travis-builds in which the config depends on the base revision of the PR (the .travis.yml of that base revision to be precise). I guess Appveyor uses the same mechanics.

@weltling
Copy link
Contributor Author

weltling commented Jan 2, 2017

IMO, the most value even is that the PRs are checked. For the core, it's nice too, but there is also the snap service that won't go away. Btw, if there's another CI opportunity, it'll be easy to switch, as all the scripts can be reused. Otherwise there's no way to get builds running in parallel with the free AppVeyor account. Days with a big commit occurrence are rare, rolling build might be therefore ok.

What were thinkable is indeed reducing the amount of tests. Also, the difference to the Travis scripts is - literally the full core is being built, however some tests are excluded. On Travis, it's a selective build. The compilation time is not significant, however, what is significant is the test time. I'm not sure, that reducing the amount of tests should be done. Or if it should, probably some candidates should be evaluated.

Thanks.

@staabm
Copy link
Contributor

staabm commented Jan 2, 2017

Fyi, Asked Appveyor for a possible sponsoring

https://twitter.com/markusstaab/status/815900307569909761

@nikic
Copy link
Member

nikic commented Jan 2, 2017

For some reason AppVeyor still builds 7.0 and 7.1, even though I added PHP-7.0 and PHP-7.1 to the branch exclude list...

@weltling
Copy link
Contributor Author

weltling commented Jan 3, 2017

I've added it to .yml file, as I remember to have read earlier, that the UI config is overridden. Seems that's what it wanted. Strangely worked earlier for 5.6 in the UI. I left 7.1 in the builds yet, otherwise one could just whitelist master only, no blacklisting.

I guess it'll indeed ignore the PRs against blocked branches, though. Probably only can be proven in action, as it's a new land for me as well.

Thanks.

@staabm
Copy link
Contributor

staabm commented Jan 5, 2017

@staabm this cache is already used for the deps and a couple of other things. The most time is spent for tests. Having something like ccache could speedup the compilation, but otherwise - that's it. Not sure, what exactly you wanted to cache.

just to let you know @weltling. here is a PR which includes ccache into a Appveyor build
servo/servo#14850

maybe its usefull to you and parts of it could be re-used for php-src

@nikic
Copy link
Member

nikic commented Jan 5, 2017

IIRC ccache does not work with msvc. The linked PR is a mingw build.

@weltling
Copy link
Contributor Author

weltling commented Jan 6, 2017

Yeah, ccache is not compatible. Some forks against the older VS versions exist in the world, but that's another story. So far i couldn't see any comparable analogue to ccache, unfortunately. One project i've stumbled upon https://github.com/frerich/clcache is very cllose to it, but it doesn't support builds with debug symbols enabled. Given that, I retained from it as it were of no use for our release builds. For CI, debug symbols are not strongly required, indeed i've turned them off some days ago to win a couple of seconds already.

But even we'd add it, probably there were no big gain as the compilation time is not the bottleneck. With clcache we colud speedup the compilatino in the test run ofc, though imo it's not worth the effort from both testing and reuse perspective. Nice would it be ofc to have some sponsorship, however it most likely looks negative. I checked also a couple of other CI providers, that can be found on the net. Seems it is somehow a business strategy, not offering the parallel runs. Or maybe it's something HW or platform dependent. ATM how it looks like, to reduce the wait times, it's rather about excluding some tests or about excluding yet 7.1 :(

Thanks.

@staabm
Copy link
Contributor

staabm commented Jan 6, 2017

I am in email contact with appveyor.. lets see..

@staabm
Copy link
Contributor

staabm commented Jan 7, 2017

As of today php-src is build using 3 concurrent workers🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants