6:07 a.m.
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(a)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
5:32 a.m.
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(a)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
7:56 a.m.
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(a)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
8:23 a.m.
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(a)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
9:58 a.m.
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
9:47 a.m.
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(a)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
12:07 p.m.
On Fri, Oct 6, 2017 at 7:47 AM, Eric Blake <eblake(a)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)$$
12:57 p.m.
On 10/06/2017 12:07 PM, Jim Meyering wrote:
On Fri, Oct 6, 2017 at 7:47 AM, Eric Blake <eblake(a)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
2604
days inactive
2605
days old
7 comments
4 participants
participants (4)
-
Eric Blake -
Jim Meyering -
Michal Privoznik -
Pino Toscano