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

Fix after test on Windows #6

Merged
merged 2 commits into from
Oct 27, 2023
Merged

Fix after test on Windows #6

merged 2 commits into from
Oct 27, 2023

Conversation

traversaro
Copy link
Contributor

I tested the example, on Windows, and I experienced the following problems.

Link failure due to use of Debug CMAKE_BUILD_TYPE

At the end of pixi run run the linking was failing with:

[151/151] Linking CXX executable rerun_ext_example.exe
FAILED: rerun_ext_example.exe
cmd.exe /C "cd . && C:\src\cpp-example-opencv-eigen\.pixi\env\Library\bin\cmake.exe -E vs_link_exe --intdir=CMakeFiles\rerun_ext_example.dir --rc=C:\PROGRA~2\WI3CF2~1\10\bin\100190~1.0\x64\rc.exe --mt=C:\PROGRA~2\WI3CF2~1\10\bin\100190~1.0\x64\mt.exe --manifests  -- C:\PROGRA~2\MICROS~3\2019\BUILDT~1\VC\Tools\MSVC\1429~1.301\bin\Hostx64\x64\link.exe /nologo @CMakeFiles\rerun_ext_example.rsp  /out:rerun_ext_example.exe /implib:rerun_ext_example.lib /pdb:rerun_ext_example.pdb /version:0.0 /machine:x64 /debug /INCREMENTAL /subsystem:console  && cd ."
LINK Pass 1: command "C:\PROGRA~2\MICROS~3\2019\BUILDT~1\VC\Tools\MSVC\1429~1.301\bin\Hostx64\x64\link.exe /nologo @CMakeFiles\rerun_ext_example.rsp /out:rerun_ext_example.exe /implib:rerun_ext_example.lib /pdb:rerun_ext_example.pdb /version:0.0 /machine:x64 /debug /INCREMENTAL /subsystem:console /MANIFEST /MANIFESTFILE:CMakeFiles\rerun_ext_example.dir/intermediate.manifest CMakeFiles\rerun_ext_example.dir/manifest.res" failed (exit code 1120) with the following output:
main.cpp.obj : error LNK2019: unresolved external symbol "void __cdecl cv::cvtColor(class cv::debug_build_guard::_InputArray const &,class cv::debug_build_guard::_OutputArray const &,int,int)" (?cvtColor@cv@@YAXAEBV_InputArray@debug_build_guard@1@AEBV_OutputArray@31@HH@Z) referenced in function main
rerun_ext_example.exe : fatal error LNK1120: 1 unresolved externals
ninja: build stopped: subcommand failed.

The reason for that is (I believe) that in general on Windows/MSVC the ABI of Release builds is not compatible with the Debug builds. conda-forge (and so pixi) only contains Release builds, so in general it is not possible to build C++ projects in Debug mode on Windows/MSVC. I think a good compromise is to use RelWithDebInfo instead of Debug, this is done in 6353d7e .

CMake warning due to unset policy

During CMake configuration, I get this warning:

-- Detecting CXX compile features - done
CMake Warning (dev) at .pixi/env/Library/share/cmake-3.27/Modules/FetchContent.cmake:1316 (message):
  The DOWNLOAD_EXTRACT_TIMESTAMP option was not given and policy CMP0135 is
  not set.  The policy's OLD behavior will be used.  When using a URL
  download, the timestamps of extracted files should preferably be that of
  the time of extraction, otherwise code that depends on the extracted
  contents might not be rebuilt if the URL changes.  The OLD behavior
  preserves the timestamps from the archive instead, but this is usually not
  what you want.  Update your project to the NEW behavior or specify the
  DOWNLOAD_EXTRACT_TIMESTAMP option with a value of true to avoid this
  robustness issue.
Call Stack (most recent call first):
  CMakeLists.txt:12 (FetchContent_Declare)
This warning is for project developers.  Use -Wno-dev to suppress it.

-- Arrow version: 13.0.0

A simple way to avoid this warning is just to set policy_max argument of cmake_minimum_required to 3.27, as done in 63bf6c6 .

Copy link
Member

@emilk emilk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for submitting this PR! Looks good to me

@emilk emilk merged commit 0977050 into rerun-io:main Oct 27, 2023
4 checks passed
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 <[email protected]>
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