[libvirt] [PATCH] build: exclude more files from all the syntax checks
The majority of the syntax check is taylored for C sources, so some of the checks already cause false positives for non-C sources (and thus there are exclusion regexps in place). Instead, just exclude more non-C files from all the checks: - pot files: they are templates for po files (already excluded), and they are automatically generated from sources - pl files: Perl sources, which have own APIs, style, etc; they are helper scripts, not "real" sources - spec/spec.in files: RPM packaging files - js/woff/html.in files: files for web pages - diff/patch files: patches - stp files: SystemTap scripts - syms files: linker symbols files - conf files: generic configuration files - data/cpuinfo files: procinfo/cpuinfo files Python files (.py) are left allowed, since there is at least one syntax check specifically for them. Signed-off-by: Pino Toscano <ptoscano@redhat.com> --- cfg.mk | 29 +++++++++++++---------------- 1 file changed, 13 insertions(+), 16 deletions(-) diff --git a/cfg.mk b/cfg.mk index 0f4065b98..44a19594e 100644 --- a/cfg.mk +++ b/cfg.mk @@ -91,7 +91,7 @@ endif # Files that should never cause syntax check failures. VC_LIST_ALWAYS_EXCLUDE_REGEX = \ - (^(docs/(news(-[0-9]*)?\.html\.in|.*\.patch))|\.(po|fig|gif|ico|png))$$ + \.(po|fig|gif|ico|png|pot|pl|spec|spec\.in|js|woff|diff|patch|html\.in|stp|syms|conf|data|cpuinfo)$$ # Functions like free() that are no-ops on NULL arguments. useless_free_options = \ @@ -1129,12 +1129,12 @@ exclude_file_name_regexp--sc_flags_usage = \ ^(cfg\.mk|docs/|src/util/virnetdevtap\.c$$|tests/((vir(cgroup|pci|test|usb)|nss|qemuxml2argv)mock|virfilewrapper)\.c$$) exclude_file_name_regexp--sc_libvirt_unmarked_diagnostics = \ - ^(src/rpc/gendispatch\.pl$$|tests/) + ^tests/ -exclude_file_name_regexp--sc_po_check = ^(docs/|src/rpc/gendispatch\.pl$$) +exclude_file_name_regexp--sc_po_check = ^(docs/) exclude_file_name_regexp--sc_prohibit_VIR_ERR_NO_MEMORY = \ - ^(cfg\.mk|include/libvirt/virterror\.h|daemon/dispatch\.c|src/util/virerror\.c|docs/internals/oomtesting\.html\.in)$$ + ^(cfg\.mk|include/libvirt/virterror\.h|daemon/dispatch\.c|src/util/virerror\.c)$$ exclude_file_name_regexp--sc_prohibit_PATH_MAX = \ ^cfg\.mk$$ @@ -1143,16 +1143,16 @@ exclude_file_name_regexp--sc_prohibit_access_xok = \ ^(cfg\.mk|src/util/virutil\.c)$$ exclude_file_name_regexp--sc_prohibit_asprintf = \ - ^(cfg\.mk|bootstrap.conf$$|examples/|src/util/virstring\.[ch]$$|tests/vircgroupmock\.c$$) + ^(cfg\.mk|examples/|src/util/virstring\.[ch]$$|tests/vircgroupmock\.c$$) exclude_file_name_regexp--sc_prohibit_strdup = \ ^(docs/|examples/|src/util/virstring\.c|tests/vir(netserverclient|cgroup)mock.c|tests/commandhelper\.c$$) exclude_file_name_regexp--sc_prohibit_close = \ - (\.p[yl]$$|\.spec\.in$$|^docs/|^(src/util/virfile\.c|src/libvirt-stream\.c|tests/vir.+mock\.c|tests/commandhelper\.c)$$) + (\.py$$|^docs/|^(src/util/virfile\.c|src/libvirt-stream\.c|tests/vir.+mock\.c|tests/commandhelper\.c)$$) exclude_file_name_regexp--sc_prohibit_empty_lines_at_EOF = \ - (^tests/(qemuhelp|virhostcpu|virpcitest)data/|docs/js/.*\.js|docs/fonts/.*\.woff|\.diff|tests/virconfdata/no-newline\.conf$$) + (^tests/(qemuhelp|virhostcpu|virpcitest)data/) _src2=src/(util/vircommand|libvirt|lxc/lxc_controller|locking/lock_daemon|logging/log_daemon) exclude_file_name_regexp--sc_prohibit_fork_wrappers = \ @@ -1163,17 +1163,14 @@ exclude_file_name_regexp--sc_prohibit_gethostname = ^src/util/virutil\.c$$ exclude_file_name_regexp--sc_prohibit_internal_functions = \ ^src/(util/(viralloc|virutil|virfile)\.[hc]|esx/esx_vi\.c)$$ -exclude_file_name_regexp--sc_prohibit_newline_at_end_of_diagnostic = \ - ^src/rpc/gendispatch\.pl$$ - exclude_file_name_regexp--sc_prohibit_nonreentrant = \ - ^((po|tests)/|docs/.*(py|js|html\.in)|run.in$$|tools/wireshark/util/genxdrstub\.pl$$) + ^(tests/|docs/.*py|run.in$$) exclude_file_name_regexp--sc_prohibit_select = \ ^cfg\.mk$$ exclude_file_name_regexp--sc_prohibit_raw_allocation = \ - ^(docs/hacking\.html\.in|src/util/viralloc\.[ch]|examples/.*|tests/(securityselinuxhelper|(vircgroup|nss)mock|commandhelper)\.c|tools/wireshark/src/packet-libvirt\.c)$$ + ^(src/util/viralloc\.[ch]|examples/.*|tests/(securityselinuxhelper|(vircgroup|nss)mock|commandhelper)\.c|tools/wireshark/src/packet-libvirt\.c)$$ exclude_file_name_regexp--sc_prohibit_readlink = \ ^src/(util/virutil|lxc/lxc_container)\.c$$ @@ -1181,7 +1178,7 @@ exclude_file_name_regexp--sc_prohibit_readlink = \ exclude_file_name_regexp--sc_prohibit_setuid = ^src/util/virutil\.c$$ exclude_file_name_regexp--sc_prohibit_sprintf = \ - ^(cfg\.mk|docs/hacking\.html\.in|.*\.stp|.*\.pl)$$ + ^cfg\.mk$$ exclude_file_name_regexp--sc_prohibit_strncpy = ^src/util/virstring\.c$$ @@ -1200,10 +1197,10 @@ exclude_file_name_regexp--sc_require_config_h_first = \ ^(examples/|tools/virsh-edit\.c$$) exclude_file_name_regexp--sc_trailing_blank = \ - /qemuhelpdata/|/sysinfodata/.*\.data|/virhostcpudata/.*\.cpuinfo$$ + /qemuhelpdata/ exclude_file_name_regexp--sc_unmarked_diagnostics = \ - ^(docs/apibuild.py|tests/virt-aa-helper-test|docs/js/.*\.js)$$ + ^(docs/apibuild.py|tests/virt-aa-helper-test)$$ exclude_file_name_regexp--sc_size_of_brackets = cfg.mk @@ -1243,7 +1240,7 @@ exclude_file_name_regexp--sc_prohibit_devname = \ ^(tools/virsh.pod|cfg.mk|docs/.*)$$ exclude_file_name_regexp--sc_prohibit_virXXXFree = \ - ^(docs/|tests/|examples/|tools/|cfg.mk|src/test/test_driver.c|src/libvirt_public.syms|include/libvirt/libvirt-(domain|network|nodedev|storage|stream|secret|nwfilter|interface|domain-snapshot).h|src/libvirt-(domain|qemu|network|nodedev|storage|stream|secret|nwfilter|interface|domain-snapshot).c$$) + ^(docs/|tests/|examples/|tools/|cfg.mk|src/test/test_driver.c|include/libvirt/libvirt-(domain|network|nodedev|storage|stream|secret|nwfilter|interface|domain-snapshot).h|src/libvirt-(domain|qemu|network|nodedev|storage|stream|secret|nwfilter|interface|domain-snapshot).c$$) exclude_file_name_regexp--sc_prohibit_sysconf_pagesize = \ ^(cfg\.mk|src/util/virutil\.c)$$ -- 2.13.6
On 10/05/2017 01:07 PM, Pino Toscano wrote:
The majority of the syntax check is taylored for C sources, so some of the checks already cause false positives for non-C sources (and thus there are exclusion regexps in place).
Instead, just exclude more non-C files from all the checks: - pot files: they are templates for po files (already excluded), and they are automatically generated from sources - pl files: Perl sources, which have own APIs, style, etc; they are helper scripts, not "real" sources - spec/spec.in files: RPM packaging files - js/woff/html.in files: files for web pages - diff/patch files: patches - stp files: SystemTap scripts - syms files: linker symbols files - conf files: generic configuration files - data/cpuinfo files: procinfo/cpuinfo files
Python files (.py) are left allowed, since there is at least one syntax check specifically for them.
Signed-off-by: Pino Toscano <ptoscano@redhat.com> --- cfg.mk | 29 +++++++++++++---------------- 1 file changed, 13 insertions(+), 16 deletions(-)
diff --git a/cfg.mk b/cfg.mk index 0f4065b98..44a19594e 100644 --- a/cfg.mk +++ b/cfg.mk @@ -91,7 +91,7 @@ endif
# Files that should never cause syntax check failures. VC_LIST_ALWAYS_EXCLUDE_REGEX = \ - (^(docs/(news(-[0-9]*)?\.html\.in|.*\.patch))|\.(po|fig|gif|ico|png))$$ + \.(po|fig|gif|ico|png|pot|pl|spec|spec\.in|js|woff|diff|patch|html\.in|stp|syms|conf|data|cpuinfo)$$
.spec is not in the repo, so it's never checked. And then, we perhaps want to check .spec.in? For instance for space at EOF. ACK if you leave those two out from the list. Michal
On Friday, 6 October 2017 12:32:28 CEST Michal Privoznik wrote:
On 10/05/2017 01:07 PM, Pino Toscano wrote:
The majority of the syntax check is taylored for C sources, so some of the checks already cause false positives for non-C sources (and thus there are exclusion regexps in place).
Instead, just exclude more non-C files from all the checks: - pot files: they are templates for po files (already excluded), and they are automatically generated from sources - pl files: Perl sources, which have own APIs, style, etc; they are helper scripts, not "real" sources - spec/spec.in files: RPM packaging files - js/woff/html.in files: files for web pages - diff/patch files: patches - stp files: SystemTap scripts - syms files: linker symbols files - conf files: generic configuration files - data/cpuinfo files: procinfo/cpuinfo files
Python files (.py) are left allowed, since there is at least one syntax check specifically for them.
Signed-off-by: Pino Toscano <ptoscano@redhat.com> --- cfg.mk | 29 +++++++++++++---------------- 1 file changed, 13 insertions(+), 16 deletions(-)
diff --git a/cfg.mk b/cfg.mk index 0f4065b98..44a19594e 100644 --- a/cfg.mk +++ b/cfg.mk @@ -91,7 +91,7 @@ endif
# Files that should never cause syntax check failures. VC_LIST_ALWAYS_EXCLUDE_REGEX = \ - (^(docs/(news(-[0-9]*)?\.html\.in|.*\.patch))|\.(po|fig|gif|ico|png))$$ + \.(po|fig|gif|ico|png|pot|pl|spec|spec\.in|js|woff|diff|patch|html\.in|stp|syms|conf|data|cpuinfo)$$
.spec is not in the repo, so it's never checked. And then, we perhaps want to check .spec.in? For instance for space at EOF.
.spec is generated from .spec.in, so for a builddir==srcdir build, syntax-check will find both; hence, IMHO both ought to be either skipped or checked, but together. Jirka, you are the one working on the packaging side: what do you think about the changes above (at least from the .spec point of view)? Thanks, -- Pino Toscano
On 10/06/2017 02:56 PM, Pino Toscano wrote:
On Friday, 6 October 2017 12:32:28 CEST Michal Privoznik wrote:
On 10/05/2017 01:07 PM, Pino Toscano wrote:
The majority of the syntax check is taylored for C sources, so some of the checks already cause false positives for non-C sources (and thus there are exclusion regexps in place).
Instead, just exclude more non-C files from all the checks: - pot files: they are templates for po files (already excluded), and they are automatically generated from sources - pl files: Perl sources, which have own APIs, style, etc; they are helper scripts, not "real" sources - spec/spec.in files: RPM packaging files - js/woff/html.in files: files for web pages - diff/patch files: patches - stp files: SystemTap scripts - syms files: linker symbols files - conf files: generic configuration files - data/cpuinfo files: procinfo/cpuinfo files
Python files (.py) are left allowed, since there is at least one syntax check specifically for them.
Signed-off-by: Pino Toscano <ptoscano@redhat.com> --- cfg.mk | 29 +++++++++++++---------------- 1 file changed, 13 insertions(+), 16 deletions(-)
diff --git a/cfg.mk b/cfg.mk index 0f4065b98..44a19594e 100644 --- a/cfg.mk +++ b/cfg.mk @@ -91,7 +91,7 @@ endif
# Files that should never cause syntax check failures. VC_LIST_ALWAYS_EXCLUDE_REGEX = \ - (^(docs/(news(-[0-9]*)?\.html\.in|.*\.patch))|\.(po|fig|gif|ico|png))$$ + \.(po|fig|gif|ico|png|pot|pl|spec|spec\.in|js|woff|diff|patch|html\.in|stp|syms|conf|data|cpuinfo)$$
.spec is not in the repo, so it's never checked. And then, we perhaps want to check .spec.in? For instance for space at EOF.
.spec is generated from .spec.in, so for a builddir==srcdir build, syntax-check will find both; hence, IMHO both ought to be either skipped or checked, but together.
Is that right? I always thought that syntax-check runs only over the files that are in the git and not generated ones: libvirt.git $ git clean -fxd; mkdir _build && cd _build && \ ../autogen.sh --prefix=${PWD} && echo "" >> libvirt.spec && \ make syntax-check ; echo $? 0 Whereas: libvirt.git $ git clean -fxd; mkdir _build && cd _build && \ ../autogen.sh --prefix=${PWD} && echo "" >> ../libvirt.spec.in && \ make syntax-check ; echo $? ../libvirt.spec.in maint.mk: empty line(s) or no newline at EOF make: *** [../maint.mk:929: sc_prohibit_empty_lines_at_EOF] Error 1 2 Michal
On 10/06/2017 08:23 AM, Michal Privoznik wrote:
.spec is not in the repo, so it's never checked. And then, we perhaps want to check .spec.in? For instance for space at EOF.
.spec is generated from .spec.in, so for a builddir==srcdir build, syntax-check will find both; hence, IMHO both ought to be either skipped or checked, but together.
Is that right? I always thought that syntax-check runs only over the files that are in the git and not generated ones:
Many checks run only on checked-in files, but some checks also run on in-tree files. It depends whether the check was written with in_vc_files, in_files, or something else. In fact, many builtin rules in maint.mk use: in_vc_files='\.[chly]$$' to limit to C-like files; I think part of the problem is that there are syntax-check rules that forget to use this limiting filter, and end up getting applied to too many files. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
On 10/05/2017 06:07 AM, Pino Toscano wrote:
The majority of the syntax check is taylored for C sources, so some of the checks already cause false positives for non-C sources (and thus there are exclusion regexps in place).
Instead, just exclude more non-C files from all the checks: - pot files: they are templates for po files (already excluded), and they are automatically generated from sources - pl files: Perl sources, which have own APIs, style, etc; they are helper scripts, not "real" sources - spec/spec.in files: RPM packaging files - js/woff/html.in files: files for web pages - diff/patch files: patches - stp files: SystemTap scripts - syms files: linker symbols files - conf files: generic configuration files - data/cpuinfo files: procinfo/cpuinfo files
There are still some useful syntax checks for performing on ALL files (for example, prohibit_doubled_word). So I'm not quite sure that blindly exempting these files from all possible checks makes sense. Maybe it's worth teaching upstream gnulib syntax-check to make it easier to auto-exclude non-C files from checks that ARE specific to the C language, without weakening the global checks that are good on all files. Maybe even something as simple as adding some sort of language= tag to feed to $(_sc_search_regexp (if not specified, run on all files, but if specified as C, the syntax-check is specific to C-like files, so it limits to .h, .c. .y). I'm adding the gnulib list to get feedback on the idea; maybe Jim Meyering has an opinion as one of the original syntax-check authors.
Python files (.py) are left allowed, since there is at least one syntax check specifically for them.
Signed-off-by: Pino Toscano <ptoscano@redhat.com> --- cfg.mk | 29 +++++++++++++---------------- 1 file changed, 13 insertions(+), 16 deletions(-)
diff --git a/cfg.mk b/cfg.mk index 0f4065b98..44a19594e 100644 --- a/cfg.mk +++ b/cfg.mk @@ -91,7 +91,7 @@ endif
# Files that should never cause syntax check failures. VC_LIST_ALWAYS_EXCLUDE_REGEX = \ - (^(docs/(news(-[0-9]*)?\.html\.in|.*\.patch))|\.(po|fig|gif|ico|png))$$ + \.(po|fig|gif|ico|png|pot|pl|spec|spec\.in|js|woff|diff|patch|html\.in|stp|syms|conf|data|cpuinfo)$$
-- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
On Fri, Oct 6, 2017 at 7:47 AM, Eric Blake <eblake@redhat.com> wrote:
On 10/05/2017 06:07 AM, Pino Toscano wrote:
The majority of the syntax check is taylored for C sources, so some of the checks already cause false positives for non-C sources (and thus there are exclusion regexps in place).
Instead, just exclude more non-C files from all the checks: - pot files: they are templates for po files (already excluded), and they are automatically generated from sources - pl files: Perl sources, which have own APIs, style, etc; they are helper scripts, not "real" sources - spec/spec.in files: RPM packaging files - js/woff/html.in files: files for web pages - diff/patch files: patches - stp files: SystemTap scripts - syms files: linker symbols files - conf files: generic configuration files - data/cpuinfo files: procinfo/cpuinfo files
There are still some useful syntax checks for performing on ALL files (for example, prohibit_doubled_word). So I'm not quite sure that blindly exempting these files from all possible checks makes sense.
Maybe it's worth teaching upstream gnulib syntax-check to make it easier to auto-exclude non-C files from checks that ARE specific to the C language, without weakening the global checks that are good on all files. Maybe even something as simple as adding some sort of language= tag to feed to $(_sc_search_regexp (if not specified, run on all files, but if specified as C, the syntax-check is specific to C-like files, so it limits to .h, .c. .y).
I'm adding the gnulib list to get feedback on the idea; maybe Jim Meyering has an opinion as one of the original syntax-check authors.
Hi Eric, Is there some reason not to use a directive like this in a rule applying exclusively to version-controlled C-like files? in_vc_files='\.[chly]$$' I looked at libvirt's cfg.mk, and if you add that line to the sc_prohibit_sprintf rule, you can then remove the lines that exempt files with unrelated suffixes from that rule: exclude_file_name_regexp--sc_prohibit_sprintf = \ ^(cfg\.mk|docs/hacking\.html\.in|.*\.stp|.*\.pl)$$ Another rule that can catch things in any non-binary file is sc_prohibit_undesirable_word_seq, even if it's only for pet peeves like detecting "can not". ...
# Files that should never cause syntax check failures. VC_LIST_ALWAYS_EXCLUDE_REGEX = \ - (^(docs/(news(-[0-9]*)?\.html\.in|.*\.patch))|\.(po|fig|gif|ico|png))$$ + \.(po|fig|gif|ico|png|pot|pl|spec|spec\.in|js|woff|diff|patch|html\.in|stp|syms|conf|data|cpuinfo)$$
On 10/06/2017 12:07 PM, Jim Meyering wrote:
On Fri, Oct 6, 2017 at 7:47 AM, Eric Blake <eblake@redhat.com> wrote:
On 10/05/2017 06:07 AM, Pino Toscano wrote:
The majority of the syntax check is taylored for C sources, so some of the checks already cause false positives for non-C sources (and thus there are exclusion regexps in place).
Instead, just exclude more non-C files from all the checks:
Maybe it's worth teaching upstream gnulib syntax-check to make it easier to auto-exclude non-C files from checks that ARE specific to the C language, without weakening the global checks that are good on all files. Maybe even something as simple as adding some sort of language= tag to feed to $(_sc_search_regexp (if not specified, run on all files, but if specified as C, the syntax-check is specific to C-like files, so it limits to .h, .c. .y).
I'm adding the gnulib list to get feedback on the idea; maybe Jim Meyering has an opinion as one of the original syntax-check authors.
Hi Eric, Is there some reason not to use a directive like this in a rule applying exclusively to version-controlled C-like files?
in_vc_files='\.[chly]$$'
So we already have the mechanism - we just have to use it in the right places ;) Some of the existing maint.mk rules don't use it (for a quick example, sc_cast_of_x_alloc_return_value, sc_cast_of_alloca_return_value) - but it may also be a question of how often these rules are firing on non-C files to let us know that we even have false positives.
I looked at libvirt's cfg.mk, and if you add that line to the sc_prohibit_sprintf rule, you can then remove the lines that exempt files with unrelated suffixes from that rule:
Indeed, that's probably the best fix for libvirt: for any rule in cfg.mk that we want to run only on C files, use in_vc_files= to make it obvious.
exclude_file_name_regexp--sc_prohibit_sprintf = \ ^(cfg\.mk|docs/hacking\.html\.in|.*\.stp|.*\.pl)$$
Another rule that can catch things in any non-binary file is sc_prohibit_undesirable_word_seq, even if it's only for pet peeves like detecting "can not".
And sc_space_tab is globally useful, etc.
...
# Files that should never cause syntax check failures. VC_LIST_ALWAYS_EXCLUDE_REGEX = \ - (^(docs/(news(-[0-9]*)?\.html\.in|.*\.patch))|\.(po|fig|gif|ico|png))$$ + \.(po|fig|gif|ico|png|pot|pl|spec|spec\.in|js|woff|diff|patch|html\.in|stp|syms|conf|data|cpuinfo)$$
There ARE some files that are always worth excluding: the list of graphic binary files (such as .png) makes obvious sense. But it's a high bar to globally exclude a file from all syntax checks, especially when it's relatively easy to write specific checks to be language specific. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
participants (4)
-
Eric Blake -
Jim Meyering -
Michal Privoznik -
Pino Toscano