-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
GH-44950: [C++] Bump minimum CMake version to 3.25 #44989
base: main
Are you sure you want to change the base?
Conversation
|
a2f952c
to
81b112a
Compare
I am unsure on how to fix the remaining failures for R. Of course this is the initial CI (we also have to update all the extended CI jobs for crossbow). |
Updating the CMake lower bound will flip the default of several policies from legacy to new; sounds like you might be relying on legacy behaviour there in some way (once you figure out which policy is at fault, there's usually a migration path to keep the old behaviour) |
ok, it seems R forces the builds to use the CMake provided on the images:
@jonkeane @assignUser @amoeba will this be an issue for CRAN? Are we somehow forced to the CMake version on those images?
|
I went through the logs of our recent checks on cran and only one is using a version < 3.25 and that seems more incidental then purposely as it's the r-odrel arm64 but the intel version has 3.26 So I don't think we should be forced to use that cmake version, additionally we have a function that fetches current cmake if an unsuited version is found but apparently there is an issue with it as seen above. IIRC there was a change to numeric version in one of the las R Versions that is causing this? I'll have a look. |
It's possible the version comparison error is from this: https://github.com/apache/arrow/pull/44989/files#diff-935746c34b16289a07b0d9bf7642dbd268b18059b6187f7cdec7c464be47a3deL731-L743
The error case should probably |
Two other places in the R
|
It would probably be more forward-looking to avoid the error entirely. Why does the function fail parsing the CMake version? Can we add |
I could be remembering wrong, but I believe the function is used to check for cmake of a certain version, and this is to be robust to where cmake may not be installed or not found at the path provided. It does not emit an error, it traps it. This might not be where the error is coming from that was observed in CI, I was just browsing the source to see where you might get a numeric version error. Looking again, and reading the output it produced, I think we're hitting this: https://github.com/apache/arrow/pull/44989/files#diff-935746c34b16289a07b0d9bf7642dbd268b18059b6187f7cdec7c464be47a3deL718
should be (To be clear, we need to fix both this and the |
.env
Outdated
@@ -54,6 +54,7 @@ UBUNTU=22.04 | |||
|
|||
# Default versions for various dependencies | |||
CLANG_TOOLS=14 | |||
CMAKE=3.25.0 |
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.
Would it be possible (and also would it be advisable...) to call this CMAKE_VERSION
instead? One of the R failures I suspect might be due to us looking to CMAKE
for a path to the cmake executable itself at
Line 695 in bcb5bd3
Sys.getenv("CMAKE"), |
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.
Aaah I didn't see the previous comments on the PR (sorry, I must have had a tab open and didn't refresh!) but I suspect this here is the answer to:
It would probably be more forward-looking to avoid the error entirely. Why does the function fail parsing the CMake version?
…h to not require platform argument
…uire the existing version to be bumped
I've pushed the changes Neal suggested which should fix the ubuntu failure 🤞 (hope you don't mind, @raulcd !) |
I did a bit of digging on the windows front, and I suspect what's going on is the the MSYS2/mingw cmake is being preferred (I'm not familiar enough with that ecosystem to know if we can override it with something on the system, but that also seems fragile itself). https://github.com/raulcd/arrow/blob/e5d521134db4bed8507572fece3b56eb4a1b9158/ci/scripts/r_windows_build.sh#L26-L30 has some info about how to test newer dependencies, I wonder if we (conditionally) use those pacman commands to insall a newer cmake than what's in the CRAN repo (https://cloud.r-project.org/bin/windows/Rtools/4.0/ucrt64/ has the version I'm seeing installed listed, so if we override that one in our builds that might be sufficient. |
Unfortunately, that won't help us. https://github.com/r-windows/rtools-packages/ is archived; CRAN has moved away from the toolchain that Jeroen was maintaining. We should delete that comment. For newer cmake with older rtools, maybe we can get it from https://github.com/Kitware/CMake/releases/download/v3.31.2/cmake-3.31.2-windows-x86_64.zip or something? Or maybe we can install newer rtools for cmake but use the older rtools for compilers etc.? |
I don't understand why the job is installing
edit: Probably naive question here, I suppose we can't update the mirrors here to point to msys2 and download the newer cmake from there before building Arrow as we would have the same issue when we try to publish to CRAN, right? @jonkeane @nealrichardson |
Honestly I don't know. But it's not relevant for CRAN because (a) CRAN currently only builds on R 4.3 and 4.4, both of which have new enough cmake, and (b) we don't require cmake on CRAN anyway because we build the libarrow C++ library in our CI and download it in the CRAN build--CRAN only compiles the R bindings, which do not require cmake. Solving the cmake issue for R < 4.3 is only for our own purposes of building C++ libraries that are compatible with older versions of R on Windows. We support more versions of R than CRAN actively checks on, though it appears that in CI, we only check on the Windows current release version, and when we test old R versions, we do it on linux. IIUC we're building libarrow with the rtools4.0 toolchain for maximum compatibility. If we want to continue doing that, we could try installing newer cmake somewhere before https://github.com/apache/arrow/blob/main/.github/workflows/r.yml#L293 and making sure it's on the PATH. Or, to bump up, we would change the rtools version in that job from |
Do we still want to do that? How is our R support policy decided? |
We generally follow the tidyverse version support policy, which is the most recent 5 versions. Many enterprise users of R don't upgrade versions eagerly so they can be stuck on older versions longer than CRAN's testing window. If there is an easy way to get new enough cmake into the R |
One more reference to 3.16 to update: https://github.com/apache/arrow/blob/main/r/DESCRIPTION#L31 |
…cman before setting up RTools
After a bunch of trial and error, I seem to be stuck with the following issue if I override the CMake version to be used to either a manually
I am trying some things, like using an updated |
The aws-sdk-cpp it's using is coming from https://github.com/r-windows/rtools-packages/tree/master/mingw-w64-aws-sdk-cpp If you need to update it, I think you'll have to drop it from the dependencies that get downloaded in our PKGBUILD and make it dependency source=BUNDLED. Doesn't look like we're able to make any updates happen to that repo. cc @assignUser, in case you wanted to start pushing on the MXE direction again as an alternative 🙃 |
This is currently under development. I am only exercising CI to find out what things would require to be updated
This is still being decided and a decision hasn't been finalized yet.
Rationale for this change
We want to upgrade our CMake version to 3.25 as discussed on the ML:
https://lists.apache.org/thread/h8jp16ktrj11fmjmjhlg6xvkvv9wzvjk
What changes are included in this PR?
TBD
Are these changes tested?
Yes, via CI.
Are there any user-facing changes?
Yes, the minimum CMake version to be used to build Arrow is bumped to 3.25.
This PR includes breaking changes to build systems.