[libvirt PATCH 342/351] meson: add syntax-check

Pavel Hrdina posted 351 patches 5 years, 6 months ago
There is a newer version of this series
[libvirt PATCH 342/351] meson: add syntax-check
Posted by Pavel Hrdina 5 years, 6 months ago
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
 build-aux/Makefile.in                         |  9 +++
 .../Makefile.nonreentrant                     |  0
 build-aux/meson.build                         | 30 +++++++++
 build-aux/syntax-check.mk                     | 62 +++++++++----------
 meson.build                                   |  2 +
 5 files changed, 72 insertions(+), 31 deletions(-)
 create mode 100644 build-aux/Makefile.in
 rename Makefile.nonreentrant => build-aux/Makefile.nonreentrant (100%)
 create mode 100644 build-aux/meson.build

diff --git a/build-aux/Makefile.in b/build-aux/Makefile.in
new file mode 100644
index 00000000000..601f1ad7a19
--- /dev/null
+++ b/build-aux/Makefile.in
@@ -0,0 +1,9 @@
+# define variables
+
+top_srcdir = @top_srcdir@
+top_builddir = @top_builddir@
+FLAKE8 = @flake8_path@
+RUNUTF8 = @runutf8@
+
+# include syntax-check.mk file
+include $(top_srcdir)/build-aux/syntax-check.mk
diff --git a/Makefile.nonreentrant b/build-aux/Makefile.nonreentrant
similarity index 100%
rename from Makefile.nonreentrant
rename to build-aux/Makefile.nonreentrant
diff --git a/build-aux/meson.build b/build-aux/meson.build
new file mode 100644
index 00000000000..de916793cad
--- /dev/null
+++ b/build-aux/meson.build
@@ -0,0 +1,30 @@
+syntax_check_conf = configuration_data()
+syntax_check_conf.set('top_srcdir', meson.source_root())
+syntax_check_conf.set('top_builddir', meson.build_root())
+
+flake8_path = ''
+if flake8_prog.found()
+  flake8_path = flake8_prog.path()
+endif
+syntax_check_conf.set('flake8_path', flake8_path)
+syntax_check_conf.set('runutf8', ' '.join(runutf8))
+
+configure_file(
+  input: 'Makefile.in',
+  output: '@BASENAME@',
+  configuration: syntax_check_conf,
+)
+
+make_prog = find_program('make')
+
+# There is no way how to pass value to -j using run_target so let's use
+# it without any value to run all tests in parallel.
+run_target(
+  'syntax-check',
+  command: [
+    make_prog, '-C', meson.current_build_dir(), '-j', 'syntax-check',
+  ],
+  depends: [
+    potfiles_dep,
+  ],
+)
diff --git a/build-aux/syntax-check.mk b/build-aux/syntax-check.mk
index 3d7b644f01f..1fe2da4e637 100644
--- a/build-aux/syntax-check.mk
+++ b/build-aux/syntax-check.mk
@@ -21,7 +21,7 @@
 
 # This is reported not to work with make-3.79.1
 # ME := $(word $(words $(MAKEFILE_LIST)),$(MAKEFILE_LIST))
-ME := $(_build-aux)/syntax-check.mk
+ME := build-aux/syntax-check.mk
 
 # These variables ought to be defined through the configure.ac section
 # of the module description. But some packages import this file directly,
@@ -42,29 +42,29 @@ _equal = $(and $(findstring $(1),$(2)),$(findstring $(2),$(1)))
 GIT = git
 VC = $(GIT)
 
-VC_LIST = $(srcdir)/$(_build-aux)/vc-list-files -C $(srcdir)
+VC_LIST = $(top_srcdir)/build-aux/vc-list-files -C $(top_srcdir)
 
 # You can override this variable in syntax-check.mk to set your own regexp
 # matching files to ignore.
 VC_LIST_ALWAYS_EXCLUDE_REGEX ?= ^$$
 
 # This is to preprocess robustly the output of $(VC_LIST), so that even
-# when $(srcdir) is a pathological name like "....", the leading sed command
+# when $(top_srcdir) is a pathological name like "....", the leading sed command
 # removes only the intended prefix.
-_dot_escaped_srcdir = $(subst .,\.,$(srcdir))
-_dot_escaped_builddir = $(subst .,\.,$(builddir))
+_dot_escaped_srcdir = $(subst .,\.,$(top_srcdir))
+_dot_escaped_builddir = $(subst .,\.,$(top_builddir))
 
-# Post-process $(VC_LIST) output, prepending $(srcdir)/, but only
-# when $(srcdir) is not ".".
-ifeq ($(srcdir),.)
+# Post-process $(VC_LIST) output, prepending $(top_srcdir)/, but only
+# when $(top_srcdir) is not ".".
+ifeq ($(top_srcdir),.)
   _prepend_srcdir_prefix =
 else
-  _prepend_srcdir_prefix = | $(SED) 's|^|$(srcdir)/|'
+  _prepend_srcdir_prefix = | $(SED) 's|^|$(top_srcdir)/|'
 endif
 
 # In order to be able to consistently filter "."-relative names,
