[cpullvm] Use cmake 4.4.2 for Linux x86_64 builds

[srilakshmi yalamaraju (sriyalamar)]

LLVM 24 requires cmake >= 3.31.0. The DMZ self-hosted runner has cmake 3.28.3 system-wide which is insufficient.

[Eli Friedman (efriedma-quic)]

Any path that only works on a specific builder should be part of the configuration of that builder.

Maybe we can just mess with the PATH on the builder?

[Jonathon Penix (jonathonpenix)]

FWIW I think the problem with touching the PATH on the builder is that the same builders/environment is (unfortunately) shared across different versions so we'd have to check nothing goes wrong with 22/23 as well at the same time.

[Eli Friedman (efriedma-quic)]

We can make the GitHub Action set an environment variable, if we want to restrict it to a specific build config.

[Navaneeth Shanmugasundaram (navaneethshan)]

I completly agree, this build.sh update will not work on the servers which do not have this path.

srilakshmi yalamaraju (@sriyalamar), lets try the WF update like below (fix the indentation), and you don't have to change this sh file?

- name: Setup CMake 3.31.0
uses: jwlawson/actions-setup-cmake@v2
with:
cmake-version: '3.31.0'

https://github.com/marketplace/actions/actions-setup-cmake

[Navaneeth Shanmugasundaram (navaneethshan)]

May be the build.sh can just check if the cmake version is >= 3.31.0 and fail otherwise.

[Jonathon Penix (jonathonpenix)]

+1 to Eli's comment--I'd prefer we avoid this sort of thing unless we absolutely have to (ex: to unbreak builders if/when this upgrade lands upstream).

Navaneeth Shanmugasundaram (@navaneethshan) my assumption is that changing how our builders are hosted will let us more naturally fix this since we can/could have multiple different images (with different cmake versions and/or PATHs set (or whatever other dependency)). Is that right/is this slated to come early enough that we might be able to just wait for that?

[Navaneeth Shanmugasundaram (navaneethshan)]

+1 to Eli's comment--I'd prefer we avoid this sort of thing unless we absolutely have to (ex: to unbreak builders if/when this upgrade lands upstream).

Navaneeth Shanmugasundaram (Navaneeth Shanmugasundaram (@navaneethshan)) my assumption is that changing how our builders are hosted will let us more naturally fix this since we can/could have multiple different images (with different cmake versions and/or PATHs set (or whatever other dependency)). Is that right/is this slated to come early enough that we might be able to just wait for that?

I am not sure what you refering to, you mean using docker image or AWS?

[Navaneeth Shanmugasundaram (navaneethshan)]

srilakshmi yalamaraju (@sriyalamar), did you test: https://github.com/marketplace/actions/actions-setup-cmake ? good if we don't hardcode the path anywhere.

[Eli Friedman (efriedma-quic)]

did you test: https://github.com/marketplace/actions/actions-setup-cmake ?

I'd rather not add dependencies from unknown third parties; if we're going to download CMake as a build step, we should just write that out explicitly.

