-
Notifications
You must be signed in to change notification settings - Fork 147
Upload build and install artifacts from Github actions #231
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
Conversation
a89b038 to
192e711
Compare
Seems to include api fingerprints?
This avoids a warning for the no-rdiff matrix case and lets us make it an error for other cases
dbaarda
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a few thoughts on this...
.github/workflows/check.yml
Outdated
| with: | ||
| name: rdiff ${{ matrix.os }} ${{ matrix.build }} | ||
| path: | | ||
| ${{github.workspace}}/build/rdiff |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that unless its a static build, the rdiff executable will also need the librsync library to run, which is either the .so (and related symlinks) on linux or the .dll on windows.
But perhaps, rather than manually selecting the bits to put in the artifact, perhaps we should do a cmake --build '${{github.workspace}}/build' --target install and then build an artifact out of everything that is installed. That way we should get the executables, supporting libraries, required headers to use those libraries, and supporting man pages.
That also means we don't need to selectively exclude the no-rdiff case, as we probably also want to build/check the artifact you get from installing only the library without rdiff.
I'm not sure what happens on windows when you run a 'cmake --target install`, and it wouldn't surprise me if it's broken. However, this is probably just something else we need to fix.
The bad news is my experience with trying to make windows work for the github actions was kinda painful, but we are pretty close now to a working windows setup. At least github actions runs nice and fast for testing all this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if just building a self-contained static executable would be more helpful for people who want the executable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We now have install artifacts for all builds that we test. Currently, we only test static linking on Linux, so we don't have a static executable for windows, only a dynamic one. We could add another static build on windows, but I don't know if we need one.
|
The artifacts thing is interesting... I'm finding out more and more about what can be done with github actions and it's pretty cool. I'm starting to think about maybe having actions that automate much of the release process too. One thing that worries me a little bit is... this is all a bit TOO cool... building/keeping all these artefacts for every checkin is surely going to eat resources. What are the freebie limits on all this? Is there some kind of retention-horizon or quota limit on these? It would be nice to extend the release instructions in CONTRIBUTING.md to include uploading the windows binary as another "asset", the same way we grab a snapshot of the release tarball. People on Windows are far more likely to need a precompiled binary. I can update the CONTRIBUTING.md after we get this working nicely. |
They're kept for 90 days by default: https://docs.github.com/en/actions/reference/usage-limits-billing-and-administration#artifact-and-log-retention-policy |
Automating the release process sounds like a great idea, not only to reduce toil but also to move further towards https://security.googleblog.com/2021/06/introducing-slsa-end-to-end-framework.html |
Adds a "Build install" step and changes "Upload rdiff" to "Upload install" instead. Removed the "no_rdiff" setting since we always upload the install, rdiff or not.
|
I changed this to include the "build install" step and made the rdiff artifact into an "install" artifact. I also added the "options" dimension to the artifact name, so we can see all the build and install results. I'm happy with this now... do you want to check it and see? How do you want to handle the merge? Normal merge, squash merge, rebase merge, or some kind of manual cleanup of the history? I'm not that fussed either way. |
dbaarda
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is ready to merge... happy to let you do this in whatever way you think is best.
|
If I don't hear otherwise, I'm going to do a "Squash and merge" of this PR tomorrow. |
|
I'm going to squash and merge this now. |
|
Thanks! |
This makes downloadable artifacts out of the whole build tree and install results for every Check run. The build tree is perhaps useful for understanding build failures. The install results gives us compiled binaries and libraries that can be directly installed, which is particularly useful for platforms like Windows.