checkpoint-restore / criu

Checkpoint/Restore tool
criu.org
Other
2.93k stars 585 forks source link

Add CI workflow to ensure self-contained commits #2216

Closed snprajwal closed 1 year ago

snprajwal commented 1 year ago

Currently, podman and checkpointctl both use a CI workflow to determine the delta in binary size with and without the commits in a PR. If it is beyond a certain threshold, the workflow fails, alerting maintainers to take a closer look at the code. If this is not already present in CRIU, it would be beneficial to track it. A good side effect is that it also ensures every commit in a PR is self-contained.

cc @rst0git @adrianreber

adrianreber commented 1 year ago

Not sure this is that important for CRIU itself. CRIU does not import code like a Go binary would. Everything is linked dynamically. So the binary size is mainly dependent on the existing code. The problem with Go binaries is that including a library for a small feature can pull in lots of files and increase the binary size. I am not sure that that is a problem for C code.

snprajwal commented 1 year ago

Understood. Would we still want a workflow that ensures self-contained commits?

rst0git commented 1 year ago

It would be good to test that CRIU builds and works after each commit, and that commits are introduced in the correct order (e.g., a commit for a test of a new feature is added after the commit introducing that feature). We currently have the following text in the guidelines for contributing:

When dividing your change into a series of commits, take special care to ensure that CRIU builds and runs properly after each commit in the series. Developers using git bisect to track down a problem can end up splitting your patch series at any point; they will not thank you if you introduce bugs in the middle.

In the past, when we used a mailing list for patches, the person applying a patch-series would manually check that everything works as expected. With our current approach, this is part of the review process for GitHub pull requests.

@checkpoint-restore/maintainers What do you think?

avagin commented 1 year ago

@rst0git I like the idea. It should compile criu and run a few basic tests after each commit.