search

luxonis / depthai-core

DepthAI C++ Library
MIT License
235 stars 127 forks source link

preprocessor compliance flag cascades and breaks apps in Windows MSVC static builds #317

Open diablodale opened 2 years ago

diablodale commented 2 years ago

Main app builds are breaking. I suspect due to preprocessor flags that depthai-core is introducing. Commit https://github.com/luxonis/depthai-core/commit/f17b4b3286b42d0eca7f650f78bc6e30d0fe647b introduced preprocessor conformance flags for MSVC compiler and breaks the main app using static builds of depthai-core.

Setup

Repro

  1. config and build for x64, static library, debug
  2. cmake install
  3. point your main app to the install location
  4. cmake config and build your main app

Result

Many errors in main app, 3rd party libraries, and the Windows SDK itself. For example...

[build] C:\Program Files (x86)\Windows Kits\10\include\10.0.19041.0\um\winbase.h(9531): error C2220: the following warning is treated as an error
[build] C:\Program Files (x86)\Windows Kits\10\include\10.0.19041.0\um\winbase.h(9531): warning C5105: macro expansion producing 'defined' has undefined behavior
[build] C:\Program Files (x86)\Windows Kits\10\include\10.0.19041.0\um\winbase.h(9531): note: to simplify migration, consider the temporary use of /Wv:18 flag with the version of the compiler with which you used to build without warnings

[build] **redacted**: warning C5103: pasting 'oak' and '/' does not result in a valid preprocessing token
[build] **redacted**: note: to simplify migration, consider the temporary use of /Wv:18 flag with the version of the compiler with which you used to build without warnings
[build] **redacted**: note: in expansion of macro 'CAT'
[build] **redacted**: note: in expansion of macro 'CAT2'
[build] **redacted**: note: in expansion of macro 'INCLUDE_FILE'
[build] **redacted**: note: in expansion of macro 'STRINGIFY'
...

The cmake install of step 2 above, has a file lib/cmake/depthai/depthaiTargets.cmake that has a line I suspect is the cause...

  INTERFACE_COMPILE_OPTIONS "/Zc:preprocessor"

My main apps's build/compile_commands.json, I can see all my app's compile units have an unwanted flag /Zc:preprocessor added to the arguments. It should not be there.

{
  "directory": "**redacted**",
  "command": "C:\\PROGRA~2\\MICROS~2\\2019\\COMMUN~1\\VC\\Tools\\MSVC\\1429~1.301\\bin\\Hostx64\\x64\\cl.exe   **redacted** /Zc:preprocessor /FoCMakeFiles\\redacted.obj /FdTARGET_COMPILE_PDB /FS -c C:\\redacted.cpp",
  "file": "C:\\redacted.cpp"
},

Expected

No new errors about preprocessor. No /Zc:preprocessor flags on my main app's compile units.

Notes

Strongly suspect this is related to commit https://github.com/luxonis/depthai-core/commit/f17b4b3286b42d0eca7f650f78bc6e30d0fe647b

When building depthai-core for static libraries, this setting is propogating to the cmake install of depthai-core, and that propogates the flag to the main app using depthai-core. This causes unmanagable cascade of errors in the main app and in the Windows SDK.

themarpe commented 2 years ago

@diablodale

Short overview: This flag is required to force MSVC to use conforming preprocessor, needed for libnops macro, which MSVC couldn't parse properly otherwise.

There are a couple of options I see about mitigating this:

With first option, we'd have to move the serialization macros outside of structures and move them to separate files. Fortunately consumers of the library don't actually require those so could be hidden into private translation unit. Still, we have to expose those for users of depthai-shared (embedded case).

Second option is a tad more cumbersome, as requires a deeper dive into libnop macros.

Otherwise I'm not sure how granular the /Zc:preprocessor can be. If its possible to apply per translation unit, then first option would work as expected.

I reckoned that this change might be breaking for a set of MSVC users, but didn't yet have time to hide it away completely (also requires newer SDK which includes conforming standard headers)

Any suggestions on this flag and possible routes for nicer MSVC interoperability are welcome. Hopefully MSVC moves to conformance soon, to be able to avoid this kind of details when developing cross platform apps and libraries.

diablodale commented 2 years ago

If you must use libnop...

If you are determined to use libnop...I think only your 1st option is workable. Only set the flag on compile units that are private to Luxonis. As you wrote, that means you will have to move libnop macros to private compile units and cascade changes.

It isn't workable to set that flag on anything that you don't own/manage. There is an unbounded number of main apps and 3rd party libraries. That flag will break an unbounded number of them. It isn't workable. And I think you would lose customers.

And there is also a huge (but bounded) number of MSVC compilers and Windows SDK/kits that do not work with that flag. Those widely used MSVC compilers (VS2015, VS2017, etc.) and those Windows Kits are used by the unbounded number of 3rd parties and apps. So its an (unbounded) * (huge bounded) number of failure scenarios.

...or use alternatives to libnop -- better support

I suspect there are more important things to do than build or debug serialization libraries.

diablodale commented 2 years ago

Churning on the forced preprocessor compliance flagon MSVC; otherwise can not use newer depthai-core codebase.

So far

themarpe commented 2 years ago

We are looking into it - not yet sure which path we'll take, but hopefully one that also doesn't cause too much issues on MSVC side.

Regarding alternatives, there are a couple but libnop had the best fit - We require schemaless seriliaziation library which outs protobufs and similar. There are Cereal, MessagePack and a couple others which are high profile, but those come with their own set of problems and challanges that we'd have to test out and check cross platform.

If we'll have more issues with this, we might temporarily swap back to nlohmann though.

One option is also that we'll move the serialization out from public headers - which should not cause issues with above points anymore (might be possible to include more limited set of headers, where serialization macros are applied)

diablodale commented 2 years ago

FYI, as a sample set of 1 😉, I'm shifting my overall solution -- no longer a static monolithic build. Now I have my main app + a single depthai-core.dll. That depthai-core.dll contains all of depthai's dependencies statically linked into it.

This complicates my setup/testing/distribution. But I gain isolation from some of these incompatibilities and licensing issues e.g libusb.