We could download CMake as a build step instead of having it preinstalled on the bot (wget https://github.com/Kitware/CMake/releases/download/v4.4.2/cmake-4.4.2-linux-x86_64.tar.gz etc.), but I'm not sure how much that really helps. Not having an automated script to reproduce the buildbot's setup is a problem in general, but I don't think this makes that problem significantly worse.

[Jonathon Penix (jonathonpenix)]

A few scattered responses/comments below.

On the patch itself:

• Whatever we do, we need to make sure it applies to all the appropriate jobs that use DMZ machines in mainline.
  
  There are other workflows that I'd expect to be touched, as well as other jobs in nightly.yml that I'd assume need to be touched.
  
  Unless the implication is that setting the PATH once here is persistent between different jobs/workflows, which, would be bad for other reasons I think if that is the case.
• If we do something like this, please make it only apply when we're using the DMZ machines.
  
  I realize this is currently the case, but extending what is currently done (just setting the PATH next to running the build script) to other builds that use a matrix with different runners is not ideal I think.

+1 to Eli's comment--I'd prefer we avoid this sort of thing unless we absolutely have to (ex: to unbreak builders if/when this upgrade lands upstream).
Navaneeth Shanmugasundaram (Navaneeth Shanmugasundaram (Navaneeth Shanmugasundaram (@navaneethshan))) my assumption is that changing how our builders are hosted will let us more naturally fix this since we can/could have multiple different images (with different cmake versions and/or PATHs set (or whatever other dependency)). Is that right/is this slated to come early enough that we might be able to just wait for that?

I am not sure what you refering to, you mean using docker image or AWS?

Navaneeth Shanmugasundaram (@navaneethshan) I suppose either--I mentioned AWS as my understanding is moving to AWS would mean we will finally move away from having these persistent machines/environments (between runs, jobs, etc.) as in the DMZ machines. But we could do the same thing by specifying docker images, etc.

The idea being that we can just install dependencies in the workflow itself or use different images with different dependencies installed, etc without worrying about interfering with other CPULLVM branches, etc. that might use the same machines (as is the case today).

did you test: https://github.com/marketplace/actions/actions-setup-cmake ?

I'd rather not add dependencies from unknown third parties; if we're going to download CMake as a build step, we should just write that out explicitly.

Navaneeth Shanmugasundaram (@navaneethshan) Eli Friedman (@efriedma-quic)

+1

FWIW too, I think we have some qualcomm-org-imposed checks that rejects most 3rd party actions usage (there's some allow list, but I don't know what exactly is and isn't covered and I agree with Eli about not using 3rd party things so would prefer we not pursue an exception for this).

We could download CMake as a build step instead of having it preinstalled on the bot (wget https://github.com/Kitware/CMake/releases/download/v4.4.2/cmake-4.4.2-linux-x86_64.tar.gz etc.), but I'm not sure how much that really helps. Not having an automated script to reproduce the buildbot's setup is a problem in general, but I don't think this makes that problem significantly worse.

I don't necessarily mind if we install it every run, but then it seems like we should be consistent and make sure we clean it up every run in the DMZ builders.

[Navaneeth Shanmugasundaram (navaneethshan)]

Thanks srilakshmi yalamaraju (@sriyalamar), I like this apporach, but I have below questions.

• This is going to use 4.4.2 even if later version is available on the machine, which may not be the expectation?
• In build.sh, we should add a check to fail if the cmake version is <4.4.2, so if build.sh is used outside of this WF, this or later version will be used.
  Minor note - don't force push, so we will have the history of commits while we review.

Thanks Jonathon Penix (@jonathonpenix) and Eli Friedman (@efriedma-quic) for the review comments so far, I agree on all of those.

[Navaneeth Shanmugasundaram (navaneethshan)]

Navaneeth Shanmugasundaram (@navaneethshan) I suppose either--I mentioned AWS as my understanding is moving to AWS would mean we will finally move away from having these persistent machines/environments (between runs, jobs, etc.) as in the DMZ machines. But we could do the same thing by specifying docker images, etc.

The idea being that we can just install dependencies in the workflow itself or use different images with different dependencies installed, etc without worrying about interfering with other CPULLVM branches, etc. that might use the same machines (as is the case today).

When we use AWS or Docker in future, we will use the images or AMI with correct version of cmake in it, but I don't expect the current implementation will break anything there. For now, we need a WF which will work on DMZ worker and GH runner, when we add AWS, I expect the WF to be updated a bit.

[Eli Friedman (efriedma-quic)]

This is going to use 4.4.2 even if later version is available on the machine, which may not be the expectation?

This seems fine. Pinning down the versions of tools we use will make the behavior more predictable.

[Jonathon Penix (jonathonpenix)]

When we use AWS or Docker in future, we will use the images or AMI with correct version of cmake in it, but I don't expect the current implementation will break anything there. For now, we need a WF which will work on DMZ worker and GH runner, when we add AWS, I expect the WF to be updated a bit.

Agreed, I was originally just asking if this changeover was going to happen soon enough that it would make sense to skip this interim step, but I'm assuming at this point the answer is 'no'.

In build.sh, we should add a check to fail if the cmake version is <4.4.2, so if build.sh is used outside of this WF, this or later version will be used.

I'd prefer we don't do this. In general the cmake minimums should be enforced in cmake itself (cmake_minimum_required)--I don't think there's much reason to have build.sh fail but directly specifying the cmake command to succeed. Especially on such a recent version of cmake

I don't know about others, but at least I occasionally use the build.sh scripts (or variants for overlays, etc.) when doing things locally.

[srilakshmi yalamaraju (sriyalamar)]

Thanks Eli Friedman (@efriedma-quic), Jonathon Penix (@jonathonpenix), and Navaneeth Shanmugasundaram (@navaneethshan) for your reviews and suggestions.

It looks like we are in good shape with this commit. Based on the latest logs, the CMake warning related to the upgrade has also been resolved.

Could you please approve the PR when you have a chance.

[Navaneeth Shanmugasundaram (navaneethshan)]

I don't know if we have a nightly test build avialable to look at, I started one: https://github.com/qualcomm/cpullvm-toolchain/actions/runs/32753816743/job/97821249951?pr=597

[Navaneeth Shanmugasundaram (navaneethshan)]

When we use AWS or Docker in future, we will use the images or AMI with correct version of cmake in it, but I don't expect the current implementation will break anything there. For now, we need a WF which will work on DMZ worker and GH runner, when we add AWS, I expect the WF to be updated a bit.

Agreed, I was originally just asking if this changeover was going to happen soon enough that it would make sense to skip this interim step, but I'm assuming at this point the answer is 'no'.

In build.sh, we should add a check to fail if the cmake version is <4.4.2, so if build.sh is used outside of this WF, this or later version will be used.

I'd prefer we don't do this. In general the cmake minimums should be enforced in cmake itself (cmake_minimum_required)--I don't think there's much reason to have build.sh fail but directly specifying the cmake command to succeed. Especially on such a recent version of cmake

I don't know about others, but at least I occasionally use the build.sh scripts (or variants for overlays, etc.) when doing things locally.

At this point, there is no plan to move to AWS completely, it will be hybrid option, DMZ+ AWS.

[Navaneeth Shanmugasundaram (navaneethshan)]

I don't know if we have a nightly test build avialable to look at, I started one: https://github.com/qualcomm/cpullvm-toolchain/actions/runs/32753816743/job/97821249951?pr=597

LGTM.

[Jonathon Penix (jonathonpenix)]

I guess I'm fine if this goes in as-is if it is going to be a temporary/interim thing.

Long term, I don't think this is how we should be handling dependencies though/I don't think we should be using this as a model going forward:

• Having this one dependency handled differently than any other just makes things confusing. And I don't think we really get anything out of making this a special workflow + baking heuristics into dependency-specific workflows seems like it would/will get messy quickly. (Our install scripts are not ideal, but at least those are a consistent set of expected dependencies for a given "type" of builders.)
• The actual logic in .github/actions/setup-cmake/action.yml doesn't actually look like it is along the right "lines" to me either in general. In that something being a given OS/arch (where OS is apparently along the lines of "Linux") combo doesn't really say anything about whether we have a suitable cmake--we're really targeting specific builders/images here.
  
  (Our current image for AArch64 Linux builds is fine, but if we have to go back to a Ubuntu 22 build to test, it might not be. Ubuntu26 x86 Linux builds in theory should have an up-to-date enough cmake on it's own. Etc.
  
  And arguably it might just be better to use a consistent version of ex: cmake everywhere, but since that's not what we're doing here I'm setting that aside)