define QT_DEBUG or QT_NO_DEBUG depending on build type

[ggirol-rc]

to mimick what qmake does.

Fixes: #15059

cc @dcbaker

[eli-schwartz]

Prefer single quotes, as that is most commonly used in the meson codebase.

[ggirol-rc]

fixed thanks

[ggirol-rc]

unfortunately I will need help for macos support.

[eli-schwartz]

I think this happens because the qmake finder checks for macOS, and runs self._framework_detect(). Then, if a framework is found, it does an early return before seeing the changes from your patch.

[eli-schwartz]

This will result in running the entire Qt tests six times instead of three, which is pretty heavyweight..I suggest instead defining the executable twice with override_options: ['debug=true']

[ggirol-rc]

I passed override_options as arguement to executable, but it looks like it is too late, and the qt dependency was already found, and the define is already computed. So that does not work.

[eli-schwartz]

Maybe we should just make a separate test project that just builds this single executable?

[ggirol-rc]

I moved the call to get_common_defines before framework_detect, we'll see what CI says. Unfortunately it cannot be in the init of _QtBase because the other constructor of the mixin overwrites self.compile_args later.

[dcbaker]

The implementation here looks good, I know Eli has some questions about testing that need to be resolved, but the actual code looks good. Thanks for writing the patches!

[ggirol-rc]

I know Eli has some questions about testing that need to be resolved,

unfortunately I don't know how to fix that. The suggestion does not work. Unless you have a better idea, either we can leave it this way or remove the test.

[eli-schwartz]

That feels somewhat awkward and inconsistent then. If debug can be overridden per target but it only affects -g and not flags set by the dependency, then it feels like a slightly sticky situation.

I agree that I'm not actually sure how to solve this...

[dcbaker]

That almost feels like a design issue in the Dependency code, like Dependency itself should provide a way to get debug vs non-debug arguments, but that's a difficult issue to solve.

I wonder if this is a place to just put a warning and punt?

[ggirol-rc]

tbh with the current structure of the code, debug = true + method = pkg-config follows a different code path than debug = true + method = qmake so the six runs of the qt test actually add value. On my 16 cores machine with only qt5, removing the test I added saves only 3 seconds. So maybe this code is acceptable? what do you think?

[dnicolodi]

I see two issues with this change. The less severe one is that this is a backward incompatible change. The most important one is that the defines are not added accordingly to the debug option for the specific target, but accordingly to the the debug option set for the project. I don't see how adding warning makes things better as there is no way to construct a meson.build that avoids the warning (other than not having different targets in a project have different values for the debug option). I think that the convenience introduced by this approach do not justify the convenience:

executable('foo', 'foo.cc', cpp_args: [get_option('debug') ? '-DQT_DEBUG' : '-DQT_NO_DEBUG'])

does not seem so bad to me.

[ggirol-rc]

the main issue with executable('foo', 'foo.cc', cpp_args: [get_option('debug') ? '-DQT_DEBUG' : '-DQT_NO_DEBUG']) is that you have to know about QT_NO_DEBUG. If you don't know about it, your release projects are built with QT_DEBUG unknowingly, which is not the case with qmake.

[ggirol-rc]

Please also note that the current code of the qt dependency already makes decisions depending on the global debug option here

meson/mesonbuild/dependencies/qt.py

Line 300 in 9ca2764

is_debug = self.env.coredata.optstore.get_value_for('buildtype') == 'debug'

so I don't make things worse with this PR.

[dcbaker]

I suspect we have other dependencies that are broken in similar ways. I don't see how you would solve this except by having some kind of dynamic getters, but there we run up against the potential issue of ending up with both debug and !debug in the same dependency graph.