Standardize system include targets

[RReichert]

Background

This PR works towards our Platform OKR (https://swift-nav.atlassian.net/wiki/spaces/ENG/pages/804816541/Platform+OKRs+Q3+2020) where we try and avoid using THIRD_PARTY_INCLUDE to mask our issues with submodules.

When calling find_package(...) to include a submodule project, we've by default made it so that all submodule projects are treated as foreign code, this is done by doing some cmake magic where we convert all C++ header include -I flags to -isystem. This is harmless in terms of generated code, but it does differ in terms of reporting warnings. Warnings are reported when using -I but not when using -isystem, so any issues in submodules are ignored which is problematic for a multitude of reasons:

• compiler warnings are ignored for submodules (problem if customer toolchain warning flags are ignored)
• static analysis is bounded to only the current project

There is an oddity in this logic as well that when cross compiling it no longer considers it "system" targets because of the bug in the toolchain used for starling's Build for Yocto Jenkins stage, so I'd like to fix that by simply fixing the starling build script.

Changes

The changes that I'm introducing makes it clearer what the expected behaviour is. Any Find*.cmake script that calls GenericFindDependency and has SYSTEM_INCLUDES is by default treated as foreign libraries (system headers) which we have little influence over, so things like eigen, variant, gtest, etc fall under this category. If users don't specify SYSTEM_INCLUDES it is assumed to be within our control, so they are local headers by default. We can override this behaviour by passing the THIRD_PARTY_INCLUDES_AS_SYSTEM, a true value will set everything to system header, a value of false will set everything to local.

Below is a list of all the third party projects which we consider SYSTEM (outside our control) vs LOCAL (within our control).

SYSTEM

• Benchmark
• Boost-Math
• Check
• Eigen
• FastCSV
• GFlags
• GmockGlobal
• GoogleTest
• Json
• Nanopb
• Nlopt
• Optional
• Protobuf
• RapidCheck
• Variant
• Yaml-Cpp

LOCAL

• gnss-converters
• Ixcom
• Orion-Proto
• Pal++
• Pal
• Rtcm
• Sbp
• Settings
• Starling
• Swiftlets
• Swiftnav
• Ubx

PR depends on the following to be merged

• Standardize system include targets variant#2
• https://github.com/swift-nav/libpal_cpp/pull/120

Changes tested on a number of down stream PRs

• https://github.com/swift-nav/starling/pull/3655
• https://github.com/swift-nav/swiftlets/pull/653
• https://github.com/swift-nav/orion-engine/pull/1464
• https://github.com/swift-nav/orion_proto/pull/244
• https://github.com/swift-nav/piksi_apps/pull/635

NOTE: the PR failures above are due to clang-format errors (a false positive result because it uses git diff to identify format error, but the hacks in place modify the source code to reflect a uniform cmake/variant/libpal++ repo update across the organization).

[woodfell]

I think this is a good move.

In the early days of SYSTEM_INCLUDES there were loads of problems with errors being flagged in headers of swift controlled dependencies that weren't being picked up in the dependencies themselves because of differences in compiler flags and .clang-tidy. Have these differences been resolved or the problems somehow gone away now?

[RReichert]

Have these differences been resolved or the problems somehow gone away now?

No these differences have not be resolved, we now have better control of it with this PR (ie. we can restrict/enable third party analysis with a cmake flag). Swiftlets for instance fails unless I add the THIRD_PARTY_INCLUDES_AS_SYSTEM=true flag, which restricts the clang-lint to only its source code and not its submodules. gnss-converters on the other hand doesn't have that issue because the third-party which are swiftnav repos have identical or more restrictive clang-tidy checks than starling.

Long term these clang-tidy/compile flags will need to be standard across the org, this lot of work will help us get to that point slowly.

[woodfell]

Have these differences been resolved or the problems somehow gone away now?

No these differences have not be resolved, we now have better control of it with this PR (ie. we can restrict/enable third party analysis with a cmake flag). Swiftlets for instance fails unless I add the THIRD_PARTY_INCLUDES_AS_SYSTEM=true flag, which restricts the clang-lint to only its source code and not its submodules. starling on the other hand doesn't have that issue because the third-party which are swiftnav repos have identical or more restrictive clang-tidy checks than starling.

Long term these clang-tidy/compile flags will need to be standard across the org, this lot of work will help us get to that point slowly.

Ok, probably worth making sure the orion team is aware of this since they import starling as well

[RReichert]

Ok, probably worth making sure the orion team is aware of this since they import starling as well

I've produced two draft PRs for the orion repos to emulate what it would be with this cmake submodule update:

• https://github.com/swift-nav/orion_proto/pull/244
• https://github.com/swift-nav/orion-engine/pull/1464

I'll let mark know before I hit the merge button.