search

cps-org / cps-config

A drop in replacement for pkg-config/pkgconf using cps files
MIT License
15 stars 7 forks source link

CI: Use Docker for CI #46

Closed lunacd closed 2 months ago

lunacd commented 2 months ago

Changes

  1. Under tests/docker, added two dockerfiles that builds docker images for testing cps-config. The two dockerfiles are for ubuntu and rockylinux currently. More platforms can be added later.
  2. In GitHub Actions workflow, switched from building and testing directly within the workflow to building and using docker images.
  3. Partly to demonstrate the benefits of this approach, added testing for rockylinux within CI pipeline.
  4. Because the CI pipeline is no longer testing ubuntu specifically, rename the pipeline to "test", expecting that in the future, more workflows, like "lint", would be added.

Benefits

  1. Developers would be better able to reproduce CI results without repetitively pushing and triggering GitHub Actions.
  2. A single GitHub Actions workflow file would be able to run tests on multiple distributions seamlessly, where all the platform-specific behaviors are handled within dockerfiles for each distribution.
  3. In the future, other CI workflows, like running clang-tidy on the codebase, might be interesting. With docker, we will be able to reuse platform-specific setups and have one workflow file per type of check.
  4. Docker builds can be cached. This could lead to much faster CI pipeline builds.

Limitation and future work

  1. It is not entirely clear to me whether docker build cache includes ccache mounted with type=cache. Intuitively, it should, but I couldn't find official docs that say so. It that is not cached, this would be a step back from the previous CI setup. In that case, a future PR that caches ccache artifacts would be needed.
  2. Currently building the dockerfile also builds cps-config. This might not be necessary for some workflows. It might be interesting to do that as part of the workflow rather than the image build process.
  3. It would be nice to document local workflows to test cps-config with those dockerfiles, even if it's "go look at what is being used in test.yml". But that would fit into docs/dev.md which is currently WIP in #42. I will follow up with a docs PR once docs/dev.md is merged.

Testing

I have tested in my own repository, but it's after all just an approximation. Thus, I have created #47 to test that this CI pipeline correctly fails when there is a test failure.

lunacd commented 2 months ago

47 fails as expected. Failing CI run: https://github.com/cps-org/cps-config/actions/runs/8227179114/job/22494683293.

dcbaker commented 2 months ago

I'm not as familiar with github actions (particularly complex ones) as I am with gitlab pipelines, so this may not be possible (or easily possible), but one of the hopes I had for using docker was that we could decouple of the building of the base images from our push and pull request workflow. The Images could be built on a timer, or when a user manually updates them, and then we just re-use those images. I'm concerned that as we add more OSes and tests that the CI time is going to get really long.

lunacd commented 2 months ago

The Images could be built on a timer, or when a user manually updates them, and then we just re-use those images.

Not sure if I followed you correctly, are you saying that we publish to cps/cps-config or something on DockerHub and then in our CI just use the image to build CPS?

Please correct me if I'm wrong, but I don't think that would have any performance benefit over the workflow in this PR. Docker builds are cached by GH Actions by layer. If we don't change build dependencies, for example, docker/build-push-action@v5 or the cache-from: type=gha configuration specifically, will just pull the layers from GH Actions cache. This wouldn't be different from pulling the published image from a registry.

Further, the benefit of this is that it automatically detects when a layer is changed, e.g. adding/removing dependencies, and correctly invalidates the cache. If we publish the image, a PR that requires a new dependency, or one that updates the image from Ubuntu 22.04 to 24.04, is guaranteed to fail.

I'm concerned that as we add more OSes and tests that the CI time is going to get really long.

They are parallel so they probably wouldn't be too long. My concern is using too much cache to a point of GHA stopping us from doing so. But you are right, and I would propose:

  1. Have dockerfiles for a bunch of distros that we care about: ubuntu, rhel-like, fedora, debian, arch, etc.
  2. Have the CI run only 1-2
  3. Have a local workflow where a full platform compatibility test can be manually triggered. That would likely happen right before a release or something

Or we can have one primary distro we are testing multiple flavors on. All the others can just be baseline gcc. I'm flexible and this can be whichever people feel best about.

lunacd commented 2 months ago

Since the project is quite small now, I'm OK with taking a step back on ccache support to improve test coverage. If this PR merges, @lunacd, can you make an issue to revisit compile caching support?

I googled a bit and found this issue: https://github.com/moby/buildkit/issues/1512. The issue isn't directly related to this, but from the comments, it seems that cache-to only saves layer cache, not mounted cache. I'll make an issue to revisit this. Wouldn't be hard to get. We just have to manually manage volume/bind.

dcbaker commented 2 months ago

Ah, I'm demonstrating what I don't understand! It look like it's doing exactly what I hoped for then, this requires a bit more manual intervention on GitLab, so I was expecting more intervention on GitHub as well. Please disregard my comments, this all looks good from my point of view.

lunacd commented 2 months ago

Changes:

  1. dockerignore *.md
  2. Removed some unnecessary boilerplate comment in GH Actions workflow file