Add continuous integration using GitHub Actions - #52
Conversation
|
IIRC, @ctruta has CI running (just not on GitHub). (IIRC the reasoning was to split CI up across several free services so we don't hit the free limit from GitHub. That said, I don't think we're close to hitting it so I would favor the CI running here.) |
Seconded. After all what is the point of not using github CI at all? I trust no one has given out their credit card :-) so if the allocation runs out the CI tests stop (I assume). |
|
Thanks for the positive response! Actually GitHub Actions for public repositories doesn’t stop, so the worry could be set aside 😉 |
Thank you. @ctruta, @svgeesus, @ProgramMax this guy should get a credit in the libpng documentation. |
a774f23 to
e520d9e
Compare
|
The intent is great, but I'm aware of my own shallow knowledge in this area and trust @ctruta to spot any problems here, or approve it if there are none. |
|
Hello, apologies for my delayed review. I promise to take a careful look tomorrow. |
| @@ -0,0 +1 @@ | |||
| build/ No newline at end of file | |||
There was a problem hiding this comment.
This is super-duper-incomplete, especially considering that we have plenty of non-CMake artifacts as well. For a plain C project, the .gitignore file should contain (at least) something like this:
https://github.com/github/gitignore/blob/main/C.gitignore
There was a problem hiding this comment.
@ctruta This was only meant to be barebones since the project never had a .gitignore file and there was no indication what the project wanted in terms of such a file. Would you want me to copy in the contents of the C gitignore template from GitHub?
There was a problem hiding this comment.
@ronaldtse that's a good file as a barebones starter, but it's far from ideal (e.g. as I had negotiated with @jbowler about what to put in libpng, there's no exclusion for temporary files created by common editors like Vim and EMACS).
Please give me a few minutes, until I steal something from my other PNG projects, and then I'll ask you to review.
There was a problem hiding this comment.
The gitignore file in this PR is now removed.
| find_package(ZLIB) | ||
| if(ZLIB_FOUND) | ||
| target_link_libraries(pngcheck PRIVATE ZLIB::ZLIB) | ||
| target_compile_definitions(pngcheck PRIVATE USE_ZLIB) |
There was a problem hiding this comment.
From commit 00f4d73 onwards, this is no longer necessary.
There was a problem hiding this comment.
Removed as requested.
| message(STATUS "Using local ZLIB subproject (location not available)") | ||
| endif() | ||
| target_link_libraries(pngcheck PRIVATE zlibstatic) | ||
| target_compile_definitions(pngcheck PRIVATE USE_ZLIB) |
There was a problem hiding this comment.
Removed as requested.
ctruta
left a comment
ctruta
left a comment
There was a problem hiding this comment.
We removed the makefiles for the Windows builds, leaving the CMake dependency which is a standard requirement in a Windows programming workflow nowadays. (Integrated in the Visual Studio IDE, shipped with the Visual Studio installer, standard availability in Cygwin, MSYS2, etc.)
I would be glad to take in a CI automation, but may I please suggest splitting the CI contribution from any local build contributions? We can debate about taking in (or not) the makefiles. My refutation is about the simplification of testing and maintenance, and that includes the simplification of the CI build matrix.
As for the .gitignore file, if you please don't mind me, I will submit a competing PR. It took me a while to refine what to exclude and whatnot, for makefile artifacts, build system artifacts, IDE artifacts, etc. etc.
So, to conclude, if you could please resubmit, focusing exclusively on the additions required for verification via GitHub Actions, that'd be great!
|
Happy to change this in any way as long as it helps. Personally, I think the Makefiles are a burden to maintain if there is already CMake. Would the following changes be sufficient?
|
e520d9e to
c001924
Compare
|
@ctruta I've made the changes as requested, and will move the Windows Makefiles and Makefile CI workflow to a new PR if anyone wants it in the future. This is ready for review again. |
c001924 to
9d76a09
Compare
Thank you. I had a brief look and I will continue tomorrow, but an overall comment that I'd like to make: the build matrix is just too expensive. We're using 100% portable C, nothing fancy or system-specific, so, if you're testing both 32-bit and 64-bit platforms, that's cool; but if you're testing on old and new flavours of Ubuntu, that's just time wasted. Even more so on I mean, sure, it's "free" for us, until we exceed our minutes (if we do exceed our minutes at all) but my point is this: for any extra test that you add, make sure you have a reason for it. You should be fine if you're just running the tests on And then, there's the Unix Makefile. You shouldn't bother with verification of the Windows makefiles because those are gone and we aren't bringing them back; but the Unix Makefile that we're currently having, is, well, here already, and that one should be tested. But make it Ubuntu-only, please, because one Unix platform to test should be sufficient, especially considering that you aren't even testing |
|
I'm not sure about other users, but our users rely on pngcheck extensively on various platforms and architectures (amd64 and arm64). There are platforms that we could omit, for sample an older Ubuntu or a macOS version. (I haven't even filed an issue that a user is asking for NetBSD support of pngcheck.) Here's the justification for the matrix:
So we can remove These are all platforms that people (at least our users) use pngcheck on.
GitHub actually provides "unlimited minutes" for public repositories to support open source, so there is actually no monetary cost concern here. The only cost involved is the developer wait time for running tests with the potential maintenance of the build workflow.
Sure, I'll update that. |
|
@ctruta the requested changes have been made. Hope this is good to go! |
Acknowledged. I started the review. I want to do my own testing also, in order to get a better understanding of a bunch of things, and I disclose that I'll be stealing from your work 😉 I will complete the review by EOD today. |
|
@ctruta all to the benefit of PNG and its users! 🥂😜 |
|
So I haven't had a chance to run my own tests -- I was busier than expected -- but I did look over your changes, and they look good. Very thorough 👍 |
ctruta
left a comment
ctruta
left a comment
There was a problem hiding this comment.
Good morning,
I just realized I haven't selected the "Approve" button, although I accepted with these changes. May I please ask for one more change, not in the commits but in the commit messages:
Specifically, could you please amend the first commit so that the message body (especially the first line) does not exceed 72 characters? Several of Git tools, including GitHub's own web interface, rely on this limit -- for example, it splits the subject line of commit 9d76a09 like this:
feat: add github actions for cmake and make: ubuntu, macos, windows, f…
…ixes #50
My suggestion: just drop the ending (fixes #50) and that will fix the problem. Over the decades, we've moved our PNG software over a variety of hosting services, 'cause nothing lasts forever, not even GitHub, so these numeric fields don't carry that much informational value.
In the same manner, for the last line
Contribution from Ronald Tse (@ronaldtse) of Ribose (@riboseinc/@metanorma)
it's better if you use a trailer that is understood by the Git tools. (For example, see git help commit and scroll down to the --trailer option. For example, here's a suggestion:
Contributed-by: Ronald Tse (@ronaldtse) of Ribose (@riboseinc/@metanorma)
Admittedly, this suggestion does exceed the 72-character limit, too, but the trailers in commit messages have a special status, they are expected to be one-liners, so the line length is not a hard requirement in the way that it would be for regular prose.
* Fixes pnggroup#50 Contributed-by: Ronald Tse (@ronaldtse), Ribose (@riboseinc/@metanorma)
9a89a05 to
7c30964
Compare
|
@ctruta Just pushed the requested changes! If you click on the "Approve" this workflow run then you can see the CI running in this PR. |
ctruta
left a comment
ctruta
left a comment
There was a problem hiding this comment.
If you click on the "Approve" this workflow run then you can see the CI running in this PR.
(👀)
|
This got broken due to GitHub retiring |
5 participants
Fixes #50.
(This PR was split from #51 , for your review @jbowler )
The current repository did not implement continuous integration and it is hard to ensure that the latest code and build methods work across the multitude of supported platforms.
We have added comprehensive workflows using GitHub Actions that test all supported build methods across multiple platforms, including:
In accordance to INSTALL.md which describes CMake and Makefiles as both accepted/supported methods to build, two workflows are created:
build.ymltests the CMake approach on all supported platforms (this PR)build-makefile.ymltests the Makefile approach on all supported platforms (this is now in Add Makefile build workflow with Windows Makefiles #55)Both workflows share a build matrix configuration which is encoded at
matrix.json.Specifically:
The Windows Makefiles made a return (containing edits from @groelofs and @jbowler) to support the Makefile builds
CMake builds use the following setup:
vcpkgintegration with arm64 supportMakefile builds use the following setup: (this is now in Add Makefile build workflow with Windows Makefiles #55)
makenmakewithMakefile.w32makewithMakefile.mingw(32/64-bit)An additional workflow for packaging a release at GitHub Releases is added which is triggered by pushing of a version tag (i.e.
v*).Caveat:
Credits