More arrow bullshit

[poshul]

Summary by CodeRabbit

• Chores
  • Updated MSVC build installation directory configuration for Arrow and Parquet libraries.
  • Simplified CMake build configuration by removing legacy relocatability compatibility layer.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

The changes simplify Arrow CMake configuration by removing the ${PROJECT_BINARY_DIR} prefix from MSVC install directory settings and eliminating a relocatability rewriting mechanism that previously modified ArrowTargets and ParquetTargets CMake files.

Changes

Cohort / File(s)	Summary
Arrow CMake configuration simplification libraries.cmake/arrow.cmake	Modified MSVC install paths: CMAKE_INSTALL_BINDIR and CMAKE_INSTALL_LIBDIR changed from ${PROJECT_BINARY_DIR}/lib to lib. Removed a large relocatability fix block that rewrote ArrowTargets\.cmake and ParquetTargets\.cmake files to replace hardcoded _IMPORT_PREFIX and absolute paths with relocatable prefixes.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20–25 minutes

• Key areas requiring attention: Verify that removal of the relocatability rewriting mechanism does not break Arrow/Parquet CMake imports in non-standard build or deployment scenarios, particularly across different platforms or installation configurations.

Possibly related PRs

• try again #166: Modifies Arrow/Parquet relocatability handling in arrow.cmake with path normalization and rewriting logic—directly related to the relocatability fix removed in this PR.
• More arrow bullshit #165: Updates Arrow/Parquet CMake config handling including relocatability fixes and MSVC static library settings—overlaps with the configuration changes made here.

Poem

🐰 Paths simplified, no more PREFIX fuss,
The arrow flies straight without detours for us.
From ${BINARY_DIR}/lib to lib so clean,
A CMake refactoring, subtly keen!

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Title check	❓ Inconclusive	The title 'More arrow bullshit' is vague and unprofessional, using informal language that doesn't clearly describe the specific changes made to the arrow build configuration.	Replace with a descriptive title like 'Fix Arrow CMake installation paths and remove relocatability workaround' to clearly communicate the changes.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch more_arrow_bullshit

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between dc7f54a and c8eef69.

📒 Files selected for processing (1)

• libraries.cmake/arrow.cmake (1 hunks)

🔇 Additional comments (2)

libraries.cmake/arrow.cmake (2)

24-29: Simplified install directory configuration looks correct.

Using relative paths (lib) for CMAKE_INSTALL_BINDIR and CMAKE_INSTALL_LIBDIR is appropriate since they will be resolved relative to CMAKE_INSTALL_PREFIX (set to ${PROJECT_BINARY_DIR} on line 27).

Note: The Linux/MacOS section (line 132) still uses the absolute path ${PROJECT_BINARY_DIR}/lib for CMAKE_INSTALL_LIBDIR. Consider aligning the two branches for consistency, though this isn't strictly necessary.

---

17-101: Verify MSVC relocatability without the post-build fix.

The relocatability fix (rewriting ArrowTargets*.cmake and ParquetTargets*.cmake files) was removed from the MSVC section but retained for Linux/MacOS (lines 178-217).

Please confirm that the simplified CMAKE_INSTALL_BINDIR/CMAKE_INSTALL_LIBDIR changes are sufficient to produce relocatable CMake configs on MSVC, or if Arrow still generates hardcoded absolute paths in the target files that would break when the package is moved.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[poshul]

@jpfeuffer with some more research it looks like the issue was that I was passing absolute paths. This works, and is way less brittle