Skip to content
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

Update & improve build instructions for auto-bundled arrow-cpp, and get windows to work out of the box #8

Merged
merged 6 commits into from
Oct 30, 2023

Conversation

Wumpf
Copy link
Member

@Wumpf Wumpf commented Oct 27, 2023

Wumpf added a commit to rerun-io/rerun that referenced this pull request Oct 29, 2023
…run_sdk (#4051)

### What

* Fixes  #4028

Confirmed to be working on Linux, Mac and Windows. On windows also made
sure this works with debug runtime and experimented a bit with Visual
Studio setups. Also checked Windows against the external sample, see
rerun-io/cpp-example-opencv-eigen#8

Made improvements to our CMakeLists files overall. Notably we no longer
need `DOWNLOAD_EXTRACT_TIMESTAMP ON` to suppress warnings - idea from
rerun-io/cpp-example-opencv-eigen#6, setting a
range for the cmake version fixes it


Drawbacks:
* LOTS of cmake log spam during build
* a bit longer builds on the first run, seems to be a bit more
pronounced on Windows (no hard numbers available yet)

Advantages:
* no longer needed to install libarrow
* works on Windows debug builds (note that other libraries like OpenCV
might still have this issue)
* stable against all c++ library/compiler changes, no fear of weird
runtime errors because of stdlib mismatches
* more lightweight artifacts - on Windows one would have needed to ship
a large number of dlls, now none

It's still worth considering if we should ship prebuilt debug & release
arrow dlls instead.

TODO: We need to document this new behavior and how to turn it off in
the respective doc pages. By default the user no longer needs to install
libArrow with this.

Important CMake options that need documenting (can partially copy& paste
this!):
* `RERUN_ARROW_LINK_SHARED`
* exited before but now it's default OFF (EDIT:) on windows. This makes
it easier to relocate windows executable (don't need to copy Arrow.dll
around!). EDIT: On mac this made the address sanitizer crazy for me and
I reckon it's an unnecessary slowdown
* `RERUN_ARROW_EXTERNAL_PROJECT` (new!)
    * Default on
* If enabled we download Arrow 10.0.1 and build it in a as small as
possible fashion
    * If disabled we run `find_package` == old behavior
* `RERUN_C_LIB`
   * where to find the static c library that rerun_sdk links against
   * in repo defaults to cargo built
   * outside repo defaults to /lib/ folder + platform artifact name

### Checklist
* [x] I have read and agree to [Contributor
Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and
the [Code of
Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md)
* [x] I've included a screenshot or gif (if applicable)
* [x] I have tested [demo.rerun.io](https://demo.rerun.io/pr/{{
pr.number }}) (if applicable)
* [x] The PR title and labels are set such as to maximize their
usefulness for the next release's CHANGELOG

- [PR Build Summary](https://build.rerun.io/pr/{{ pr.number }})
- [Docs preview](https://rerun.io/preview/{{ pr.commit }}/docs)
<!--DOCS-PREVIEW-->
- [Examples preview](https://rerun.io/preview/{{ pr.commit }}/examples)
<!--EXAMPLES-PREVIEW-->
- [Recent benchmark results](https://ref.rerun.io/dev/bench/)
- [Wasm size tracking](https://ref.rerun.io/dev/sizes/)

---------

Co-authored-by: Clement Rey <cr.rey.clement@gmail.com>
confirmed working nicely on windows
@emilk emilk mentioned this pull request Oct 30, 2023
33 tasks
CMakeLists.txt Outdated Show resolved Hide resolved
@Wumpf Wumpf marked this pull request as ready for review October 30, 2023 09:05
@Wumpf Wumpf merged commit 84271e6 into main Oct 30, 2023
5 checks passed
@Wumpf Wumpf deleted the andreas/update-with-embedded-arrow branch October 30, 2023 12:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants