KhronosGroup / KhronosGroup/SPIRV-LLVM-Translator

Revise scope of CI testing

Open
#716 8 comments 0 reactions 0 assignees View on GitHub
CI enhancement
Dominant language
LLVM
Stars
625
Forks
279
Avg merge
3d 5h
Merged PRs (30d)
34

Description

This issue aims to collect feedback about current scope of testing done in CI in order to improve the situation

From #699:

@AlexeySachkov:
> I wonder why we haven't spotted the original build fail in CI.
>
> @svenvh, @AlexeySotkin, looks like we don't have a configuration without NDEBUG set (should be equal to LLVM release without assertions)

@svenvh:
> That might be worth adding indeed, as it would align better with LLVM configurations.
>
> Perhaps it's a good opportunity to revise the configurations being tested, because right now there are quite a few configurations. I am not sure there is value in testing all combinations for the majority of the pull requests as it consumes quite a bit of Travis time, which is particularly noticeable when there are many pull requests in flight.

@AlexeySachkov:
> Totally agree here.
>
> BTW, we might want to try GitHub Actions - one thing I love about it is that you can have several config files each of them describing different workflows (and each of them could be triggered by different actions).
>
> For example, having separate workflow for launching clang-format could save us some time there as we don't need to install all those packages to get clang-format and it allows us, for example, attach a patch-file which need to be applied to fix formatting. See https://github.com/intel/llvm/actions/runs/222983731 as an example

@svenvh:
> I am not familiar with GitHub Actions, but what you describe sounds useful indeed. I wonder if it would also allow testing based on the patch? For example if a patch touches CMakeLists.txt then it would run more configurations compared with a patch touching only .cpp files.
>
> Sorry for sidetracking in this PR btw, perhaps we should continue the discussion in a new issue about CI changes?

I propose the following short-term changes:

- add configuration, which perform build without assertions (classic Release build) to catch errors like improper `#ifdef NDEBUG`, unused variables around `assert`-ions, etc.
- drop almost half of existing configurations: I don't think that we need to run all configurations twice with g++ and clang++ compilers - if we care about both, I suggest we focus on one and add 1 or 2 configurations with another, just to have basic acceptance check - in 99% cases results should be the same

Also I propose to consider switching to [GitHub Actions](https://docs.github.com/en/actions) which might get us the following benefits:
- we can have several [workflows](https://docs.github.com/en/actions/configuring-and-managing-workflows/configuring-a-workflow) for different purposes like performing clang-format check and build + tests
- this should simplify our CI config files as each workflow is described in a separate config file - there will be no more bunch of `if`s to detect what exactly we need to do
- each workflow is displayed as a separate item in "checks" section of a PR page, which will help to quicker understand what exactly went wrong with particular PR (no need to explicitly go to travis to see that only clang-format failed)
- workflows are more flexible than travis jobs: we can launch particular workflows depending on target branch, files that changed in a PR or even react to comments in PRs and issues
- it is possible to generate some artifacts in GitHub Actions: for example we can publish a `.patch` file that contains required changes to fix formatting issues in a PR ([example from intel/llvm](https://github.com/intel/llvm/actions/runs/224538306))

Contributor guide

Open the contributing guide

Research direction

Review the existing Travis CI configuration and the CMakeLists.txt references mentioned in the issue, then compare the current compiler and assertion configurations with the proposed coverage. Done means the CI setup includes a non-asserting Release build, avoids redundant configurations, and separates any adopted GitHub Actions workflows for formatting and build checks.

Written by the indexing model from the issue text.

Assessment

Tech stack
cmake, cpp, github-actions
Domain
build-system, ci-cd, testing-qa
Issue type
Refactor
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.