try again #166
try again #166
Conversation
WalkthroughExtends relocatability fixes in Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
libraries.cmake/arrow.cmake (4)
92-107: Relocatability approach looks good but relies on an exact_IMPORT_PREFIXline matchThe strategy of replacing Arrow’s hardcoded
_IMPORT_PREFIXwith:get_filename_component(_IMPORT_PREFIX "${CMAKE_CURRENT_LIST_DIR}/../../.." ABSOLUTE)is solid and should make Arrow/Parquet configs relocatable on both Windows and Linux/macOS.
However, the
ARROW_ORIGINAL_PATTERNyou search/replace is the fullset(_IMPORT_PREFIX "...")line:
- Windows:
set(_IMPORT_PREFIX "${PROJECT_BINARY_DIR_NORMALIZED}")- Linux/macOS:
set(_IMPORT_PREFIX "${PROJECT_BINARY_DIR}")This makes the fix quite sensitive to small upstream formatting changes in Arrow’s generated
*Targets.cmake(extra spaces, different quoting, added options, trailing slash, etc.). Any such change will causestring(FIND)to fail and the replacement to silently do nothing (beyond the warning and preview).If you want this to be more robust across Arrow versions, consider matching only the path portion (or using a
string(REGEX REPLACE ...)that looks forset(_IMPORT_PREFIX ".*")and replaces the whole command), while still producing the sameget_filename_componentcall.Also applies to: 221-234
99-104: Consider normalizingPROJECT_BINARY_DIRconsistently across platformsOn Windows you normalize:
file(TO_CMAKE_PATH "${PROJECT_BINARY_DIR}" PROJECT_BINARY_DIR_NORMALIZED) set(ARROW_ORIGINAL_PATTERN "set(_IMPORT_PREFIX \"${PROJECT_BINARY_DIR_NORMALIZED}\")")but on Linux/macOS you use the raw
PROJECT_BINARY_DIRinARROW_ORIGINAL_PATTERN.Since CMake usually renders paths with forward slashes on all platforms, this asymmetry is probably harmless, but for long‑term maintainability it might be cleaner to use the same normalization step (and pattern construction) on all platforms so the matching behavior is identical.
Also applies to: 228-230
110-127: Verify that the narrower globbing still catches all relevant Arrow/Parquet targetsYou now only patch:
file(GLOB ARROW_TARGET_FILES "${ARROW_CMAKE_DIR}/ArrowTargets.cmake") file(GLOB PARQUET_TARGET_FILES "${PARQUET_CMAKE_DIR}/ParquetTargets.cmake")instead of
ArrowTargets*.cmake/ParquetTargets*.cmake(per the summary). That’s nice for predictability, but it assumes that the only_IMPORT_PREFIXassignments that matter live in those exact files and not in configuration-specific ones (e.g.ArrowTargets-debug.cmake).Given Arrow’s current layout that’s likely true, but it would be good to double‑check the generated CMake package to ensure:
ArrowTargets.cmake/ParquetTargets.cmakeare the authoritative places_IMPORT_PREFIXis set.- There aren’t additional
set(_IMPORT_PREFIX ...)occurrences in other*Targets*.cmakefiles that also need the same treatment.If those assumptions hold, this change is fine and helps avoid over‑touching unrelated files.
Also applies to: 130-141, 236-253, 256-267
115-123: CMake version dependency forstring(SUBSTRING ...)with fixed length 800The debug preview:
string(SUBSTRING "${TARGET_CONTENT}" 0 800 TARGET_PREVIEW)is helpful. On CMake ≥3.2 this is safe even when the file is shorter than 800 characters (the substring is just truncated). On CMake 3.1 and below, though, an over‑long
lengthis an error.If the project’s minimum required CMake version is already ≥3.2, this is fine as‑is. If you still support very old CMake versions, you may want to guard this or compute
min(800, actual_length)first.Also applies to: 239-249
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libraries.cmake/arrow.cmake(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (ubuntu-24.04-arm)
- GitHub Check: build (windows-latest)
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-24.04-arm)
- GitHub Check: build (ubuntu-latest)
🔇 Additional comments (1)
libraries.cmake/arrow.cmake (1)
92-144: Nice diagnostic logging and symmetry between Windows and Linux/macOSThe added status/warning messages and the consistent application of the relocatability fix across both MSVC and non‑MSVC branches make this much easier to debug if Arrow’s generated configs ever change. The final:
message(STATUS "Fixed Arrow/Parquet CMake configs for relocatability")is also helpful as a clear indicator that the patch step ran.
No functional issues here from my side.
Also applies to: 221-270
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
libraries.cmake/arrow.cmake (3)
109-122: Consider adding validation for GLOB results.If the Arrow build or installation fails partially,
file(GLOB ...)may return an empty list, and the loop will silently do nothing. Consider adding a check to warn or fail if no matching files are found.file(GLOB ARROW_TARGET_FILES "${ARROW_CMAKE_DIR}/ArrowTargets*.cmake") +list(LENGTH ARROW_TARGET_FILES ARROW_TARGET_FILES_COUNT) +if(ARROW_TARGET_FILES_COUNT EQUAL 0) + message(WARNING "No ArrowTargets*.cmake files found in ${ARROW_CMAKE_DIR}") +endif() foreach(TARGET_FILE ${ARROW_TARGET_FILES})
218-220: Inconsistency: Missing path normalization on Linux/macOS.The Windows block uses
file(TO_CMAKE_PATH ...)at line 99 for path normalization, but this block uses${PROJECT_BINARY_DIR}directly. While Linux/macOS paths typically already use forward slashes, adding normalization would ensure consistency and robustness.+# Normalize path separators for replacement (for consistency) +file(TO_CMAKE_PATH "${PROJECT_BINARY_DIR}" PROJECT_BINARY_DIR_NORMALIZED) + # Fix 1: Replace hardcoded _IMPORT_PREFIX in ArrowTargets.cmake set(ARROW_RELOCATABLE_REPLACEMENT "get_filename_component(_IMPORT_PREFIX \"\${CMAKE_CURRENT_LIST_DIR}/../../..\" ABSOLUTE)") -set(ARROW_ORIGINAL_PATTERN "set(_IMPORT_PREFIX \"${PROJECT_BINARY_DIR}\")") +set(ARROW_ORIGINAL_PATTERN "set(_IMPORT_PREFIX \"${PROJECT_BINARY_DIR_NORMALIZED}\")")Then update line 235 to use
PROJECT_BINARY_DIR_NORMALIZEDas well.
92-134: Consider extracting relocatability fix into a helper macro.The Windows and Linux/macOS blocks contain nearly identical relocatability fix logic (pattern definition, GLOB, foreach, string replacement). This duplication could be reduced by extracting a helper macro, improving maintainability.
Example approach:
# Define helper macro before the platform-specific blocks macro(FIX_CMAKE_RELOCATABILITY CMAKE_DIR FILE_PATTERN BUILD_DIR) file(TO_CMAKE_PATH "${BUILD_DIR}" _NORMALIZED_DIR) set(_REPLACEMENT "get_filename_component(_IMPORT_PREFIX \"\${CMAKE_CURRENT_LIST_DIR}/../../..\" ABSOLUTE)") set(_ORIGINAL "set(_IMPORT_PREFIX \"${_NORMALIZED_DIR}\")") file(GLOB _TARGET_FILES "${CMAKE_DIR}/${FILE_PATTERN}") foreach(_TARGET_FILE ${_TARGET_FILES}) file(READ "${_TARGET_FILE}" _CONTENT) string(REPLACE "${_ORIGINAL}" "${_REPLACEMENT}" _CONTENT "${_CONTENT}") string(REPLACE "\"${_NORMALIZED_DIR}/" "\"\${_IMPORT_PREFIX}/" _CONTENT "${_CONTENT}") file(WRITE "${_TARGET_FILE}" "${_CONTENT}") message(STATUS " Fixed: ${_TARGET_FILE}") endforeach() endmacro()Then call it for each directory:
FIX_CMAKE_RELOCATABILITY("${ARROW_CMAKE_DIR}" "ArrowTargets*.cmake" "${PROJECT_BINARY_DIR}") FIX_CMAKE_RELOCATABILITY("${PARQUET_CMAKE_DIR}" "ParquetTargets*.cmake" "${PROJECT_BINARY_DIR}")Also applies to: 212-252
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libraries.cmake/arrow.cmake(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-24.04-arm)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (macos-latest)
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-24.04-arm)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (macos-latest)
🔇 Additional comments (3)
libraries.cmake/arrow.cmake (3)
98-99: Good: Path normalization for Windows.Using
file(TO_CMAKE_PATH ...)ensures consistent forward slashes in the replacement pattern, which is important for matching Arrow's generated CMake files.
124-132: LGTM - Consistent relocation pattern for Parquet.The Parquet files are processed using the same replacement strategy, which is correct since they share the same
_IMPORT_PREFIXmechanism.
101-103: Approve: Well-structured relocation strategy.The approach of replacing the hardcoded
set(_IMPORT_PREFIX "...")withget_filename_component(_IMPORT_PREFIX "${CMAKE_CURRENT_LIST_DIR}/../../.." ABSOLUTE)is a standard and effective way to make CMake configs relocatable. Combined with replacing absolute paths inIMPORTED_LOCATIONproperties, this should correctly handle package relocation.
|
Arrow's build system helpfully produces cmake files to tell any dependent projects where the generated libraries are, but it unhelpfully uses absolute paths, which makes the whole build fold non-relocatable. This is fine in the linux builds since it doesn't get relocated, but for windows it means the build system looks for the arrow libraries in a path that doesn't exist. |
|
Please report this to arrow devs, as this workaround feels very unstable. There are definitely ways to make this relocatable if they do it correctly! |
|
Maybe even patch the install/export parts here instead of post-replacing paths if you can find the error quickly. |
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.