Fix -isystem in favor of -I flag for imported targets

[nachovizzo]

Problem tackled

The compiler flag -isystem is used instead of -I independently if the user specifies or not the SYSTEM flag. The current behavior treats all targets as SYSTEM targets, even when no one specified this

Proposed solution

Set the NO_SYSTEM_FROM_IMPORTED to the given target. NOTE: This could be replaced by IMPORTED_NO_SYSTEM

By using NO_SYSTEM_FROM_IMPORTED then

ament_target_dependencies(<target> ...) # use -I/include

will be different (not the case right now) from:

ament_target_dependencies(<target> SYSTEM ...) # use -isystem /include

Example workspace to use as a test bench

isystem_example.zip

This workspace can be used to play around with the change and actually see the difference on the build, and on the Wall output

Where -isystem is populated

The problem I found is by using this function in CMakeLists.txt

ament_target_dependencies(header_only_consumer "header_only_library")

Is injecting the keyword SYSTEM to the target_include_directories. This produces (on the example attached) a building line that includes a user-library as a system one -isystem. Most modern compilers, use this flag to ignore all sorts of warnings, etc:

/usr/bin/c++ \
  -O2 -g -DNDEBUG -Wall -Wextra -Wpedantic \
  -isystem /home/ivizzo/isystem_ws/install/header_only_library/include \
  -o CMakeFiles/header_only_consumer.dir/src/header_only_consumer.cpp.o \
  -c /home/ivizzo/isystem_ws/src/header_only_consumer/src/header_only_consumer.cpp

Downstream behavior on consumer libraries

See the workspace attached, but basically, my humble header-only library has a very silly error that should be caught by the -Wall we instruct in the CMakeLists.txt, but it's completely ignored since it's being treated as a system header (such as iostream, vector, etc). I know, if I have unit tests on the library the errors would be seen when compiling those tests, but this doesn't mean that ament is still masking out these errors

namespace header_only_library {

// This should at least give a warning
inline void foo() { int a; }

}  // namespace header_only_library

If you build the example workspace, no warnings (or errors) are reported, which for this case is wrong, since I haven't specified the flag SYSTEM in the build script

Where this "bug" was introduced

It's unclear to me, this seems like a change in CMake. There was some action about the SYSTEM flag some years ago, but this is not the problem:

• Add SYSTEM keyword option to ament_target_dependencies #297

Warning

Although I'm convinced that the current behavior of ament_target_dependencies is not adequate (in terms of CMake usage) this change should be treated carefully, since it will probably make most builds out there at least start producing some warning

[sloretz]

The compiler flag -isystem is used instead of -I independently if the user specifies or not the SYSTEM flag. The current behavior treats all targets as SYSTEM targets, even when no one specified this

This is CMake behavior rather than ament_cmake behavior, and it doesn't affect all targets. It only affects targets from other packages. In the example workspace, header_only_library is a different CMake package from header_only_consumer. I don't think ament_cmake should try to change this as it would also change the order that include directories are searched.

I know, if I have unit tests on the library the errors would be seen when compiling those tests, but this doesn't mean that ament is still masking out these errors

I think unit tests in the same package are the best solution here.

Although I'm convinced that the current behavior of ament_target_dependencies is not adequate (in terms of CMake usage) [...]

I'd recommend using target_link_libraries() these days. ament_target_dependencies isn't all that useful now that we're using modern CMake targets instead of old-style standard CMake variables. #292

[nachovizzo]

Hi @sloretz thanks a lot for such a detailed answer!

This is CMake behavior rather than ament_cmake behavior, and it doesn't affect all targets. It only affects targets from other packages. In the example workspace, header_only_library is a different CMake package from header_only_consumer. I don't think ament_cmake should try to change this as it would also change the order that include directories are searched.

Yes they are, and to some extent, it even makes sense to me an -isystem flag, but it's something I'd love to control myself. It's very common to have a workspace with multiple "sub" packages (that are, just packages :P ) In such scenarios I'd like to control the build system as they would all be from the same package. If using an external package, I don't want to bother with compiler warnings, then I'd use the SYSTEM flag from #297 (which now it's basically obsolete)

And yes, it's 100% a CMake behavior ament_cmake is just inheriting. BUT, when using plain CMake at least is more "visible", as you suggested.

I think unit tests in the same package are the best solution here.

Yes yes, we agree on this one

I'd recommend using target_link_libraries() these days. ament_target_dependencies isn't all that useful now that we're using modern CMake targets instead of old-style standard CMake variables. #292

This would be always my default option :) since modern CMake can do all these bits, the problem is that it's yet to be fully discovered in our community, so I don't see this changing soon.

In any case, thanks a lot for the feedback. If the mantainers agree then I will close this PR

[clalancette]

This would be always my default option :) since modern CMake can do all these bits, the problem is that it's yet to be fully discovered in our community, so I don't see this changing soon.

Well, ament_target_dependencies is still the option we discuss in the tutorials and other documentation. We are slowly moving away from it in the core, and one day we'll likely deprecate it. At that point, we'll fix up the documentation and not recommend it anymore.

That said, we do recommend target_link_libraries, it's just a "soft" recommendation for now.

[clalancette]

Given the last comments, I'm going to go ahead and close this. Thanks for bringing up the issue.