-
Notifications
You must be signed in to change notification settings - Fork 13
Cmake version and CI python version upgrades #178
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
Conversation
ProfFan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some nitpicks
CMakeLists.txt
Outdated
| @@ -1,4 +1,4 @@ | |||
| cmake_minimum_required(VERSION 3.9) | |||
| cmake_minimum_required(VERSION 3.30) | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3.30 is really really new
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cmake is currently on version 4. Plus I figured this is more for our internal use anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we use whatever we currently use in GTSAM? Same goes for CI runners, btw. Otherwise we could get in a situation where we use something here that is not yet supported in GTSAM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, CMake will default to use the cmake config provided by upstream package even with FindBoost, see https://cmake.org/cmake/help/v3.10/module/FindBoost.html#boost-cmake
So we don't need to bump the minimum requirement, just need to have a test for the existence of the CMP rule and set it to NEW if exists to silence the warning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've made that change in GTSAM and was going to push it along with some other minor fixes.
(CMake had been yelling at me a lot recently)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Main project is at 3.9 right now https://github.com/borglab/gtsam/blob/develop/CMakeLists.txt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Asking for too new a version imposes a burden on our users. What is the default version installed for Ubuntu 22 and 24? What is the version in brew and whatever windows uses (Unix subsystem uses Ubuntu). That is what should drive this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Main project is at 3.9 right now borglab/gtsam@
develop/CMakeLists.txt
I made that change locally, I haven't pushed it yet since I am trying to verify dependencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have the bandwidth to deal with this (IMO trivial) issue, so I dropped the version down to 3.19.2 since that version is when CMake started supporting Apple Silicon.
(I say trivial since I already spent the time compiling and testing compatibility already before making the PR, as a good contributor would)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To approve I would need a rationale for minimum cmake version that aligns with GTSAM along the lines of my comment.
The main reasons here are:
Regarding supported versions:
|
|
Also CMake 3.10 is deprecated so I am not a fan of using 3.9 either. |
|
I thought we included wrap as a subtree inside GTSAM? So, whatever version we impose, should we not synchronize? I don't know why this is a trivial issue. Maybe you and I @varunagrawal should chat about it rather than trying to resolve via comments etc - I might not see the full picture here. |
It's not necessary to synchronize CMake versions since the only thing CMake is doing is pulling in some variables from GTSAM which are version agnostic. The main compatibility comes with binary level interfacing which is what pybind11 is responsible for. Also, like I mentioned above, I've already tested the whole pipeline of wrap, compile and testing of the Python wrapper within GTSAM using all these changes. I made the PR after verifying everything works well. |
Uh oh!
There was an error while loading. Please reload this page.