Conversation
WalkthroughModified Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
libraries.cmake/eigen.cmake (2)
28-32: Null-input stdin redirection is a good fix; minor comment/doc nitCreating
null_input.txtand wiring it viaINPUT_FILEis a clean way to avoid inherited stdin hangs, especially on Windows. The only nit is that the comment says “Use CMAKE_COMMAND to create an empty file”, while you’re using CMake’sfile(WRITE ...)instead; consider rewording the comment so future readers don’t look for a missing-Einvocation.
67-72: Build step now has no timeout; confirm intentional removalPreviously the build/install
execute_processhad a timeout; now it can run indefinitely if the build hangs. That may be exactly what you want (avoid spurious timeouts on slow Windows builders), but it does change failure mode from “timeout” to “potentially stuck job”. Consider either confirming that this is acceptable in your CI, or reintroducing a more generous timeout if you still want protection against hard hangs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libraries.cmake/eigen.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). (4)
- GitHub Check: build (ubuntu-24.04-arm)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (windows-latest)
- GitHub Check: build (macos-latest)
🔇 Additional comments (1)
libraries.cmake/eigen.cmake (1)
39-46: Disabling Fortran and extending configure timeout: confirm this is acceptable for all targetsSetting
CMAKE_Fortran_COMPILER=NOTFOUNDwill suppress Fortran detection globally for this Eigen configure step, even on platforms where a Fortran toolchain exists. Given that this contrib build only installs headers and already disables testing, that’s probably fine, but worth confirming that no downstream expects Eigen’s CMake to see a Fortran compiler. The increased configureTIMEOUT 600also seems reasonable for slower Windows setups.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.