KhronosGroup / KhronosGroup/SPIRV-LLVM-Translator
Revise scope of CI testing
- 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
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