-# (i.e., with no $(srcdir) prefix), this definition is careful to
-# remove any $(srcdir) prefix, and to restore what it removes.
+# (i.e., with no $(top_srcdir) prefix), this definition is careful to
+# remove any $(top_srcdir) prefix, and to restore what it removes.
 _sc_excl = \
   $(or $(exclude_file_name_regexp--$@),^$$)
 VC_LIST_EXCEPT = \
@@ -84,11 +84,11 @@ export LC_ALL = C
 ## Sanity checks.  ##
 ## --------------- ##
 
-_cfg_mk := $(wildcard $(srcdir)/$(_build-aux)/syntax-check.mk)
+_cfg_mk := $(wildcard $(top_srcdir)/build-aux/syntax-check.mk)
 
 # Collect the names of rules starting with 'sc_'.
 syntax-check-rules := $(sort $(shell $(SED) -n \
-   's/^\(sc_[a-zA-Z0-9_-]*\):.*/\1/p' $(srcdir)/$(ME) $(_cfg_mk)))
+   's/^\(sc_[a-zA-Z0-9_-]*\):.*/\1/p' $(top_srcdir)/$(ME) $(_cfg_mk)))
 .PHONY: $(syntax-check-rules)
 
 ifeq ($(shell $(VC_LIST) >/dev/null 2>&1; echo $$?),0)
@@ -333,11 +333,11 @@ sc_flags_debug:
 # than d).  The existence of long long, and of documentation about
 # flags, makes the regex in the third test slightly harder.
 sc_flags_usage:
-	@test "$$(cat $(srcdir)/include/libvirt/libvirt-domain.h \
-	    $(srcdir)/include/libvirt/virterror.h \
-	    $(srcdir)/include/libvirt/libvirt-qemu.h \
-	    $(srcdir)/include/libvirt/libvirt-lxc.h \
-	    $(srcdir)/include/libvirt/libvirt-admin.h \
+	@test "$$(cat $(top_srcdir)/include/libvirt/libvirt-domain.h \
+	    $(top_srcdir)/include/libvirt/virterror.h \
+	    $(top_srcdir)/include/libvirt/libvirt-qemu.h \
+	    $(top_srcdir)/include/libvirt/libvirt-lxc.h \
+	    $(top_srcdir)/include/libvirt/libvirt-admin.h \
 	  | $(GREP) -c '\(long\|unsigned\) flags')" != 4 && \
 	  { echo '$(ME): new API should use "unsigned int flags"' 1>&2; \
 	    exit 1; } || :
@@ -496,7 +496,7 @@ sc_prohibit_PATH_MAX:
 	halt='dynamically allocate paths, do not use PATH_MAX' \
 	  $(_sc_search_regexp)
 
-include $(srcdir)/Makefile.nonreentrant
+include $(top_srcdir)/build-aux/Makefile.nonreentrant
 sc_prohibit_nonreentrant:
 	@prohibit="\\<(${NON_REENTRANT_RE}) *\\(" \
 	halt="use re-entrant functions (usually ending with _r)" \
@@ -833,7 +833,7 @@ sc_prohibit_gettext_markup:
 
 # Our code is divided into modular subdirectories for a reason, and
 # lower-level code must not include higher-level headers.
-cross_dirs=$(patsubst $(srcdir)/src/%.,%,$(wildcard $(srcdir)/src/*/.))
+cross_dirs=$(patsubst $(top_srcdir)/src/%.,%,$(wildcard $(top_srcdir)/src/*/.))
 cross_dirs_re=($(subst / ,/|,$(cross_dirs)))
 mid_dirs=access|admin|conf|cpu|locking|logging|rpc|security
 sc_prohibit_cross_inclusion:
@@ -1177,7 +1177,7 @@ sc_prohibit_dirent_d_type:
 #     grep-E-style regexp selecting the files to check.  For in_vc_files,
 #     the regexp is used to select matching files from the list of all
 #     version-controlled files; for in_files, it's from the names printed
-#     by "find $(srcdir)".  When neither is specified, use all files that
+#     by "find $(top_srcdir)".  When neither is specified, use all files that
 #     are under version control.
 #
 #  containing | non_containing
@@ -1249,7 +1249,7 @@ define _sc_search_regexp
 									\
    : Filter by file name;						\
    if test -n "$$in_files"; then					\
