[PATCH v2 0/2] Using sparse in a CI job

[PATCH v2 0/2] Using sparse in a CI job

[Ramsay Jones]

Changes in v2:

  - Add a new patch #1, which removes an obsolete use of SPARSE_FLAGS
  - Actually initialise the new SP_EXTRA_FLAGS variable
  - Reword the commit message (now #2) to, hopefully, clarify the
    usage of SPARSE_FLAGS and SP_EXTRA_FLAGS

A range-diff against v1 is given at the very end.

Original cover-letter content:

I suspect that the Makefile sparse target is not easy to use in a CI
job, since the 'sparse' program (via cgcc -no-compile) does not exit
with a non-zero value, even when issuing errors and warnings.

The way I use sparse, this is not an issue. I run sparse over the
master branch, check that the output file has the no/expected
errors and warnings, then just diff the output files from the
next and pu branches. something like:

  $ make sparse >sp-out 2>&1 # on master branch
  $ vim sp-out # check for errors/warnings
  ...
  $ make sparse >nsp-out 2>&1 # on next branch
  $ diff sp-out nsp-out # any new errors/warnings?
  ...
  $ make sparse >psp-out 2>&1 # on pu branch
  $ diff nsp-out psp-out # any new errors/warnings?

At the moment, on Linux, the sp-out file is free from any sparse errors
or warnings. So are next and pu:

  $ grep error sp-out
  $ grep warning sp-out
  $ diff sp-out nsp-out
  391a392
  >     SP fuzz-commit-graph.c
  $ diff nsp-out psp-out
  170a171,180
  >     SP trace2.c
  >     SP trace2/tr2_cfg.c
  >     SP trace2/tr2_dst.c
  >     SP trace2/tr2_sid.c
  >     SP trace2/tr2_tbuf.c
  >     SP trace2/tr2_tgt_event.c
  >     SP trace2/tr2_tgt_normal.c
  >     SP trace2/tr2_tgt_perf.c
  >     SP trace2/tr2_tls.c
  >     SP trace2/tr2_verb.c
  298a309
  >     SP builtin/stash.c
  375a387
  >     SP t/helper/test-trace2.c
  376a389
  >     SP t/helper/test-xml-encode.c
  $ 

However, if we go back a few days, then the pu branch was not clean:

  $ diff nsp-out psp-out
  18a20,24
  >     SP change-table.c
  > change-table.h:53:24: error: dubious one-bit signed bitfield
  > change-table.h:54:25: error: dubious one-bit signed bitfield
  > change-table.h:55:25: error: dubious one-bit signed bitfield
  > change-table.h:56:26: error: dubious one-bit signed bitfield
  69a76
  >     GEN command-list.h
  93a101,106
  >     SP metacommit.c
  > change-table.h:53:24: error: dubious one-bit signed bitfield
  > change-table.h:54:25: error: dubious one-bit signed bitfield
  > change-table.h:55:25: error: dubious one-bit signed bitfield
  > change-table.h:56:26: error: dubious one-bit signed bitfield
  >     SP metacommit-parser.c
  170a184,193
  >     SP trace2.c
  >     SP trace2/tr2_cfg.c
  >     SP trace2/tr2_dst.c
  >     SP trace2/tr2_sid.c
  >     SP trace2/tr2_tbuf.c
  >     SP trace2/tr2_tgt_event.c
  >     SP trace2/tr2_tgt_normal.c
  >     SP trace2/tr2_tgt_perf.c
  >     SP trace2/tr2_tls.c
  >     SP trace2/tr2_verb.c
  213a237
  >     SP builtin/change.c
  298a323
  >     SP builtin/stash.c
  375a401
  >     SP t/helper/test-trace2.c
  376a403,405
  >     SP t/helper/test-xml-encode.c
  > t/helper/test-xml-encode.c:29:40: warning: Using plain integer as NULL pointer
  > t/helper/test-xml-encode.c:37:40: warning: Using plain integer as NULL pointer
  $ 
 
Note that 'make' does not exit at the first 'error', since the command exit
code is zero (or not non-zero! :) ):
 
  $ make change-table.sp
      SP change-table.c
  change-table.h:53:24: error: dubious one-bit signed bitfield
  change-table.h:54:25: error: dubious one-bit signed bitfield
  change-table.h:55:25: error: dubious one-bit signed bitfield
  change-table.h:56:26: error: dubious one-bit signed bitfield
  $ echo $?
  0
  $ 

We can change that by passing '-Wsparse-error' to 'sparse':

  $ make SPARSE_FLAGS=-Wsparse-error change-table.sp
      SP change-table.c
  change-table.h:53:24: error: dubious one-bit signed bitfield
  change-table.h:54:25: error: dubious one-bit signed bitfield
  change-table.h:55:25: error: dubious one-bit signed bitfield
  change-table.h:56:26: error: dubious one-bit signed bitfield
  Makefile:2729: recipe for target 'change-table.sp' failed
  make: *** [change-table.sp] Error 1
  $ echo $?
  2
  $ 

