-
Notifications
You must be signed in to change notification settings - Fork 373
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
Improve rerun_cpp readme & CMakeLists.txt #4126
Conversation
uh. needs windows fixing. Why am I not surprised |
da4b137
to
e8176e4
Compare
There was more wrong than just windows build. Had to kill the path of being in the repo and still setting |
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.
Overall looks great. Splitting out the arrow stuff makes the CMakeLists.txt much more approachable.
# Hack to propagate INTERFACE_INCLUDE_DIRECTORIES. | ||
# via https://stackoverflow.com/a/47358004 | ||
file(MAKE_DIRECTORY ${ARROW_DOWNLOAD_PATH}/include) | ||
include(download_and_build_arrow.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.
Does this expose the variables necessary to implement https://github.com/kyle-figure/bazel-minimal-rerun/blob/main/third_party/rerun/patches/cmake.patch ?
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.
tbh I'm deeply confused (and troubled) about the visibility scope of cmake variables, so I don't know. Worst case this requires more patches then
What
Checklist