-     files=$$(find $(srcdir) | $(GREP) -E "$$in_files"			\
+     files=$$(find $(top_srcdir) | $(GREP) -E "$$in_files"			\
               | $(GREP) -Ev '$(_sc_excl)');				\
    else									\
      files=$$($(VC_LIST_EXCEPT));					\
@@ -1293,7 +1293,7 @@ sc_avoid_if_before_free:
 	@$(VC_LIST_EXCEPT)						\
 	  | $(GREP) -v useless-if-before-free				\
 	  | xargs							\
-	      $(srcdir)/$(_build-aux)/useless-if-before-free		\
+	      $(top_srcdir)/build-aux/useless-if-before-free		\
 	      $(useless_free_options)					\
 	  && { printf '$(ME): found useless "if"'			\
 		      ' before "free" above\n' 1>&2;			\
@@ -1781,9 +1781,6 @@ sc_const_long_option:
 	halt='add "const" to the above declarations'			\
 	  $(_sc_search_regexp)
 
-gen_source_files:
-	$(MAKE) -C src generated-sources
-
 fix_po_file_diag = \
 'you have changed the set of files with translatable diagnostics;\n\
 apply the above patch\n'
@@ -1805,26 +1802,26 @@ perl_translatable_files_list_ =						\
 
 # Verify that all source files using _() (more specifically, files that
 # match $(_gl_translatable_string_re)) are listed in po/POTFILES.in.
-po_file ?= $(srcdir)/po/POTFILES.in
+po_file ?= $(top_srcdir)/po/POTFILES.in
 
 # List of additional files that we want to pick up in our POTFILES.in
 # This is all generated files for RPC code.
 generated_files = \
-  $(builddir)/src/*.[ch] \
-  $(builddir)/src/*/*.[ch]
+  $(top_builddir)/src/*.[ch] \
+  $(top_builddir)/src/*/*.[ch]
 
 _gl_translatable_string_re ?= \b(N?_|gettext *)\([^)"]*("|$$)
 
 # sc_po_check can fail if generated files are not built first
-sc_po_check: gen_source_files
+sc_po_check:
 	@if test -f $(po_file); then					\
 	  $(GREP) -E -v '^(#|$$)' $(po_file)				\
 	    | $(GREP) -v '^src/false\.c$$' | sort > $@-1;		\
 	  { $(VC_LIST_EXCEPT); echo $(generated_files); }		\
 	    | xargs perl $(perl_translatable_files_list_)		\
 	    | xargs $(GREP) -E -l '$(_gl_translatable_string_re)'	\
-	    | $(SED) 's|^$(_dot_escaped_srcdir)/|@SRCDIR@|'		\
 	    | $(SED) 's|^$(_dot_escaped_builddir)/|@BUILDDIR@|'		\
+	    | $(SED) 's|^$(_dot_escaped_srcdir)/|@SRCDIR@|'		\
 	    | sort -u > $@-2;						\
 	  diff -u -L $(po_file) -L $(po_file) $@-1 $@-2			\
 	    || { printf '$(ME): '$(fix_po_file_diag) 1>&2; exit 1; };	\
@@ -2074,3 +2071,6 @@ exclude_file_name_regexp--sc_prohibit_backslash_alignment = \
 
 exclude_file_name_regexp--sc_prohibit_select = \
   ^build-aux/syntax-check\.mk|src/util/vireventglibwatch\.c$$
+
+exclude_file_name_regexp--sc_prohibit_config_h_in_headers = \
+  ^config\.h$$
diff --git a/meson.build b/meson.build
index d14223829e2..587e05a02da 100644
--- a/meson.build
+++ b/meson.build
@@ -2213,6 +2213,8 @@ subdir('po')
 
 subdir('docs')
 
+subdir('build-aux')
+
 
 # install pkgconfig files
 pkgconfig_files = [
-- 
2.26.2

Re: [libvirt PATCH 342/351] meson: add syntax-check
Posted by Peter Krempa 5 years, 6 months ago
On Thu, Jul 16, 2020 at 11:59:38 +0200, Pavel Hrdina wrote:
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
>  build-aux/Makefile.in                         |  9 +++
>  .../Makefile.nonreentrant                     |  0
>  build-aux/meson.build                         | 30 +++++++++
>  build-aux/syntax-check.mk                     | 62 +++++++++----------
>  meson.build                                   |  2 +
>  5 files changed, 72 insertions(+), 31 deletions(-)
>  create mode 100644 build-aux/Makefile.in
>  rename Makefile.nonreentrant => build-aux/Makefile.nonreentrant (100%)
>  create mode 100644 build-aux/meson.build

[...]

> +make_prog = find_program('make')
> +
> +# There is no way how to pass value to -j using run_target so let's use
> +# it without any value to run all tests in parallel.
> +run_target(
> +  'syntax-check',
> +  command: [
> +    make_prog, '-C', meson.current_build_dir(), '-j', 'syntax-check',
> +  ],

While I do run syntax check with unlimited '-j'. I don't think it's
entirely cool to impose that on everybody. Specifically overcommiting
the system is not cool. Since meson is automagically scaling can't we
use the meson-detected cpu number here?

Re: [libvirt PATCH 342/351] meson: add syntax-check
Posted by Pavel Hrdina 5 years, 6 months ago
On Tue, Jul 28, 2020 at 03:48:29PM +0200, Peter Krempa wrote:
> On Thu, Jul 16, 2020 at 11:59:38 +0200, Pavel Hrdina wrote:
> > Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> > ---
> >  build-aux/Makefile.in                         |  9 +++
> >  .../Makefile.nonreentrant                     |  0
> >  build-aux/meson.build                         | 30 +++++++++
> >  build-aux/syntax-check.mk                     | 62 +++++++++----------
> >  meson.build                                   |  2 +
> >  5 files changed, 72 insertions(+), 31 deletions(-)
> >  create mode 100644 build-aux/Makefile.in
> >  rename Makefile.nonreentrant => build-aux/Makefile.nonreentrant (100%)
> >  create mode 100644 build-aux/meson.build
> 
> [...]
> 
> > +make_prog = find_program('make')
> > +
> > +# There is no way how to pass value to -j using run_target so let's use
> > +# it without any value to run all tests in parallel.
> > +run_target(
> > +  'syntax-check',
> > +  command: [
> > +    make_prog, '-C', meson.current_build_dir(), '-j', 'syntax-check',
> > +  ],
> 
> While I do run syntax check with unlimited '-j'. I don't think it's
> entirely cool to impose that on everybody. Specifically overcommiting
> the system is not cool. Since meson is automagically scaling can't we
> use the meson-detected cpu number here?

Unfortunately no, that was the first thing I was trying to figure out
by going through meson code as well. It's not ideal I know.

Other options are to not use -j at all which is no-go or we can add some
code to detect the available number of CPUs. But again it would not
reflect the fact if user runs 'ninja -j N'.

Pavel
Re: [libvirt PATCH 342/351] meson: add syntax-check
Posted by Daniel P. Berrangé 5 years, 6 months ago
On Tue, Jul 28, 2020 at 04:04:39PM +0200, Pavel Hrdina wrote:
> On Tue, Jul 28, 2020 at 03:48:29PM +0200, Peter Krempa wrote:
> > On Thu, Jul 16, 2020 at 11:59:38 +0200, Pavel Hrdina wrote:
> > > Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> > > ---
> > >  build-aux/Makefile.in                         |  9 +++
> > >  .../Makefile.nonreentrant                     |  0
> > >  build-aux/meson.build                         | 30 +++++++++
> > >  build-aux/syntax-check.mk                     | 62 +++++++++----------
> > >  meson.build                                   |  2 +
> > >  5 files changed, 72 insertions(+), 31 deletions(-)
> > >  create mode 100644 build-aux/Makefile.in
> > >  rename Makefile.nonreentrant => build-aux/Makefile.nonreentrant (100%)
> > >  create mode 100644 build-aux/meson.build
> > 
> > [...]
> > 
> > > +make_prog = find_program('make')
> > > +
> > > +# There is no way how to pass value to -j using run_target so let's use
> > > +# it without any value to run all tests in parallel.
> > > +run_target(
> > > +  'syntax-check',
> > > +  command: [
> > > +    make_prog, '-C', meson.current_build_dir(), '-j', 'syntax-check',
> > > +  ],
> > 
> > While I do run syntax check with unlimited '-j'. I don't think it's
> > entirely cool to impose that on everybody. Specifically overcommiting
> > the system is not cool. Since meson is automagically scaling can't we
> > use the meson-detected cpu number here?
> 
> Unfortunately no, that was the first thing I was trying to figure out
> by going through meson code as well. It's not ideal I know.
> 
> Other options are to not use -j at all which is no-go or we can add some
> code to detect the available number of CPUs. But again it would not
> reflect the fact if user runs 'ninja -j N'.

In libosinfo we put "syntax-check" as part of the unit tests, rather
than as a separate meson target. With that you don't need to pass
-j to syntax-check, because other unit tests are running in parallel
already, and chances are syntax-check will finish first even when
serialized.


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 :|

Re: [libvirt PATCH 342/351] meson: add syntax-check
Posted by Pavel Hrdina 5 years, 6 months ago
On Tue, Jul 28, 2020 at 03:08:11PM +0100, Daniel P. Berrangé wrote:
> On Tue, Jul 28, 2020 at 04:04:39PM +0200, Pavel Hrdina wrote:
> > On Tue, Jul 28, 2020 at 03:48:29PM +0200, Peter Krempa wrote:
> > > On Thu, Jul 16, 2020 at 11:59:38 +0200, Pavel Hrdina wrote:
> > > > Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> > > > ---
> > > >  build-aux/Makefile.in                         |  9 +++
> > > >  .../Makefile.nonreentrant                     |  0
> > > >  build-aux/meson.build                         | 30 +++++++++
> > > >  build-aux/syntax-check.mk                     | 62 +++++++++----------
> > > >  meson.build                                   |  2 +
> > > >  5 files changed, 72 insertions(+), 31 deletions(-)
> > > >  create mode 100644 build-aux/Makefile.in
> > > >  rename Makefile.nonreentrant => build-aux/Makefile.nonreentrant (100%)
> > > >  create mode 100644 build-aux/meson.build
> > > 
> > > [...]
> > > 
> > > > +make_prog = find_program('make')
> > > > +
> > > > +# There is no way how to pass value to -j using run_target so let's use
> > > > +# it without any value to run all tests in parallel.
> > > > +run_target(
> > > > +  'syntax-check',
> > > > +  command: [
> > > > +    make_prog, '-C', meson.current_build_dir(), '-j', 'syntax-check',
> > > > +  ],
> > > 
> > > While I do run syntax check with unlimited '-j'. I don't think it's
> > > entirely cool to impose that on everybody. Specifically overcommiting
> > > the system is not cool. Since meson is automagically scaling can't we
> > > use the meson-detected cpu number here?
> > 
> > Unfortunately no, that was the first thing I was trying to figure out
> > by going through meson code as well. It's not ideal I know.
> > 
> > Other options are to not use -j at all which is no-go or we can add some
> > code to detect the available number of CPUs. But again it would not
> > reflect the fact if user runs 'ninja -j N'.
> 
> In libosinfo we put "syntax-check" as part of the unit tests, rather
> than as a separate meson target. With that you don't need to pass
> -j to syntax-check, because other unit tests are running in parallel
> already, and chances are syntax-check will finish first even when
> serialized.

That was my idea as well, but I know that we don't want to run
syntax-check in downstream packaging as it usually fails because of
outdated tools or GCC or other bit's involved in syntax check.

On the other hand we would have syntax-check part of the meson dist
command which is equivalent to make distcheck. Not sure if desired.

To solve the issue with downstream packaging meson supports labels by
using 'suite' argument when calling test() function. We can use
suite: 'syntax-check' for this purpose and in spec file we can have this
line:

    VIR_TEST_DEBUG=1 %meson_test --no-suite syntax-check

With all that in mind, running 'meson test' or 'ninja test' will show
that our syntax-check running using single thread takes really long.

On my box all the whole test suite without syntax-check takes 5.793s
so let's say 6s but the syntax-check part takes 24.716s.

I don't think we want to do that.

Pavel
Re: [libvirt PATCH 342/351] meson: add syntax-check
Posted by Peter Krempa 5 years, 6 months ago
On Tue, Jul 28, 2020 at 15:08:11 +0100, Daniel Berrange wrote:
> On Tue, Jul 28, 2020 at 04:04:39PM +0200, Pavel Hrdina wrote:
> > On Tue, Jul 28, 2020 at 03:48:29PM +0200, Peter Krempa wrote:
> > > On Thu, Jul 16, 2020 at 11:59:38 +0200, Pavel Hrdina wrote:
> > > > Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> > > > ---
> > > >  build-aux/Makefile.in                         |  9 +++
> > > >  .../Makefile.nonreentrant                     |  0
> > > >  build-aux/meson.build                         | 30 +++++++++
> > > >  build-aux/syntax-check.mk                     | 62 +++++++++----------
> > > >  meson.build                                   |  2 +
> > > >  5 files changed, 72 insertions(+), 31 deletions(-)
> > > >  create mode 100644 build-aux/Makefile.in
> > > >  rename Makefile.nonreentrant => build-aux/Makefile.nonreentrant (100%)
> > > >  create mode 100644 build-aux/meson.build
> > > 
> > > [...]
> > > 
> > > > +make_prog = find_program('make')
> > > > +
> > > > +# There is no way how to pass value to -j using run_target so let's use
> > > > +# it without any value to run all tests in parallel.
> > > > +run_target(
> > > > +  'syntax-check',
> > > > +  command: [
> > > > +    make_prog, '-C', meson.current_build_dir(), '-j', 'syntax-check',
> > > > +  ],
> > > 
> > > While I do run syntax check with unlimited '-j'. I don't think it's
> > > entirely cool to impose that on everybody. Specifically overcommiting
> > > the system is not cool. Since meson is automagically scaling can't we
> > > use the meson-detected cpu number here?
> > 
> > Unfortunately no, that was the first thing I was trying to figure out
> > by going through meson code as well. It's not ideal I know.
> > 
> > Other options are to not use -j at all which is no-go or we can add some
> > code to detect the available number of CPUs. But again it would not
> > reflect the fact if user runs 'ninja -j N'.
> 
> In libosinfo we put "syntax-check" as part of the unit tests, rather
> than as a separate meson target. With that you don't need to pass
> -j to syntax-check, because other unit tests are running in parallel
> already, and chances are syntax-check will finish first even when
> serialized.

Unfortunately it's not even close.

Serialized syntax-check:
real	0m22.139s
user	0m24.209s
sys	0m6.788s

test suite:
real	0m4.833s
user	0m12.408s
sys	0m3.918s

syntax-check with -j == ncpus: (24 thread box)

real	0m2.099s
user	0m28.558s
sys	0m7.739s


As said, I'm a big fan of -jncpus or -j. so I really want to keep it
especially given the data above, but on the other hand I don't want to
set the CI boxes on fire.

Re: [libvirt PATCH 342/351] meson: add syntax-check
Posted by Pavel Hrdina 5 years, 6 months ago
On Tue, Jul 28, 2020 at 04:19:56PM +0200, Peter Krempa wrote:
> On Tue, Jul 28, 2020 at 15:08:11 +0100, Daniel Berrange wrote:
> > On Tue, Jul 28, 2020 at 04:04:39PM +0200, Pavel Hrdina wrote:
> > > On Tue, Jul 28, 2020 at 03:48:29PM +0200, Peter Krempa wrote:
> > > > On Thu, Jul 16, 2020 at 11:59:38 +0200, Pavel Hrdina wrote:
> > > > > Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> > > > > ---
> > > > >  build-aux/Makefile.in                         |  9 +++
> > > > >  .../Makefile.nonreentrant                     |  0
> > > > >  build-aux/meson.build                         | 30 +++++++++
> > > > >  build-aux/syntax-check.mk                     | 62 +++++++++----------
> > > > >  meson.build                                   |  2 +
> > > > >  5 files changed, 72 insertions(+), 31 deletions(-)
> > > > >  create mode 100644 build-aux/Makefile.in
> > > > >  rename Makefile.nonreentrant => build-aux/Makefile.nonreentrant (100%)
> > > > >  create mode 100644 build-aux/meson.build
> > > > 
> > > > [...]
> > > > 
> > > > > +make_prog = find_program('make')
> > > > > +
> > > > > +# There is no way how to pass value to -j using run_target so let's use
> > > > > +# it without any value to run all tests in parallel.
> > > > > +run_target(
> > > > > +  'syntax-check',
> > > > > +  command: [
> > > > > +    make_prog, '-C', meson.current_build_dir(), '-j', 'syntax-check',
> > > > > +  ],
> > > > 
> > > > While I do run syntax check with unlimited '-j'. I don't think it's
> > > > entirely cool to impose that on everybody. Specifically overcommiting
> > > > the system is not cool. Since meson is automagically scaling can't we
> > > > use the meson-detected cpu number here?
> > > 
> > > Unfortunately no, that was the first thing I was trying to figure out
> > > by going through meson code as well. It's not ideal I know.
> > > 
> > > Other options are to not use -j at all which is no-go or we can add some
> > > code to detect the available number of CPUs. But again it would not
> > > reflect the fact if user runs 'ninja -j N'.
> > 
> > In libosinfo we put "syntax-check" as part of the unit tests, rather
> > than as a separate meson target. With that you don't need to pass
> > -j to syntax-check, because other unit tests are running in parallel
> > already, and chances are syntax-check will finish first even when
> > serialized.
> 
> Unfortunately it's not even close.
> 
> Serialized syntax-check:
> real	0m22.139s
> user	0m24.209s
> sys	0m6.788s
> 
> test suite:
> real	0m4.833s
> user	0m12.408s
> sys	0m3.918s
> 
> syntax-check with -j == ncpus: (24 thread box)
> 
> real	0m2.099s
> user	0m28.558s
> sys	0m7.739s
> 
> 
> As said, I'm a big fan of -jncpus or -j. so I really want to keep it
> especially given the data above, but on the other hand I don't want to
> set the CI boxes on fire.

So I was trying to figure out what to do with our syntax-check and this
could be one solution:


rc = run_command(
  'sed', '-n',
  's/^\\(sc_[a-zA-Z0-9_-]*\\):.*/\\1/p',
  meson.current_source_dir() / 'syntax-check.mk',
  check: true,
)

sc_tests = rc.stdout().strip().split()


This is how syntax-check.mk gets the list of targets to run for
syntax-check target. We can use the same list to define tests like this:


foreach target : sc_tests
  rc = run_command(
    python3_prog, '-c',
    'print("@0@".replace("sc_", ""))'.format(target),
    check: true,
    env: runutf8,
  )
  name = rc.stdout().strip()

  test(
    name,
    make_prog,
    args: [ '-C', meson.current_build_dir(), target ],
    depends: [
      potfiles_dep,
    ],
    suite: 'syntax-check',
  )
endforeach


It could be simplified if we don't care that all the syntax-check tests
would have 'sc_' prefix.


To use it with meson/ninja would be possible with the following
commands:


ninja test

    - will run all tests regardless of the test suite, it is not
      possible to specify test suite

meson test
    - same as ninja test

meson test --no-suite syntax-check
    - this would be equivalent to make check

meson test --suite syntax-check
    - this would be equivalent to make syntax-check


In addition we can add `suite: 'check'` to the remaining test() calls to
make `meson test --suite check` available as well.

With this change running `meson test --suite syntax-check` takes on my
machine 3.037s compared to `ninja syntax-check` which takes 2.256s.

Pavel
Re: [libvirt PATCH 342/351] meson: add syntax-check
Posted by Daniel P. Berrangé 5 years, 6 months ago
On Wed, Jul 29, 2020 at 09:11:11AM +0200, Pavel Hrdina wrote:
> So I was trying to figure out what to do with our syntax-check and this
> could be one solution:
> 
> 
> rc = run_command(
>   'sed', '-n',
>   's/^\\(sc_[a-zA-Z0-9_-]*\\):.*/\\1/p',
>   meson.current_source_dir() / 'syntax-check.mk',
>   check: true,
> )
> 
> sc_tests = rc.stdout().strip().split()
> 
> 
> This is how syntax-check.mk gets the list of targets to run for
> syntax-check target. We can use the same list to define tests like this:
> 
> 
> foreach target : sc_tests
>   rc = run_command(
>     python3_prog, '-c',
>     'print("@0@".replace("sc_", ""))'.format(target),
>     check: true,
>     env: runutf8,
>   )
>   name = rc.stdout().strip()
> 
>   test(
>     name,
>     make_prog,
>     args: [ '-C', meson.current_build_dir(), target ],
>     depends: [
>       potfiles_dep,
>     ],
>     suite: 'syntax-check',
>   )
> endforeach

I like this idea as it eliminates a little bit more of the "make"
usage. BTW, can we just run them more directly instead of via
"python_prog" ? The tests don't use python, so avoiding creating
a python intepretor for each syntax check rule probably wins for
performance a litle


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 :|

Re: [libvirt PATCH 342/351] meson: add syntax-check
Posted by Pavel Hrdina 5 years, 6 months ago
On Wed, Jul 29, 2020 at 10:29:46AM +0100, Daniel P. Berrangé wrote:
> On Wed, Jul 29, 2020 at 09:11:11AM +0200, Pavel Hrdina wrote:
> > So I was trying to figure out what to do with our syntax-check and this
> > could be one solution:
> > 
> > 
> > rc = run_command(
> >   'sed', '-n',
> >   's/^\\(sc_[a-zA-Z0-9_-]*\\):.*/\\1/p',
> >   meson.current_source_dir() / 'syntax-check.mk',
> >   check: true,
> > )
> > 
> > sc_tests = rc.stdout().strip().split()
> > 
> > 
> > This is how syntax-check.mk gets the list of targets to run for
> > syntax-check target. We can use the same list to define tests like this:
> > 
> > 
> > foreach target : sc_tests
> >   rc = run_command(
> >     python3_prog, '-c',
> >     'print("@0@".replace("sc_", ""))'.format(target),
> >     check: true,
> >     env: runutf8,
> >   )
> >   name = rc.stdout().strip()
> > 
> >   test(
> >     name,
> >     make_prog,
> >     args: [ '-C', meson.current_build_dir(), target ],
> >     depends: [
> >       potfiles_dep,
> >     ],
> >     suite: 'syntax-check',
> >   )
> > endforeach
> 
> I like this idea as it eliminates a little bit more of the "make"
> usage. BTW, can we just run them more directly instead of via
> "python_prog" ? The tests don't use python, so avoiding creating
> a python intepretor for each syntax check rule probably wins for
> performance a litle

The run_command() using python3_prog is executed during `meson setup`
phase and the only purpose of that is to rename `sc_test_name` to
`test_name`. It will not affect the performance of running meson test
as that one will execute only `make_prog -C builddir sc_test_name`.

I'm OK with dropping the run_command() part completely, which would make
the output of `meson test` look like this:

147/154 libvirt:syntax-check / sc_prohibit_test_double_equal  OK

instead of

147/154 libvirt:syntax-check / prohibit_test_double_equal  OK

I just realized a huge drawback of this approach. In order to run
`meson test` or `ninja test` it will first compile everything. It can be
disabled by running `meson test --no-rebuild` which will ignore explicit
dependencies as well.

To workaround it for our CI codestyle job we would have to run these
commands:

    meson build
    ninja -C build libvirt-pot-dep
    meson test -C build --suite syntax-check --no-rebuild

The libvirt-pot-dep is an alias target which ensures that all generated
sources are created in order to build libvirt.pot or for our
syntax-check script.

Pavel
Re: [libvirt PATCH 342/351] meson: add syntax-check
Posted by Peter Krempa 5 years, 6 months ago
On Wed, Jul 29, 2020 at 11:47:31 +0200, Pavel Hrdina wrote:
> On Wed, Jul 29, 2020 at 10:29:46AM +0100, Daniel P. Berrangé wrote:
> > On Wed, Jul 29, 2020 at 09:11:11AM +0200, Pavel Hrdina wrote:
> > > So I was trying to figure out what to do with our syntax-check and this
> > > could be one solution:
> > > 
> > > 
> > > rc = run_command(
> > >   'sed', '-n',
> > >   's/^\\(sc_[a-zA-Z0-9_-]*\\):.*/\\1/p',
> > >   meson.current_source_dir() / 'syntax-check.mk',
> > >   check: true,
> > > )
> > > 
> > > sc_tests = rc.stdout().strip().split()
> > > 
> > > 
> > > This is how syntax-check.mk gets the list of targets to run for
> > > syntax-check target. We can use the same list to define tests like this:
> > > 
> > > 
> > > foreach target : sc_tests
> > >   rc = run_command(
> > >     python3_prog, '-c',
> > >     'print("@0@".replace("sc_", ""))'.format(target),
> > >     check: true,
> > >     env: runutf8,
> > >   )
> > >   name = rc.stdout().strip()
> > > 
> > >   test(
> > >     name,
> > >     make_prog,
> > >     args: [ '-C', meson.current_build_dir(), target ],
> > >     depends: [
> > >       potfiles_dep,
> > >     ],
> > >     suite: 'syntax-check',
> > >   )
> > > endforeach
> > 
> > I like this idea as it eliminates a little bit more of the "make"
> > usage. BTW, can we just run them more directly instead of via
> > "python_prog" ? The tests don't use python, so avoiding creating
> > a python intepretor for each syntax check rule probably wins for
> > performance a litle
> 
> The run_command() using python3_prog is executed during `meson setup`
> phase and the only purpose of that is to rename `sc_test_name` to
> `test_name`. It will not affect the performance of running meson test
> as that one will execute only `make_prog -C builddir sc_test_name`.
> 
> I'm OK with dropping the run_command() part completely, which would make
> the output of `meson test` look like this:
> 
> 147/154 libvirt:syntax-check / sc_prohibit_test_double_equal  OK

I think this is okay. It even helps you finding the checking rule.

> 
> instead of
> 
> 147/154 libvirt:syntax-check / prohibit_test_double_equal  OK
> 
> I just realized a huge drawback of this approach. In order to run
> `meson test` or `ninja test` it will first compile everything. It can be
> disabled by running `meson test --no-rebuild` which will ignore explicit
> dependencies as well.
> 
> To workaround it for our CI codestyle job we would have to run these
> commands:
> 
>     meson build
>     ninja -C build libvirt-pot-dep
>     meson test -C build --suite syntax-check --no-rebuild

IMO we can live with this. CI can be fixed. Downstreams which
specifically care about backporting non-conformat patches will probably
disable the syntax-check suite anyways. For developers, you usually
compile stuff anyways before running tests.

Re: [libvirt PATCH 342/351] meson: add syntax-check
Posted by Pavel Hrdina 5 years, 6 months ago
On Wed, Jul 29, 2020 at 12:00:46PM +0200, Peter Krempa wrote:
> On Wed, Jul 29, 2020 at 11:47:31 +0200, Pavel Hrdina wrote:
> > On Wed, Jul 29, 2020 at 10:29:46AM +0100, Daniel P. Berrangé wrote:
> > > On Wed, Jul 29, 2020 at 09:11:11AM +0200, Pavel Hrdina wrote:
> > > > So I was trying to figure out what to do with our syntax-check and this
> > > > could be one solution:
> > > > 
> > > > 
> > > > rc = run_command(
> > > >   'sed', '-n',
> > > >   's/^\\(sc_[a-zA-Z0-9_-]*\\):.*/\\1/p',
> > > >   meson.current_source_dir() / 'syntax-check.mk',
> > > >   check: true,
> > > > )
> > > > 
> > > > sc_tests = rc.stdout().strip().split()
> > > > 
> > > > 
> > > > This is how syntax-check.mk gets the list of targets to run for
> > > > syntax-check target. We can use the same list to define tests like this:
> > > > 
> > > > 
> > > > foreach target : sc_tests
> > > >   rc = run_command(
> > > >     python3_prog, '-c',
> > > >     'print("@0@".replace("sc_", ""))'.format(target),
> > > >     check: true,
> > > >     env: runutf8,
> > > >   )
> > > >   name = rc.stdout().strip()
> > > > 
> > > >   test(
> > > >     name,
> > > >     make_prog,
> > > >     args: [ '-C', meson.current_build_dir(), target ],
> > > >     depends: [
> > > >       potfiles_dep,
> > > >     ],
> > > >     suite: 'syntax-check',
> > > >   )
> > > > endforeach
> > > 
> > > I like this idea as it eliminates a little bit more of the "make"
> > > usage. BTW, can we just run them more directly instead of via
> > > "python_prog" ? The tests don't use python, so avoiding creating
> > > a python intepretor for each syntax check rule probably wins for
> > > performance a litle
> > 
> > The run_command() using python3_prog is executed during `meson setup`
> > phase and the only purpose of that is to rename `sc_test_name` to
> > `test_name`. It will not affect the performance of running meson test
> > as that one will execute only `make_prog -C builddir sc_test_name`.
> > 
> > I'm OK with dropping the run_command() part completely, which would make
> > the output of `meson test` look like this:
> > 
> > 147/154 libvirt:syntax-check / sc_prohibit_test_double_equal  OK
> 
> I think this is okay. It even helps you finding the checking rule.
> 
> > 
> > instead of
> > 
> > 147/154 libvirt:syntax-check / prohibit_test_double_equal  OK
> > 
> > I just realized a huge drawback of this approach. In order to run
> > `meson test` or `ninja test` it will first compile everything. It can be
> > disabled by running `meson test --no-rebuild` which will ignore explicit
> > dependencies as well.
> > 
> > To workaround it for our CI codestyle job we would have to run these
> > commands:
> > 
> >     meson build
> >     ninja -C build libvirt-pot-dep
> >     meson test -C build --suite syntax-check --no-rebuild
> 
> IMO we can live with this. CI can be fixed. Downstreams which
> specifically care about backporting non-conformat patches will probably
> disable the syntax-check suite anyways. For developers, you usually
> compile stuff anyways before running tests.

Perfect, I'm OK with all of the above so I'll go with it.

Thanks

Pavel