Test multiple LLVM versions #4745
Conversation
chantra
requested review from
drzaeus77,
goldshtn,
yonghong-song,
4ast,
brendangregg and
davemarchevsky
as code owners
chantra
force-pushed
the
custom_llvm_docker
branch
from
c19daaa
to
b123f91
Compare
.github/workflows/bcc-test.yml
Outdated
| @@ -21,7 +21,8 @@ jobs: | |||
| runs-on: ubuntu-20.04 | |||
| strategy: | |||
| matrix: | |||
| os: [{distro: "ubuntu", version: "18.04", nick: bionic}, {distro: "ubuntu", version: "20.04", nick: focal}] | |||
| os: [{distro: "ubuntu", version: "18.04", nick: bionic, installed_llvm_versions: "11 12"}, {distro: "ubuntu", version: "20.04", nick: focal, installed_llvm_versions: "11 12 15"}] | |||
There was a problem hiding this comment.
IIUC, installed_llvm_versions / llvm_versions is used to populate almost everything, while llvm_version here is just needed for a few things:
- job name below (
name: Run bcc build - llvm-${{ matrix.llvm_version }}) LLVM_ROOTbelow
Do these need to be kept in sync? e.g. here you have os: line w/ installed_llvm_versions: "11 12" and installed_llvm_versions: "11 12 15" - does the llvm_version: line added one line below need to contain all the installed_llvm_versions ?
There was a problem hiding this comment.
Aside from this comment, everything in this PR makes sense to me, I think it's good to go
There was a problem hiding this comment.
so... this is kind of messy, but I did not find a better way to fix this. Well, a simple way would be to retire Ubuntu 18.04.
{distro: "ubuntu", version: "20.04", nick: focal, installed_llvm_versions: "11 12 15"}
Essentially drives the container creation/usage, this installed_llvm_versions indeed specify that we want to install LLVM versions 11, 12, and 15 in the container.
The llvm_version below drives the matrix.
The problem is that GH will generate a combination of all os/llvm_version/env tuples. There is no way to associate a separate llvm_version for ubuntu bionic, or infer the llvm_versions from the installed_llvm_versions.
Which explains there is the exclude specifically for ubuntu bionic and llvm 15.
llvm_version: line added one line below need to contain all the installed_llvm_versions
it is rather the opposite.... installed_llvm_versions needs to be a superset of llvm_version. The former drives what is installed during docker build in the container, the latter, which of those specific versions we are going to use.
In the PR I commented:
Ubuntu bionic had failure installing llvm 15, so I ma skipping it for that version of ubuntu.
If we just retire bionic, I think it would simplify this workflow.
Separately, I think the container logic could be aggregated in a separate action that can be re-used eliminating code duplication. I will give this a try.
There was a problem hiding this comment.
Thanks for the explanation!
If we just retire bionic, I think it would simplify this workflow.
No objection from me, but let's not hold up this PR for it
There was a problem hiding this comment.
let me follow up with the retirement from that build. It does not make much sense to keep it when the distro itself is EOL.
This change allow passing a list of llvm versions to be installed in the container. At a later stage, we can use this to force the version of llvm used to build bcc. Fedora only install the version of llvm from its repo. Bionic had failure installing llvm 15.
Default to LLVM-11
During the bcc-test workflow, we do not really need `installed_llvm_versions` as we only need to build the ephemeral container for the llvm version we are testing (e.g matrix.llvm_version). This container is only built IFF a docker file is changed in order to make sure the PR runs on a container which has the changes from the docker file. At the end of the day, the container that we use only needs 1 llvm version installed, the one we test.
Both f34 and f36 are EOL: https://docs.fedoraproject.org/en-US/releases/eol/ This chnage remove outdated Fedora containers and use F38 instead, the latest Fedora release.
chantra
force-pushed
the
custom_llvm_docker
branch
from
b123f91
to
74d6daa
Compare
Ubuntu 18.04 is EOL. This change remove it from CI as a follow-up from iovisor#4745
Ubuntu 18.04 is EOL. This change remove it from CI as a follow-up from iovisor#4745 Signed-off-by: Manu Bretelle <[email protected]>
Ubuntu 18.04 is EOL. This change remove it from CI as a follow-up from #4745 Signed-off-by: Manu Bretelle <[email protected]>
Currently, only llvm-11 get tested on Ubuntu, on Fedora, it is whatever is the default llvm package of that Fedora release.
Given the checks on LLVM versions in the code base, it would be useful to test different LLVM versions too.
This change allow installing multiple versions when building the Ubuntu docker image, and when building bcc, LLVM_ROOT is used to force building against a specific llvm version.
Ubuntu bionic had failure installing llvm 15, so I ma skipping it for that version of ubuntu.
On the other hand, I am not sure how much value testing on Bionic has. Given the tests are running in docker, the kernel bcc is tested against are the same so.
Also Bionic reached end of standard support https://ubuntu.com/18-04 .
Removing this Ubuntu flavor would make sense in my opinion. Happy to do this on top of this stack.