madler
Add continuous integration (CI)
To make it easier to accept contributions to zlib we should establish automated testing.
This commit uses the existing test/example.c testing. It connects those tests to AppVeyor, allowing AppVeyor to mark commits as passing or failing the tests. It also automatically runs tests when a pull request is submitted or updated.
There is some room for future expansion enabled, such as tracking the file and line of a failing test. This is thanks to @dankegel, who also got this working on CI providers other than AppVeyor.
In order for this to land properly in madler/zlib, the README.md (the renamed and reformatted README file) must be updated with new [appveyor-shield] and [appveyor-link] links. These can be obtained by @madler once they enable AppVeyor on madler/zlib.
Could you elaborate on bypassing ctest? Everything should continue to work the way it did, including ctest.
It's just my personal bias towards using same test command on cloud as one does locally. I don't think it's that important.
I like your idea of keeping as much as possible the same between local & CI testing.
The existing local tests just print to stdout, which isn't an option for the CI testing. So the two must diverge for now.
(We can later add proper tests & test harnesses, which could help keep the two uses similar. But I figure that warrants a separate pull request since it would be an independent project.)
Maybe we could have AppVeyor use ctest to keep things more similar. Is there a way to pass a command line parameter to ctest? I checked https://cmake.org/cmake/help/latest/manual/ctest.1.html but don't see anything.
googletest obeys both a commandline argument and an environment variable for exactly this situation; passing ZLIBTEST_JUNIT=foo could do the same thing as --junit=foo.
Incidentally, that option should probably add the junit output file without disabling the normal output. That keeps the remote and local cases closer IMHO.
Now that I look at it, why does the non-junit case terminate on first error? Might want to make that a separate option. I know it was the old behavior, but...
uh why appveyor? Does github actions not support C?
I thought github actions supports windows-latest, ubuntu-latest, and also macosx-latest and I thin the macosx and the ubuntu got gcc / clang too so I do not see why not.
While Windows got VS2019.
Maybe not a bad idea for the --junit command line parameter to add to the existing output. The problem was I wanted to preserve the old behavior (which called exit(1)). I suppose I could do something like if (!is_junit) but then I need to pass that into the test functions, which changes their interface and feels messy.
In the end, we should be using a different test harness anyway. So I figured for now this was the way to go.
@AraHaan When we first looked into this, GitHub Actions weren't an option because of security. My understanding is at the time, relying on an Action maintained by someone else opened up the possibility of an Action committing code into zlib that we didn't want. Since that original glance, there is apparently a way to provide your own Actions and not have this security issue.
I'll look into GitHub Actions as an option again. I feel like it might be the better option since it'll integrate more nicely.
Re other test harness - are there any that support being built with a K & R C compiler? If that's not a requirement, https://github.com/ThrowTheSwitch/Unity is rather nice.
is_junit should probably be a global anyway, at which point it's easy to add g_abort_on_arror. I'll throw together an illustration...
I'm not sure about the K&R requirement for testing. That's probably up for discussion among the zlib contributors / maintainer. I suspect it would be ideal but there might not actually be any. At which point, I imagine we'll all weigh how much we want to write our own vs. give up that requirement.
The various CI integrations have different types of support for test harnesses. JUnit format seemed to be common enough, so that is what I went with here. It looks like Unity doesn't have support for the common types, but does allow you to control output. That would be an option, I suppose. It is basically what this pull request does. Either way, that's probably a follow-up pull request.
Even if we make is_junit and g_abort_on_error globals, we still need a way to pass --junit to ctest. I saw your example that bakes the parameter into ctest, but doing that loses the old behavior. In either case, I'm not terribly worried because I suspect we'll want to either use a proper test harness (at which point we don't want to keep the old behavior) or write a more robust output formatter. Maybe we should cross that bridge when we get there?
In zlib-ng we use GitHub Actions. You might want to take a look at that. https://github.com/zlib-ng/zlib-ng/tree/develop/.github/workflows
@nmoinvaz Right. Will do. :) I mentioned earlier in the thread that GitHub Actions weren't an option when we first looked because of security. But something about that has changed since then, so I'll look into it.
So, with https://github.com/dankegel/zlib/tree/env-envy, you can pass --junit foo.xml to example by setting ZLIBTEST_JUNIT="$($env:APPVEYOR_BUILD_FOLDER)$($env:CONFIGURATION)\junit-results.xml" in appveyor.yml before running ctest.
Wait, that's kinda dumb, lemme make sure it works with linux, where I need to run both example and example64.
Kinda tempting to make the env var prefix ZTEST_ and the commandline option prefix --ztest_.
Good idea to get the environment variables. I'll try that.
I'm not sure I like the idea of moving things to globals though. I could imagine a situation where we need to use two CI providers (maybe to have an external logging site?). If we do globals, we need to run the tests twice. If we use formatters, we run the tests once and produce two results.
I don't even like that I used a global for the sprintfs. I needed the lifetime to live beyond the function (so couldn't be local).
That said, I'm happy to do it if people like it.
(BTW Dan, I think you can fork my repo and submit pull requests there to get your contributions counted as part of this PR.)
Not sure I understand the global vs. formatter thing. I'm just running once, and everybody's happy, I think.
Pull request coming...
Having forked madler/zlib, I can't fork yours :-(
https://github.com/dankegel/zlib/tree/develop adds a --force_fail option which injects a fault to test the annotations. (It also rejiggers the xml to be closer to what I was generating before, because I wasn't seeing annotations in github; still not quite sure about it, but at least clicking on the head commit in my branch shows nice test failure annotations on the responsible source line.)
https://github.com/dankegel/zlib/tree/develop has been tidied up (and, since I couldn't submit it as a pull request against Chris's branch, submitted for review as https://github.com/madler/zlib/pull/495 ). It includes cirrus-ci builds, adds options/env vars (--force_fail, --fail_fast) that are useful when doing ci, and has a couple minor cleanups, including better failure annotations on github.
https://github.com/dankegel/zlib/tree/moreci includes examples of using other CI infrastructures (travis, drone) that reach more CPU types.
For key infrastructure, backwards compatibility is pretty important, and it's customary to give a year or so's warning before dropping it, so developers can prepare or object.
zlib's bunkmate libpng seems to have warned in 2011 that K&R support was going away, https://sourceforge.net/p/libpng/code/ci/baeb6d1e920ecfc321b1d5e7291345dcb424d205 and it finally went away with version 1.6.0 in 2013.
Amazingly, there may still be a few K&R-only compilers around, e.g. HP/UX's bundled compiler: https://support.hpe.com/hpesc/public/docDisplay?docId=emr_na-c01158951 https://stackoverflow.com/questions/43762148/k-r-function-declaration and $deity knows what compilers are still in use for embedded systems.
So FWIW if zlib has not yet warned that K&R support is going away, I'd suggest making the announcement now but staying K&R compatible through the end of the year.
The alternate viewpoint (kill K&R with fire) has merit, though.
I noticed there is Cirrus CI, Drone CI, Travis CI, and AppVeyor. Are you adding all the CI platforms? Would you be open to GItHub Actions as well or is AppVeyor settled?
It is possible to use CMake and qemu to run the emulated tests for platforms that are not natively supported by one CI platform or another.
I doubt we'll be adding all those CI platforms, they're just examples.
I'm aware of the qemu approach and am looking at it now.
For key infrastructure, backwards compatibility is pretty important, and it's customary to give a year or so's warning before dropping it, so developers can prepare or object.
Very much agree with that statement.
I guess even knowing that zlib is used in some crazy environments, I still didn't expect there would still be environments that require K & R C in 2020.
LGTM except for the readability problem in handle_test_results, and the somewhat nonstandard name of the tooling directory.
Great to know your effort of adding CI in zlib repo.
I think multi-arch support is also very imortant, so looks like AppVeyor is not the best choice. and travis ci support amd64, ppc64le, s390x, arm64, maybe it's a better choice?
[1] https://docs.travis-ci.com/user/multi-cpu-architectures/#testing-on-multiple-cpu-architectures
As someone pointed out, qemu will help a lot here.
For what qemu can't do: until zlib has its own build farm, the project may need to use several CI services to maximize coverage on the platforms it supports.
Landing something like https://github.com/dankegel/zlib/commit/75d462d266a4865a40b7d95f20688a02beb209e0 could add Power and S390x coverage via Travis, and https://github.com/dankegel/zlib/commit/fab53b9f2507f3bb6d0b0da10f93d017efa91d63 could add ARM coverage via Drone. (And potentially the project could use AWS Graviton directly for some ARM coverage.)
There may be some security hoops that need to be jumped through first, though. (e.g. is everyone really comfortable with letting each new cloud service nose their way into the zlib team's tent?)
I agree with @dankegel's suggestion that the CI should run the tests the same way the manual tests do.
Since the manual tests are run by invoking ctest, I changed the CI to reflect that. And since I cannot find a way to pass command line parameters to the individual tests, I used an environment variable like Dan suggested.
The option for passing --junit file.xml is still there.
Thank you for that suggestion. :)
@Yikun I agree that we'll want coverage of every platform we support, ideally.
This pull request isn't supposed to completely fix every problem. It is only supposed to get the ball rolling. (Similarly, we should probably move to a proper test framework and add more test coverage. But doing all of that here would make for a monster of a pull request.)
In time, we'll get there. :)
I separated the stdout and junit formatters. This results in easier to read code and more room for extensibility (if we want to later add other output formats).
With that done, I feel like I've adopted every comment that seems to fit well into this initial CI pull request. Other suggestions felt to me like they fit best in a separate pull request that builds on top of this one.
@dankegel mentioned LGTM with some changes. Those changes have been made. I now feel this LGTM too (as in, I'm comfortable with the idea of this landing).
I'll look into GitHub Actions next and what that would take. I think (not yet completely sure) that in order for it to be secure, we would need a monster commit. My understanding for this is we would want all of the actions we use to be part of this repo, rather than an external dependency. Once I have a better grasp, I'll comment here. If it wouldn't require a monster commit, I'll look into changing from AppVeyor to GitHub Actions.
Lots of activity here. Thanks!
So what does this do? How does it work? How is it enabled? (Imagine explaining these things to a small child ...)
Also how do I know that the work on this pull is complete?
When ever someone creates a pull request, it triggers the CI. It will fetch their code, build it, and run the tests. It then marks their pull request as either passing or failing those tests. You can see an example of that here: https://github.com/ProgramMax/zlib/commits/develop . The green checkmarks and red Xes indicate whether each commit passed or failed. (It can fail because it failed to build or it failed the tests.)
The goal is to make it easier to accept people's pull requests with the confidence gained by knowing it passed these tests.
If someone's pull request isn't covered by a test, it would be wise to add a test first to keep our confidence high as we accept other pull requests. This also includes platforms. If someone submits a pull request that is specific to Arm for example, we'll want to make sure we have an Arm builder & tester in place. To keep this PR small and not bite off too much in one go, this PR only adds an x86-64 builder/tester as seen here: https://github.com/madler/zlib/pull/492/files#diff-1281cd94f04bdff7ff3842eb2f303597R18
Just below that line you'll see it builds Debug and Release configurations, which are generated through CMake. Correspondingly, when the CI runs, it produces separate results for the separate configurations: https://ci.appveyor.com/project/ProgramMax/zlib/builds/33108019
If you click one of those configurations from the AppVeyor results page, you can see the console output as the bot ran (including CMake, building, and running the tests). You can also see tabs for Messages (like compiler warnings) and Tests (each test case separated out).
As a fun side benefit, if you go to https://github.com/ProgramMax/zlib/tree/develop and scroll down past the files you will see a badge that indicates that the develop branch is passing its tests. This isn't required. But it is common enough to include here.
The work you would need to do if you accept this PR is (this is from memory so might not be 100% correct):
- Go to AppVeyor.com, sign up, add your github zlib repro as a project.
- Once done, the AppVeyor project settings page will include a "Webhook URL" that we'll need.
- At the top of https://github.com/madler/zlib under the "Watch", "Star", and "Fork" buttons is probably a "Settings" tab. On that page is a "Webhooks" button on the left. Add the Webhook URL.
- On the AppVeyor project settings page, set the "Custom configuration .yml file name" to ci/appveyor.yml
- Merge this pull request
It may not trigger automatically on existing pull requests. I believe we can manually trigger them. Plus, as people make additional changes to their pull requests it will begin triggering.
There is one extra change that will need to be made. I can make it here if you want. At the bottom of README.md, you'll want to change the lines for [appveyor-shield] and [appveyor-link] to use the URLs you'll get from AppVeyor. You can find your URLs on the AppVeyor project settings page, under the "Badges" button on the left. It should look like: [appveyor-shield]: https://ci.appveyor.com/api/projects/status/your_api_key_here/branch/develop?svg=true [appveyor-link]: https://ci.appveyor.com/project/madler/zlib/branch/develop