mesonbuild / meson

The Meson Build System
http://mesonbuild.com
Apache License 2.0
5.54k stars 1.61k forks source link

Recursive dep.as_system() #11988

Open kiwixz opened 1 year ago

kiwixz commented 1 year ago

I expected the two following lines to be equivalent:

qt = dependency('qt5', modules: ['Gui', 'Qml', 'Quick'], include_type: 'system')
qt = dependency('qt5', modules: ['Gui', 'Qml', 'Quick']).as_system()

But the second one still pass -I to the compiler. After a quick investigation, I believe it's because the dependency internally have children dependencies and as_system() is not recursive.

While this could probably be fixed for this particular case with Qt; I wonder if it wouldn't be better to make as_system() recursive.

It could be an option but I think this should be the default behavior because if you explicitly disable warnings about a dependency by calling this function, you certainly don't want the warnings of the dependencies of your dependency.

eli-schwartz commented 1 year ago

because if you explicitly disable warnings about a dependency by calling this function, you certainly don't want the warnings of the dependencies of your dependency.

The purpose of this function isn't to "disable warnings", and that's not the purpose of the underlying GCC flag it uses either. The fact that instrumenting the compiler to modify the priority list for where to include files from when using #include_next -- an option that is supposed to be used when messing with the compiler's internal headers when, say, defining your own target platform -- also has a side effect which is not even documented in the compiler's user manual of suppressing warnings, is... unfortunate.

And I know, most people using that option do so unknowingly, for the side effect, and not for its actual purpose. But that doesn't necessarily mean it's a good idea to change how the function works in order to make the side effect operate differently.

This option can and does change the output binaries (or in a number of cases, simply causes the compiler to error out). Changing the way it works should only be performed with great caution, and because we believe that it's the correct way to handle dependencies that modify the priority order of compiler internals such as #include_next.

kiwixz commented 1 year ago

Interesting take ! I never even used or seen in action the non-standard #include_next and admittedly completely forgot it existed. The suppressing-warnings bit does seem to be documented here at least for GCC; but I'd assume most people read about it in the CMake documentation:

If SYSTEM is specified, the compiler will be told the directories are meant as system include directories on some platforms. This may have effects such as suppressing warnings or skipping the contained headers in dependency calculations (see compiler documentation). Additionally, system include directories are searched after normal include directories regardless of the order specified.

Anyway, I hope you'll consider it but I don't think it is that important considering include_type seems to work fine for this already.

eli-schwartz commented 1 year ago

The proper solution to this is, of course, https://devblogs.microsoft.com/cppblog/broken-warnings-theory/

Kind of bizarre that MSVC is the state of the art in compiler design here, also, someone should implement this in meson. :D

(not via .as_system(), but rather via e.g. .as_external() or perhaps just automatically...)

I really wish that GCC would join us in the 2020s and implement a similar idea. Imagine if we could just use -iexternal=/path/to/includes and then, separately, tune exactly which warnings we cared about from external code (which might be "none of them"). This is something it would be plausible to do automatically at the meson level, because it doesn't change the results unless the person doing the build also passes their own -Wexternal-no-foo or whatever, and all we'd be doing is ensuring that such flags become more accurate.

sammyj85 commented 1 year ago

Anyway, I hope you'll consider it but I don't think it is that important considering include_type seems to work fine for this already.

I seem to have a similar problem, but it isn't solved by a workaround I can find such as include_type: 'system'.

Subproject A (Meson) has itself a number of CMake and Meson subprojects (all "include_type: system") as dependencies. A can be built with Meson, and the subprojects are included with "-isystem" on the command line.

Subproject B (Meson) uses subproject A as a dependency. Whether A has include_type normal or system (no matter what I try), all of A and its subprojects all are included twice: first with "-I" and then "-isystem", but it is the first "-I" that has effect. To repeat, if I set subproject A's include_type as system, subproject A itself is effectively included with "-I" (because the second "-isystem" is ignored). Is there a workaround I can use here?

We all absolutely need a way of ignoring warnings in external libraries, whether or not GCC's command argument was originally designed for that.