Note that '-Wsparse-error' not only returns a non-zero exit code (1), but
it also changes a 'warning' into an 'error' (see above):
  
  $ make SPARSE_FLAGS=-Wsparse-error t/helper/test-xml-encode.sp
      SP t/helper/test-xml-encode.c
  t/helper/test-xml-encode.c:29:40: error: Using plain integer as NULL pointer
  t/helper/test-xml-encode.c:37:40: error: Using plain integer as NULL pointer
  Makefile:2729: recipe for target 't/helper/test-xml-encode.sp' failed
  make: *** [t/helper/test-xml-encode.sp] Error 1
  $ echo $?
  2
  $ 

Unfortunately, using SPARSE_FLAGS from the 'make' command-line is not
ideal, since it was not really intended to be used that way. This will
cause problems for those files which have target specific settings for
SPARSE_FLAGS. For example:

  $ make pack-revindex.sp
      SP pack-revindex.c
  $ make SPARSE_FLAGS=-Wsparse-error pack-revindex.sp
      SP pack-revindex.c
  pack-revindex.c:65:23: error: memset with byte count of 262144
  Makefile:2729: recipe for target 'pack-revindex.sp' failed
  make: *** [pack-revindex.sp] Error 1
  $ echo $?
  2
  $ 

So, passing SPARSE_FLAGS on the command-line, effectively nullifies the
target specific settings (making them useless).

This patch splits the duties of SPARSE_FLAGS, by introducing a separate
SP_EXTRA_FLAGS variable, for use with the target specific settings. In
addition, we use a conditional assignment (?=) of the default (empty)
value of SPARSE_FLAGS, to allow setting the value of this variable from
the environment as well as the command-line. So, after this patch:

  $ make SPARSE_FLAGS=-Wsparse-error pack-revindex.sp
      SP pack-revindex.c
  $ echo $?
  0
  $ 

  $ SPARSE_FLAGS=-Wsparse-error make pack-revindex.sp
      SP pack-revindex.c
  $ echo $?
  0
  $ 

Now, we should be able to run the sparse Makefile target in a CI job, and
still find all sparse errors and warnings (now marked as errors also),
using something like this:

  $ SPARSE_FLAGS=-Wsparse-error make -k sparse >sp-out 2>&1

Note that the '-k' argument to 'make' is now required. ;-)


ATB,
Ramsay Jones 


Ramsay Jones (2):
  config.mak.uname: remove obsolete SPARSE_FLAGS setting
  Makefile: improve SPARSE_FLAGS customisation

 Makefile         | 14 +++++++++-----
 config.mak.uname |  1 -
 2 files changed, 9 insertions(+), 6 deletions(-)

Range-diff against v1:
-:  ---------- > 1:  7b15bd9b31 config.mak.uname: remove obsolete SPARSE_FLAGS setting
1:  889cc64f90 ! 2:  c2b6ce71da Makefile: improve SPARSE_FLAGS customisation
    @@ -7,10 +7,13 @@
         target specific settings. Without using the new variable, setting
         the SPARSE_FLAGS on the 'make' command-line would also override the
         value set by the target-specific rules in the Makefile (effectively
    -    making them useless). In addition, we initialise the SPARSE_FLAGS
    -    to the default (empty) value using a conditional assignment (?=).
    -    This allows the SPARSE_FLAGS to be set from the environment as
    -    well as from the command-line.
    +    making them useless). Also, this enables the SP_EXTRA_FLAGS to be
    +    used in the future for any other internal customisations, such as
    +    for some platform specific values.
    +
    +    In addition, we initialise the SPARSE_FLAGS to the default (empty)
    +    value using a conditional assignment (?=). This allows SPARSE_FLAGS
    +    to be set from the environment as well as from the command-line.
     
      diff --git a/Makefile b/Makefile
      --- a/Makefile
    @@ -20,7 +23,11 @@
      export TCL_PATH TCLTK_PATH
      
     -SPARSE_FLAGS =
    ++# user customisation variable for 'sparse' target
     +SPARSE_FLAGS ?=
    ++# internal/platform customisation variable for 'sparse'
    ++SP_EXTRA_FLAGS =
    ++
      SPATCH_FLAGS = --all-includes --patch .
      
      
-- 
2.20.0

Re: [PATCH v2 0/2] Using sparse in a CI job

[Junio C Hamano]

Ramsay Jones <ramsay@ramsayjones.plus.com> writes:

> Changes in v2:
>
>   - Add a new patch #1, which removes an obsolete use of SPARSE_FLAGS
>   - Actually initialise the new SP_EXTRA_FLAGS variable
>   - Reword the commit message (now #2) to, hopefully, clarify the
>     usage of SPARSE_FLAGS and SP_EXTRA_FLAGS

Thanks.  Applied.