GH-51138: [Python] Fix ccache efficiency - #51139
Conversation
|
|
9a5a1b7 to
a66cc75
Compare
|
@github-actions crossbow submit -g python |
|
Revision: a66cc75 Submitted crossbow builds: ursacomputing/crossbow @ actions-290df36268
|
|
@github-actions crossbow submit test-ubuntu-22.04-cpp -p image=ubuntu-python |
|
Revision: a66cc75 Submitted crossbow builds: ursacomputing/crossbow @ actions-ba899c1522
|
What's this @rok ? |
| # 2. Set ccache base_dir to the build output directory as it is typically | ||
| # a temporary directory, and would otherwise fail caching because of | ||
| # using different paths everytime. | ||
| set(ccache_command ${CCACHE_FOUND} keep_comments_cpp=true base_dir=${CMAKE_BINARY_DIR}) |
There was a problem hiding this comment.
As per my 🤖 ccache pre-4.8 does not accept compiler options (keep_comments_cpp=true in this case). And testing on an older ccache I get:
> ccache keep_comments_cpp=true base_dir=/tmp gcc --version
ccache: error: Could not find compiler "keep_comments_cpp=true" in PATHSo users with old ccache building pyarrow would have this crash.
There was a problem hiding this comment.
Hmm, are there any Python builds which use an old ccache?
There was a problem hiding this comment.
I think no, but users might. I think this is obscure enough to ignore.
There was a problem hiding this comment.
If there was an easy way to query the ccache version we could work around this to ensure a better UX. But I'm not sure how to do that. @kou ?
There was a problem hiding this comment.
(for the record, ccache 4.8 was released in March 2023)
There was a problem hiding this comment.
My 🤖 suggest two options, preferring the first one. I would defer to @kou
diff --git a/python/CMakeLists.txt b/python/CMakeLists.txt
--- a/python/CMakeLists.txt
+++ b/python/CMakeLists.txt
@@ -136,7 +136,13 @@ if(CCACHE_FOUND
# 2. Set ccache base_dir to the build output directory as it is typically
# a temporary directory, and would otherwise fail caching because of
# using different paths everytime.
- set(ccache_command ${CCACHE_FOUND} keep_comments_cpp=true base_dir=${CMAKE_BINARY_DIR})
+ set(ccache_command
+ ${CMAKE_COMMAND}
+ -E
+ env
+ "CCACHE_COMMENTS=1"
+ "CCACHE_BASEDIR=${CMAKE_BINARY_DIR}"
+ ${CCACHE_FOUND})
set(CMAKE_C_COMPILER_LAUNCHER ${ccache_command})
set(CMAKE_CXX_COMPILER_LAUNCHER ${ccache_command})
endif() or
diff --git a/python/CMakeLists.txt b/python/CMakeLists.txt
--- a/python/CMakeLists.txt
+++ b/python/CMakeLists.txt
@@ -130,6 +130,17 @@ if(CCACHE_FOUND
AND NOT CMAKE_CXX_COMPILER_LAUNCHER)
message(STATUS "Using ccache: ${CCACHE_FOUND}")
+ execute_process(
+ COMMAND ${CCACHE_FOUND} --version
+ OUTPUT_VARIABLE ccache_version_output
+ OUTPUT_STRIP_TRAILING_WHITESPACE)
+ set(ccache_version "0")
+ if(ccache_version_output
+ MATCHES "ccache version ([0-9]+\\.[0-9]+(\\.[0-9]+)?)")
+ set(ccache_version "${CMAKE_MATCH_1}")
+ endif()
+
# 1. Let ccache preserve C++ comments, because some of them may be
# meaningful to the compiler (ARROW-3985)
# 2. Set ccache base_dir to the build output directory as it is typically
@@ -137,7 +148,12 @@ if(CCACHE_FOUND
# a temporary directory, and would otherwise fail caching because of
# using different paths everytime.
- set(ccache_command ${CCACHE_FOUND} keep_comments_cpp=true base_dir=${CMAKE_BINARY_DIR})
+ if(ccache_version VERSION_GREATER_EQUAL 4.8)
+ set(ccache_command ${CCACHE_FOUND} keep_comments_cpp=true
+ base_dir=${CMAKE_BINARY_DIR})
+ else()
+ message(STATUS "ccache ${ccache_version} does not support command-line configuration")
+ set(ccache_command ${CCACHE_FOUND})
+ endif()
set(CMAKE_C_COMPILER_LAUNCHER ${ccache_command})
set(CMAKE_CXX_COMPILER_LAUNCHER ${ccache_command})
endif() There was a problem hiding this comment.
And I still think we don't need this.
There was a problem hiding this comment.
Ah, the env version is neat, but would it work on Windows?
There was a problem hiding this comment.
As per 🤖:
cmake -E env is cross-platform and sets variables through CMake rather than Unix shell syntax, so it works on Windows with Ninja
github-actions
Bot
added
awaiting changes
|
@github-actions crossbow submit wheelcp311* |
|
Revision: a66cc75 Submitted crossbow builds: ursacomputing/crossbow @ actions-131e0bf55a
|
github-actions
Bot
added
awaiting merge
2 participants
Rationale for this change
Our current ccache support for PyArrow currently has two issues:
What changes are included in this PR?
base_dirto the temporary build directory so that ccache strips away the build directory and hits previously cached results obtained from a different build directory.Are these changes tested?
Yes, locally I confirmed that ccache now efficiently reuses compilation outputs for Cython-generated C++ sources.
Before:
After:
Are there any user-facing changes?
No.