[libvirt PATCH] meson: add -Wall and -Wextra explicitly for buildtype=plain

Pavel Hrdina posted 1 patch 3 years, 6 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/a84825b48acdb3b1d211a8bf53fdd71853f402df.1598969276.git.phrdina@redhat.com
meson.build | 19 +++++++++++++++----
1 file changed, 15 insertions(+), 4 deletions(-)
[libvirt PATCH] meson: add -Wall and -Wextra explicitly for buildtype=plain
Posted by Pavel Hrdina 3 years, 6 months ago
If someone runs `meson setup --buildtype plain` meson ignores
warning_level=2 that is in our meson.build file. The implication is
that Meson will not automatically add -Wall which enables -Wformat.

This breaks building libvirt from git with the buildtype set to plain.

There is an issue reported [1] to not ignore warning_level silently
and the change to ignore it was done by upstream commit [2].

[1] <https://github.com/mesonbuild/meson/issues/7399>
[2] <https://github.com/mesonbuild/meson/commit/8ee1c9a07a3a35e3ed262fbc358fd86c257a966e>

Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
 meson.build | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/meson.build b/meson.build
index e193166a9b..f38785eeae 100644
--- a/meson.build
+++ b/meson.build
@@ -503,10 +503,6 @@ cc_flags_disabled = [
   # In meson this is specified using 'c_std=gnu99' in project() function.
   '-std=gnu99',
 
-  # In meson this is specified using 'warning_level=2' in project() function.
-  '-Wall',
-  '-Wextra',
-
   # don't care about C++ compiler compat
   '-Wc++-compat',
   '-Wabi',
@@ -573,6 +569,21 @@ cc_flags_disabled = [
   '-Wsuggest-attribute=malloc',
 ]
 
+# In meson this is specified using 'warning_level=2' in project() function.
+# However, meson silently ignores 'warning_level' option with 'buildtype=plain'
+# so we have to enable them explicitly.
+if get_option('buildtype') == 'plain'
+  cc_flags += [
+    '-Wall',
+    '-Wextra',
+  ]
+else
+  cc_flags_disabled += [
+    '-Wall',
+    '-Wextra',
+  ]
+endif
+
 foreach flag : cc_flags_disabled
   if cc_flags.contains(flag)
     error('@0@ is disabled but listed in cc_flags'.format(flag))
-- 
2.26.2

Re: [libvirt PATCH] meson: add -Wall and -Wextra explicitly for buildtype=plain
Posted by Jim Fehlig 3 years, 6 months ago
On 9/1/20 8:08 AM, Pavel Hrdina wrote:
> If someone runs `meson setup --buildtype plain` meson ignores
> warning_level=2 that is in our meson.build file. The implication is
> that Meson will not automatically add -Wall which enables -Wformat.
> 
> This breaks building libvirt from git with the buildtype set to plain.
> 
> There is an issue reported [1] to not ignore warning_level silently
> and the change to ignore it was done by upstream commit [2].
> 
> [1] <https://github.com/mesonbuild/meson/issues/7399>
> [2] <https://github.com/mesonbuild/meson/commit/8ee1c9a07a3a35e3ed262fbc358fd86c257a966e>
> 
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
>   meson.build | 19 +++++++++++++++----
>   1 file changed, 15 insertions(+), 4 deletions(-)

I'm just learning meson so not particularly confident to review patches, but 
will note this change causes a warning in downstream package build where the 
%meson rpm macro is used

[   23s] Compiler for C supports arguments -Wall: YES
[   23s] Compiler for C supports arguments -Wextra: YES
[   23s] meson.build:594: WARNING: Consider using the built-in warning_level 
option instead of using "-Wall".
[   23s] meson.build:594: WARNING: Consider using the built-in warning_level 
option instead of using "-Wextra".

Regards,
Jim

> 
> diff --git a/meson.build b/meson.build
> index e193166a9b..f38785eeae 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -503,10 +503,6 @@ cc_flags_disabled = [
>     # In meson this is specified using 'c_std=gnu99' in project() function.
>     '-std=gnu99',
>   
> -  # In meson this is specified using 'warning_level=2' in project() function.
> -  '-Wall',
> -  '-Wextra',
> -
>     # don't care about C++ compiler compat
>     '-Wc++-compat',
>     '-Wabi',
> @@ -573,6 +569,21 @@ cc_flags_disabled = [
>     '-Wsuggest-attribute=malloc',
>   ]
>   
> +# In meson this is specified using 'warning_level=2' in project() function.
> +# However, meson silently ignores 'warning_level' option with 'buildtype=plain'
> +# so we have to enable them explicitly.
> +if get_option('buildtype') == 'plain'
> +  cc_flags += [
> +    '-Wall',
> +    '-Wextra',
> +  ]
> +else
> +  cc_flags_disabled += [
> +    '-Wall',
> +    '-Wextra',
> +  ]
> +endif
> +
>   foreach flag : cc_flags_disabled
>     if cc_flags.contains(flag)
>       error('@0@ is disabled but listed in cc_flags'.format(flag))
> 

Re: [libvirt PATCH] meson: add -Wall and -Wextra explicitly for buildtype=plain
Posted by Pavel Hrdina 3 years, 6 months ago
On Tue, Sep 01, 2020 at 04:37:06PM -0600, Jim Fehlig wrote:
> On 9/1/20 8:08 AM, Pavel Hrdina wrote:
> > If someone runs `meson setup --buildtype plain` meson ignores
> > warning_level=2 that is in our meson.build file. The implication is
> > that Meson will not automatically add -Wall which enables -Wformat.
> > 
> > This breaks building libvirt from git with the buildtype set to plain.
> > 
> > There is an issue reported [1] to not ignore warning_level silently
> > and the change to ignore it was done by upstream commit [2].
> > 
> > [1] <https://github.com/mesonbuild/meson/issues/7399>
> > [2] <https://github.com/mesonbuild/meson/commit/8ee1c9a07a3a35e3ed262fbc358fd86c257a966e>
> > 
> > Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> > ---
> >   meson.build | 19 +++++++++++++++----
> >   1 file changed, 15 insertions(+), 4 deletions(-)
> 
> I'm just learning meson so not particularly confident to review patches, but
> will note this change causes a warning in downstream package build where the
> %meson rpm macro is used
> 
> [   23s] Compiler for C supports arguments -Wall: YES
> [   23s] Compiler for C supports arguments -Wextra: YES
> [   23s] meson.build:594: WARNING: Consider using the built-in warning_level
> option instead of using "-Wall".
> [   23s] meson.build:594: WARNING: Consider using the built-in warning_level
> option instead of using "-Wextra".

With the way how Meson handles buildtype=plain and warning_level it is
expected to see the warning.

They check the existence of these flags and print a warning regardless
of the used buildtype but as explained in the commit message Meson
ignores warning_level if buildtype=plain and doesn't add these flags
even if warning_level is set. It is inconsistent behavior in Meson and
it should be somehow fixed.

Pavel
Re: [libvirt PATCH] meson: add -Wall and -Wextra explicitly for buildtype=plain
Posted by Daniel P. Berrangé 3 years, 6 months ago
On Tue, Sep 01, 2020 at 04:08:04PM +0200, Pavel Hrdina wrote:
> If someone runs `meson setup --buildtype plain` meson ignores
> warning_level=2 that is in our meson.build file. The implication is
> that Meson will not automatically add -Wall which enables -Wformat.
> 
> This breaks building libvirt from git with the buildtype set to plain.
> 
> There is an issue reported [1] to not ignore warning_level silently
> and the change to ignore it was done by upstream commit [2].
> 
> [1] <https://github.com/mesonbuild/meson/issues/7399>
> [2] <https://github.com/mesonbuild/meson/commit/8ee1c9a07a3a35e3ed262fbc358fd86c257a966e>
> 
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
>  meson.build | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/meson.build b/meson.build
> index e193166a9b..f38785eeae 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -503,10 +503,6 @@ cc_flags_disabled = [
>    # In meson this is specified using 'c_std=gnu99' in project() function.
>    '-std=gnu99',
>  
> -  # In meson this is specified using 'warning_level=2' in project() function.
> -  '-Wall',
> -  '-Wextra',
> -
>    # don't care about C++ compiler compat
>    '-Wc++-compat',
>    '-Wabi',
> @@ -573,6 +569,21 @@ cc_flags_disabled = [
>    '-Wsuggest-attribute=malloc',
>  ]
>  
> +# In meson this is specified using 'warning_level=2' in project() function.
> +# However, meson silently ignores 'warning_level' option with 'buildtype=plain'
> +# so we have to enable them explicitly.
> +if get_option('buildtype') == 'plain'
> +  cc_flags += [
> +    '-Wall',
> +    '-Wextra',
> +  ]
> +else
> +  cc_flags_disabled += [
> +    '-Wall',
> +    '-Wextra',
> +  ]
> +endif

I'd really just simplify this to remove the conditional and always
add -Wall/-Wextra, and ignore the warning meson will print.

I tend to think they should provide a way to squelch that warning,
as it is too presumtous that apps will only ever need to set the
basic warning flags, instead of warning to set many like libvirt
does.

Or maybe, meson can be enhanced to allow setting many warnings
like libvirt uses 

> +
>  foreach flag : cc_flags_disabled
>    if cc_flags.contains(flag)
>      error('@0@ is disabled but listed in cc_flags'.format(flag))
> -- 
> 2.26.2
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|