Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bump cmake minimum required version to 3.20.0 #2064

Merged
merged 4 commits into from
Sep 18, 2023

Conversation

sstamenova
Copy link
Collaborator

This matches the current minimum required version in llvm (see https://reviews.llvm.org/D144509)

See #2063 as well

This matches the current minimum required version in llvm (see https://reviews.llvm.org/D144509)

Signed-off-by: Stella Stamenova <[email protected]>
@NathanielMcVicar
Copy link
Collaborator

LGTM, although somewhat painful as described in the linked issue.

@NathanielMcVicar NathanielMcVicar self-assigned this Mar 8, 2023
@gongsu832
Copy link
Collaborator

Unfortunately Ubuntu Focal, which our dev docker image is based on, is only up to cmake 3.16.3.

@sstamenova
Copy link
Collaborator Author

Unfortunately Ubuntu Focal, which our dev docker image is based on, is only up to cmake 3.16.3.

How are we going to address that once we try to update llvm to a commit that requires 3.20?

@gongsu832
Copy link
Collaborator

I will have to look into upgrading the base image to Jammy, which will have cmake 3.22.1.

@sstamenova
Copy link
Collaborator Author

I will have to look into upgrading the base image to Jammy, which will have cmake 3.22.1.

@gongsu832 Ok, I will leave this PR up to be completed once the base image supports it. I am not sure when (though I suspect very soon) LLVM will re-commit the new version requirement and one of the next llvm updates will need this.

@NathanielMcVicar
Copy link
Collaborator

Another option is to add the apt.kitware repo to the docker image setup. That might be lower friction until you are ready to move to jammy.

@gongsu832
Copy link
Collaborator

I'll try Jammy and see how many things break and proceed with it if not too many. Otherwise I'll look into apt.kitware. Thanks for the suggestion.

@gongsu832
Copy link
Collaborator

gongsu832 commented Mar 12, 2023

@sstamenova I'm wondering, if we upgrade our base image to Jammy, we will have to also upgrade the LLVM no-warning buildbot to Jammy, right? Since the buildbot config is supposed to match our build env.

@sstamenova
Copy link
Collaborator Author

@sstamenova I'm wondering, if we upgrade our base image to Jammy, we will have to also upgrade the LLVM no-warning buildbot to Jammy, right? Since the buildbot config is supposed to match our build env.

Yes, whichever way you use to upgrade cmake here will need to be applied to the buildbot since it will also require the newer version of cmake.

@gongsu832
Copy link
Collaborator

Yes, whichever way you use to upgrade cmake here will need to be applied to the buildbot since it will also require the newer version of cmake.

I tried Jammy and it was eventless. Everything pretty much just worked. So I will put out a PR to upgrade to Jammy. Once that's done, can I just create another Jammy based docker container for the LLVM no-warning buildbot, replicating all the configurations from the Focal based buildbot, and then shutting down the Focal based buildbot?

@sstamenova
Copy link
Collaborator Author

Yes, whichever way you use to upgrade cmake here will need to be applied to the buildbot since it will also require the newer version of cmake.

I tried Jammy and it was eventless. Everything pretty much just worked. So I will put out a PR to upgrade to Jammy. Once that's done, can I just create another Jammy based docker container for the LLVM no-warning buildbot, replicating all the configurations from the Focal based buildbot, and then shutting down the Focal based buildbot?

Assuming you connect with the same worker name and password, it should work.

@gongsu832
Copy link
Collaborator

Assuming you connect with the same worker name and password, it should work.

Great. I'll give that a shot once we upgrade to Jammy. Thanks.

@MikeHolman
Copy link
Collaborator

@jenkins-droid test this please

@NathanielMcVicar
Copy link
Collaborator

@gongsu832 it looks like the CMake version on the Jenkins builds is still below 3.20. Is there an additional step we should take to update these? Thanks!

@gongsu832
Copy link
Collaborator

gongsu832 commented May 12, 2023

It's being addressed by #2234

@NathanielMcVicar Windows CI appears to be stuck not reporting status. Is there any way to trigger it manually?

@NathanielMcVicar
Copy link
Collaborator

@NathanielMcVicar Windows CI appears to be stuck not reporting status. Is there any way to trigger it manually?

I see it reporting currently, can you point me to an example? Sorry, I missed this on Friday.

@AlexandreEichenberger
Copy link
Collaborator

@gongsu832 are the conditions that were holding us to a lower cmake minimum version still holding us at this time?

@NathanielMcVicar
Copy link
Collaborator

@gongsu832 @AlexandreEichenberger What is the current status of CMake in the docker image?

@AlexandreEichenberger
Copy link
Collaborator

I will defer this question for @gongsu832

@gongsu832
Copy link
Collaborator

Dev image is based on Ubuntu Jammy which comes with cmake 3.22.1 and user image is based on Redhat ubi8 which comes with cmake 3.20.2. So bumping up to 3.20.0 shouldn't be a problem.

@NathanielMcVicar NathanielMcVicar merged commit 8ea6d8f into onnx:main Sep 18, 2023
4 checks passed
@jenkins-droid
Copy link
Collaborator

Jenkins Linux ppc64le Build #11724 [push] Bump cmake minimum requi... started at 18:16

@jenkins-droid
Copy link
Collaborator

Jenkins Linux s390x Build #12731 [push] Bump cmake minimum requi... started at 18:06

@jenkins-droid
Copy link
Collaborator

Jenkins Linux amd64 Build #12708 [push] Bump cmake minimum requi... started at 17:06

@jenkins-droid
Copy link
Collaborator

Jenkins Linux amd64 Build #12708 [push] Bump cmake minimum requi... failed after 1 hr 11 min

@jenkins-droid
Copy link
Collaborator

Jenkins Linux s390x Build #12731 [push] Bump cmake minimum requi... passed after 1 hr 29 min

@jenkins-droid
Copy link
Collaborator

Jenkins Linux ppc64le Build #11724 [push] Bump cmake minimum requi... passed after 1 hr 48 min

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants