Suppress warnings from compilation of dependencies

[pedroerp]

Today we make warnings into errors when compiling Velox (-Werror). By default, when compiling dependencies and vendored code (like folly and duckdb), we push the same compiler options, which results in the compilation of these dependencies to also turn warnings into errors. We have seen issues with that in the past (since some of this code might not have the same level of hygiene), but also that's harder to fix these warnings/errors as they do not come from Velox code. So we end having multiple cmake files where we selectively disable specific warnings when compiling dependencies as we hit them:

https://github.com/facebookincubator/velox/blob/main/velox/external/duckdb/CMakeLists.txt#L22
https://github.com/facebookincubator/velox/blob/main/CMake/resolve_dependency_modules/folly.cmake#L30
(and more)

When upgrading my system and trying a slightly newer version of gcc (12.2), I bumped into new issues, supposedly because the new compiler version added or changed the criteria for these warnings.

The suggestion comes from a quick discussion with @assignUser , but should we, by default, suppress warnings when compiling dependency code (say, folly and duckdb)? These warning just pollute the output, and make our build system harder to maintain. Ultimately, fixing these warning in most cases cannot be done in our repo and should be handled by that projects maintainers.

Something as simple as target_compile_options(duckdb PUBLIC -w) seems to solve the issue.

Thoughts?

[pedroerp]

Cc: @assignUser @kgpai @majetideepak @mbasmanova

[assignUser]

Ah yes that seems to be a good solution for dependencies gcc and clang share the flag too!

[assignUser]

I created #4673

[kgpai]

Agreed on target_compile_options !
One nit though I presume it should be target_compile_options(duckdb PRIVATE -w) since we want them to only apply to the target.

[assignUser]

Yes indeed!

[assignUser]

I just tested this and while it works for simple one target things like the vendored ddb it fails on folly as folly is only the top level target and there are sub targets that don't get the flag. For Boost we need to add it to each component but only to non-INTERFACE targets... so overall this would add quite a bit of code just to suppress the warnings (we can stop them from failing in a different way).

I think I have a better approach though that will also have other advantages but I will need to experiment a bit.

[kgpai]

Thats interesting - so target_compile_options doesnt transitively apply to its dependent targets ?

[assignUser]

A, B and C are targets.

target_compile_options(A PUBLIC -w)
target_link_libraries(A B)
target_link_libraries(B C)

Only A will be compiled with -w

target_compile_options(C PUBLIC -w)
target_link_libraries(A B)
target_link_libraries(B C)

All will be compiled with -w.

target_compile_options(C PRIVATE -w)
target_link_libraries(A B)
target_link_libraries(B C)

Only C will be compiled with -w