Matthew Woehlke wrote: > On 2013-07-25 13:25, Stephen Kelly wrote: >> Brad King wrote: >> >>> On 07/25/2013 12:22 PM, Stephen Kelly wrote: >>>> library A should have a unit test which attempts to compile all >>>> of its headers with all warnings enabled. In Qt every module has a >>>> 'headersclean' unit test which does exactly that. >>> >>> While this is a good idea we're not going to assume every project has >>> such discipline. Also some consumers may include headers with a >>> different preprocessing context that exposes the warning. >> >> Right. Even if all headers are used by the upstream itself, this still >> applies. However, I still think IMPORTED=SYSTEM by *default* is a good >> idea, and let the edge case switch it back. I'm not seeing support for it >> though, so I guess I'll drop it. > > Somewhat echoing Brad's reply here, but it's not that I'm opposed to the > idea, merely "concerned" that it is possible to get non-system includes > when that is desired. > > Perhaps that is a misreading on my part, but I was getting the > impression this change would make it so that imported targets can only > ever be SYSTEM.
I didn't really make that clear as the discussion developed, but yes, I think it makes sense to allow treating the include directories as non-system somehow, even if not by default. > >>> Then why not make INTERFACE_SYSTEM_INCLUDE_DIRECTORIES the default in >>> the Qt imported targets and have a switch to turn them off? The code >>> >>> set(QT_INCLUDE_DIRS_NO_SYSTEM 1) >>> find_package(Qt5Core) >>> >>> is not so bad in the edge case that needs it. >> >> I don't like it though :). I'd prefer not to have any variables that >> change the behavior of a find_package call, so that only one find_package >> is ever needed, not multiple in different directories for scope. > > FWIW, I agree; variables controlling find_package (except search paths) > are almost always an inferior solution :-). > > That said... what about having a SYSTEM (and/or NO_SYSTEM) flag for > find_package? This could, for config-based packages with imported > targets, control the default behavior for imported include directories. I don't think that solves the problem. You might have one target which you want to treat the directories as SYSTEM, and one which you don't. I think the API for this should have target scope: A target property like set_property(TARGET foo PROPERTY IMPORTED_INTERFACE_DIRS_SYSTEM 1) and set(CMAKE_IMPORTED_INTERFACE_DIRS_SYSTEM 1) to set the default, as usual. or set_property(TARGET foo PROPERTY IMPORTED_INTERFACE_DIRS "system") set_property(TARGET foo PROPERTY IMPORTED_INTERFACE_DIRS "non_system") set(CMAKE_IMPORTED_INTERFACE_DIRS "system") >>> Either way, the tll() SYSTEM option could still be useful. >> >> I'll add that tomorrow. > > Again FWIW, a nice advantage of this is the ability to specify SYSTEM > includes per target. (I'm not sure why you'd want to build one target > with imported includes as SYSTEM and another not, but you could...) Yes. Even with the target property as above you could still do target_link_libraries(foo SYSTEM Bar::core) target_link_libraries(foo Bar::gui) We could also add the opposite if you want. target_link_libraries(foo NON_SYSTEM Bar::core) However, as the above already is enough for all use-cases and what we're discussing is an edge-case already, we don't need too much API for it. We can always add it later if needed. Thanks, Steve. -- Powered by www.kitware.com Visit other Kitware open-source projects at http://www.kitware.com/opensource/opensource.html Please keep messages on-topic and check the CMake FAQ at: http://www.cmake.org/Wiki/CMake_FAQ Follow this link to subscribe/unsubscribe: http://public.kitware.com/cgi-bin/mailman/listinfo/cmake-developers