{"thread":{"id":"50400","subject":"[PATCH v2 0/2] Using sparse in a CI job","startedAt":"2019-02-05T02:24:22Z","lastAt":"2019-02-05T18:08:18Z","messageCount":2,"participants":["Ramsay Jones","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":2},"messages":[{"id":"368507","messageId":"af361187-4d9d-2c41-0850-bf774d5523b2@ramsayjones.plus.com","threadId":"50400","inReplyTo":null,"subject":"[PATCH v2 0/2] Using sparse in a CI job","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2019-02-05T02:24:16Z","receivedAt":"2019-02-05T02:24:22Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\nChanges in v2:\n\n  - Add a new patch #1, which removes an obsolete use of SPARSE_FLAGS\n  - Actually initialise the new SP_EXTRA_FLAGS variable\n  - Reword the commit message (now #2) to, hopefully, clarify the\n    usage of SPARSE_FLAGS and SP_EXTRA_FLAGS\n\nA range-diff against v1 is given at the very end.\n\nOriginal cover-letter content:\n\nI suspect that the Makefile sparse target is not easy to use in a CI\njob, since the 'sparse' program (via cgcc -no-compile) does not exit\nwith a non-zero value, even when issuing errors and warnings.\n\nThe way I use sparse, this is not an issue. I run sparse over the\nmaster branch, check that the output file has the no/expected\nerrors and warnings, then just diff the output files from the\nnext and pu branches. something like:\n\n  $ make sparse >sp-out 2>&1 # on master branch\n  $ vim sp-out # check for errors/warnings\n  ...\n  $ make sparse >nsp-out 2>&1 # on next branch\n  $ diff sp-out nsp-out # any new errors/warnings?\n  ...\n  $ make sparse >psp-out 2>&1 # on pu branch\n  $ diff nsp-out psp-out # any new errors/warnings?\n\nAt the moment, on Linux, the sp-out file is free from any sparse errors\nor warnings. So are next and pu:\n\n  $ grep error sp-out\n  $ grep warning sp-out\n  $ diff sp-out nsp-out\n  391a392\n  >     SP fuzz-commit-graph.c\n  $ diff nsp-out psp-out\n  170a171,180\n  >     SP trace2.c\n  >     SP trace2/tr2_cfg.c\n  >     SP trace2/tr2_dst.c\n  >     SP trace2/tr2_sid.c\n  >     SP trace2/tr2_tbuf.c\n  >     SP trace2/tr2_tgt_event.c\n  >     SP trace2/tr2_tgt_normal.c\n  >     SP trace2/tr2_tgt_perf.c\n  >     SP trace2/tr2_tls.c\n  >     SP trace2/tr2_verb.c\n  298a309\n  >     SP builtin/stash.c\n  375a387\n  >     SP t/helper/test-trace2.c\n  376a389\n  >     SP t/helper/test-xml-encode.c\n  $ \n\nHowever, if we go back a few days, then the pu branch was not clean:\n\n  $ diff nsp-out psp-out\n  18a20,24\n  >     SP change-table.c\n  > change-table.h:53:24: error: dubious one-bit signed bitfield\n  > change-table.h:54:25: error: dubious one-bit signed bitfield\n  > change-table.h:55:25: error: dubious one-bit signed bitfield\n  > change-table.h:56:26: error: dubious one-bit signed bitfield\n  69a76\n  >     GEN command-list.h\n  93a101,106\n  >     SP metacommit.c\n  > change-table.h:53:24: error: dubious one-bit signed bitfield\n  > change-table.h:54:25: error: dubious one-bit signed bitfield\n  > change-table.h:55:25: error: dubious one-bit signed bitfield\n  > change-table.h:56:26: error: dubious one-bit signed bitfield\n  >     SP metacommit-parser.c\n  170a184,193\n  >     SP trace2.c\n  >     SP trace2/tr2_cfg.c\n  >     SP trace2/tr2_dst.c\n  >     SP trace2/tr2_sid.c\n  >     SP trace2/tr2_tbuf.c\n  >     SP trace2/tr2_tgt_event.c\n  >     SP trace2/tr2_tgt_normal.c\n  >     SP trace2/tr2_tgt_perf.c\n  >     SP trace2/tr2_tls.c\n  >     SP trace2/tr2_verb.c\n  213a237\n  >     SP builtin/change.c\n  298a323\n  >     SP builtin/stash.c\n  375a401\n  >     SP t/helper/test-trace2.c\n  376a403,405\n  >     SP t/helper/test-xml-encode.c\n  > t/helper/test-xml-encode.c:29:40: warning: Using plain integer as NULL pointer\n  > t/helper/test-xml-encode.c:37:40: warning: Using plain integer as NULL pointer\n  $ \n \nNote that 'make' does not exit at the first 'error', since the command exit\ncode is zero (or not non-zero! :) ):\n \n  $ make change-table.sp\n      SP change-table.c\n  change-table.h:53:24: error: dubious one-bit signed bitfield\n  change-table.h:54:25: error: dubious one-bit signed bitfield\n  change-table.h:55:25: error: dubious one-bit signed bitfield\n  change-table.h:56:26: error: dubious one-bit signed bitfield\n  $ echo $?\n  0\n  $ \n\nWe can change that by passing '-Wsparse-error' to 'sparse':\n\n  $ make SPARSE_FLAGS=-Wsparse-error change-table.sp\n      SP change-table.c\n  change-table.h:53:24: error: dubious one-bit signed bitfield\n  change-table.h:54:25: error: dubious one-bit signed bitfield\n  change-table.h:55:25: error: dubious one-bit signed bitfield\n  change-table.h:56:26: error: dubious one-bit signed bitfield\n  Makefile:2729: recipe for target 'change-table.sp' failed\n  make: *** [change-table.sp] Error 1\n  $ echo $?\n  2\n  $ \n\nNote that '-Wsparse-error' not only returns a non-zero exit code (1), but\nit also changes a 'warning' into an 'error' (see above):\n  \n  $ make SPARSE_FLAGS=-Wsparse-error t/helper/test-xml-encode.sp\n      SP t/helper/test-xml-encode.c\n  t/helper/test-xml-encode.c:29:40: error: Using plain integer as NULL pointer\n  t/helper/test-xml-encode.c:37:40: error: Using plain integer as NULL pointer\n  Makefile:2729: recipe for target 't/helper/test-xml-encode.sp' failed\n  make: *** [t/helper/test-xml-encode.sp] Error 1\n  $ echo $?\n  2\n  $ \n\nUnfortunately, using SPARSE_FLAGS from the 'make' command-line is not\nideal, since it was not really intended to be used that way. This will\ncause problems for those files which have target specific settings for\nSPARSE_FLAGS. For example:\n\n  $ make pack-revindex.sp\n      SP pack-revindex.c\n  $ make SPARSE_FLAGS=-Wsparse-error pack-revindex.sp\n      SP pack-revindex.c\n  pack-revindex.c:65:23: error: memset with byte count of 262144\n  Makefile:2729: recipe for target 'pack-revindex.sp' failed\n  make: *** [pack-revindex.sp] Error 1\n  $ echo $?\n  2\n  $ \n\nSo, passing SPARSE_FLAGS on the command-line, effectively nullifies the\ntarget specific settings (making them useless).\n\nThis patch splits the duties of SPARSE_FLAGS, by introducing a separate\nSP_EXTRA_FLAGS variable, for use with the target specific settings. In\naddition, we use a conditional assignment (?=) of the default (empty)\nvalue of SPARSE_FLAGS, to allow setting the value of this variable from\nthe environment as well as the command-line. So, after this patch:\n\n  $ make SPARSE_FLAGS=-Wsparse-error pack-revindex.sp\n      SP pack-revindex.c\n  $ echo $?\n  0\n  $ \n\n  $ SPARSE_FLAGS=-Wsparse-error make pack-revindex.sp\n      SP pack-revindex.c\n  $ echo $?\n  0\n  $ \n\nNow, we should be able to run the sparse Makefile target in a CI job, and\nstill find all sparse errors and warnings (now marked as errors also),\nusing something like this:\n\n  $ SPARSE_FLAGS=-Wsparse-error make -k sparse >sp-out 2>&1\n\nNote that the '-k' argument to 'make' is now required. ;-)\n\n\nATB,\nRamsay Jones \n\n\nRamsay Jones (2):\n  config.mak.uname: remove obsolete SPARSE_FLAGS setting\n  Makefile: improve SPARSE_FLAGS customisation\n\n Makefile         | 14 +++++++++-----\n config.mak.uname |  1 -\n 2 files changed, 9 insertions(+), 6 deletions(-)\n\nRange-diff against v1:\n-:  ---------- > 1:  7b15bd9b31 config.mak.uname: remove obsolete SPARSE_FLAGS setting\n1:  889cc64f90 ! 2:  c2b6ce71da Makefile: improve SPARSE_FLAGS customisation\n    @@ -7,10 +7,13 @@\n         target specific settings. Without using the new variable, setting\n         the SPARSE_FLAGS on the 'make' command-line would also override the\n         value set by the target-specific rules in the Makefile (effectively\n    -    making them useless). In addition, we initialise the SPARSE_FLAGS\n    -    to the default (empty) value using a conditional assignment (?=).\n    -    This allows the SPARSE_FLAGS to be set from the environment as\n    -    well as from the command-line.\n    +    making them useless). Also, this enables the SP_EXTRA_FLAGS to be\n    +    used in the future for any other internal customisations, such as\n    +    for some platform specific values.\n    +\n    +    In addition, we initialise the SPARSE_FLAGS to the default (empty)\n    +    value using a conditional assignment (?=). This allows SPARSE_FLAGS\n    +    to be set from the environment as well as from the command-line.\n     \n      diff --git a/Makefile b/Makefile\n      --- a/Makefile\n    @@ -20,7 +23,11 @@\n      export TCL_PATH TCLTK_PATH\n      \n     -SPARSE_FLAGS =\n    ++# user customisation variable for 'sparse' target\n     +SPARSE_FLAGS ?=\n    ++# internal/platform customisation variable for 'sparse'\n    ++SP_EXTRA_FLAGS =\n    ++\n      SPATCH_FLAGS = --all-includes --patch .\n      \n      \n-- \n2.20.0\n"},{"id":"368535","messageId":"xmqqmunartv6.fsf@gitster-ct.c.googlers.com","threadId":"50400","inReplyTo":"af361187-4d9d-2c41-0850-bf774d5523b2@ramsayjones.plus.com","subject":"Re: [PATCH v2 0/2] Using sparse in a CI job","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-02-05T18:08:13Z","receivedAt":"2019-02-05T18:08:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramsay Jones <ramsay@ramsayjones.plus.com> writes:\n\n> Changes in v2:\n>\n>   - Add a new patch #1, which removes an obsolete use of SPARSE_FLAGS\n>   - Actually initialise the new SP_EXTRA_FLAGS variable\n>   - Reword the commit message (now #2) to, hopefully, clarify the\n>     usage of SPARSE_FLAGS and SP_EXTRA_FLAGS\n\nThanks.  Applied.\n"}]}