| Message ID | 0dbef17c-be73-1d88-cd20-83f3123361bf@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | build: correct cppcheck-misra make rule | expand |
Hi Jan, > On 9 Sep 2022, at 14:41, Jan Beulich <jbeulich@suse.com> wrote: > > It has been bothering me for a while that I made a bad suggestion during This is not a sentence for a commit message. > review: Having cppcheck-misra.json depend on cppcheck-misra.txt does not > properly address the multiple targets problem. If cppcheck-misra.json > is deleted from the build tree but cppcheck-misra.txt is still there, > nothing will re-generate cppcheck-misra.json. > > With GNU make 4.3 or newer we could use the &: grouped target separator, > but since we support older make as well we need to use some other > mechanism. Convert the rule to a pattern one (with "cppcheck" > arbitrarily chosen as the stem), thus making known to make that both > files are created by a single command invocation. Since, as a result, > the JSON file is now "intermediate" from make's perspective, prevent it > being deleted again by making it a prereq of .PRECIOUS. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > I've not been able to spot where / how cppcheck-misra.txt is used. If > it's indeed unused, a perhaps better alternative would be to convert the > original rule to specify cppcheck-misra.json as (the only) target. One > might then even consider using "-o /dev/null" instead of producing an > unused *.txt file. Txt file is used by cppcheck to give a text description of the rule. If you look inside the json content you will see it mentioned. > > --- a/xen/Makefile > +++ b/xen/Makefile > @@ -746,11 +746,9 @@ cppcheck-version: > # documentation file. Also generate a json file with the right arguments for > # cppcheck in json format including the list of rules to ignore. > # > -cppcheck-misra.txt: $(XEN_ROOT)/docs/misra/rules.rst $(srctree)/tools/convert_misra_doc.py > - $(Q)$(PYTHON) $(srctree)/tools/convert_misra_doc.py -i $< -o $@ -j $(@:.txt=.json) > - > -# convert_misra_doc is generating both files. > -cppcheck-misra.json: cppcheck-misra.txt > +.PRECIOUS: %-misra.json > +%-misra.txt %-misra.json: $(XEN_ROOT)/docs/misra/rules.rst $(srctree)/tools/convert_misra_doc.py > + $(Q)$(PYTHON) $(srctree)/tools/convert_misra_doc.py -i $< -o $*-misra.txt -j $*-misra.json As far as I know, this is not saying to make that both files are generated by this rule, but that this rule can generate both files so nothing is telling make here that calling it once is enough I think. Anyway this should work but the commit message needs some rephrasing and I cannot test this right now. Bertrand > > # Put this in generated headers this way it is cleaned by include/Makefile > $(objtree)/include/generated/compiler-def.h:
On Fri, Sep 09, 2022 at 01:50:38PM +0000, Bertrand Marquis wrote: > > On 9 Sep 2022, at 14:41, Jan Beulich <jbeulich@suse.com> wrote: > > --- a/xen/Makefile > > +++ b/xen/Makefile > > @@ -746,11 +746,9 @@ cppcheck-version: > > # documentation file. Also generate a json file with the right arguments for > > # cppcheck in json format including the list of rules to ignore. > > # > > -cppcheck-misra.txt: $(XEN_ROOT)/docs/misra/rules.rst $(srctree)/tools/convert_misra_doc.py > > - $(Q)$(PYTHON) $(srctree)/tools/convert_misra_doc.py -i $< -o $@ -j $(@:.txt=.json) > > - > > -# convert_misra_doc is generating both files. > > -cppcheck-misra.json: cppcheck-misra.txt > > +.PRECIOUS: %-misra.json > > +%-misra.txt %-misra.json: $(XEN_ROOT)/docs/misra/rules.rst $(srctree)/tools/convert_misra_doc.py > > + $(Q)$(PYTHON) $(srctree)/tools/convert_misra_doc.py -i $< -o $*-misra.txt -j $*-misra.json > > As far as I know, this is not saying to make that both files are generated by this rule, > but that this rule can generate both files so nothing is telling make here that calling > it once is enough I think. A comment could be added, the same one as the one used for syncconfig: # This exploits the 'multi-target pattern rule' trick. # convert_misra_doc.py should be executed only once to make all the targets. Cheers,
On 09.09.2022 15:50, Bertrand Marquis wrote: >> On 9 Sep 2022, at 14:41, Jan Beulich <jbeulich@suse.com> wrote: >> >> It has been bothering me for a while that I made a bad suggestion during > > This is not a sentence for a commit message. How else should I express the motivation for the change? >> review: Having cppcheck-misra.json depend on cppcheck-misra.txt does not >> properly address the multiple targets problem. If cppcheck-misra.json >> is deleted from the build tree but cppcheck-misra.txt is still there, >> nothing will re-generate cppcheck-misra.json. >> >> With GNU make 4.3 or newer we could use the &: grouped target separator, >> but since we support older make as well we need to use some other >> mechanism. Convert the rule to a pattern one (with "cppcheck" >> arbitrarily chosen as the stem), thus making known to make that both >> files are created by a single command invocation. Since, as a result, >> the JSON file is now "intermediate" from make's perspective, prevent it >> being deleted again by making it a prereq of .PRECIOUS. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> --- >> I've not been able to spot where / how cppcheck-misra.txt is used. If >> it's indeed unused, a perhaps better alternative would be to convert the >> original rule to specify cppcheck-misra.json as (the only) target. One >> might then even consider using "-o /dev/null" instead of producing an >> unused *.txt file. > > Txt file is used by cppcheck to give a text description of the rule. > If you look inside the json content you will see it mentioned. Oh, that's properly hidden then. >> --- a/xen/Makefile >> +++ b/xen/Makefile >> @@ -746,11 +746,9 @@ cppcheck-version: >> # documentation file. Also generate a json file with the right arguments for >> # cppcheck in json format including the list of rules to ignore. >> # >> -cppcheck-misra.txt: $(XEN_ROOT)/docs/misra/rules.rst $(srctree)/tools/convert_misra_doc.py >> - $(Q)$(PYTHON) $(srctree)/tools/convert_misra_doc.py -i $< -o $@ -j $(@:.txt=.json) >> - >> -# convert_misra_doc is generating both files. >> -cppcheck-misra.json: cppcheck-misra.txt >> +.PRECIOUS: %-misra.json >> +%-misra.txt %-misra.json: $(XEN_ROOT)/docs/misra/rules.rst $(srctree)/tools/convert_misra_doc.py >> + $(Q)$(PYTHON) $(srctree)/tools/convert_misra_doc.py -i $< -o $*-misra.txt -j $*-misra.json > > As far as I know, this is not saying to make that both files are generated by this rule, > but that this rule can generate both files so nothing is telling make here that calling > it once is enough I think. As said in the description - it specifically has this effect. We're using this elsewhere already, see e.g. tools/libs/light/Makefile generating three headers and a C file all in one go. Iirc this is also explicitly described in make documentation (and contrasted to the different behavior for non-pattern rules). Jan
Hi Jan, > On 9 Sep 2022, at 15:26, Jan Beulich <jbeulich@suse.com> wrote: > > On 09.09.2022 15:50, Bertrand Marquis wrote: >>> On 9 Sep 2022, at 14:41, Jan Beulich <jbeulich@suse.com> wrote: >>> >>> It has been bothering me for a while that I made a bad suggestion during >> >> This is not a sentence for a commit message. > > How else should I express the motivation for the change? I would say, start with “cppcheck-misra.json depend on …” and remove everything before. Being bothered is not really something interesting to read in the git log. > >>> review: Having cppcheck-misra.json depend on cppcheck-misra.txt does not >>> properly address the multiple targets problem. If cppcheck-misra.json >>> is deleted from the build tree but cppcheck-misra.txt is still there, >>> nothing will re-generate cppcheck-misra.json. >>> >>> With GNU make 4.3 or newer we could use the &: grouped target separator, >>> but since we support older make as well we need to use some other >>> mechanism. Convert the rule to a pattern one (with "cppcheck" >>> arbitrarily chosen as the stem), thus making known to make that both >>> files are created by a single command invocation. Since, as a result, >>> the JSON file is now "intermediate" from make's perspective, prevent it >>> being deleted again by making it a prereq of .PRECIOUS. >>> >>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>> --- >>> I've not been able to spot where / how cppcheck-misra.txt is used. If >>> it's indeed unused, a perhaps better alternative would be to convert the >>> original rule to specify cppcheck-misra.json as (the only) target. One >>> might then even consider using "-o /dev/null" instead of producing an >>> unused *.txt file. >> >> Txt file is used by cppcheck to give a text description of the rule. >> If you look inside the json content you will see it mentioned. > > Oh, that's properly hidden then. This is how cppcheck needs it and why I added a comment but it might needs improving. > >>> --- a/xen/Makefile >>> +++ b/xen/Makefile >>> @@ -746,11 +746,9 @@ cppcheck-version: >>> # documentation file. Also generate a json file with the right arguments for >>> # cppcheck in json format including the list of rules to ignore. >>> # >>> -cppcheck-misra.txt: $(XEN_ROOT)/docs/misra/rules.rst $(srctree)/tools/convert_misra_doc.py >>> - $(Q)$(PYTHON) $(srctree)/tools/convert_misra_doc.py -i $< -o $@ -j $(@:.txt=.json) >>> - >>> -# convert_misra_doc is generating both files. >>> -cppcheck-misra.json: cppcheck-misra.txt >>> +.PRECIOUS: %-misra.json >>> +%-misra.txt %-misra.json: $(XEN_ROOT)/docs/misra/rules.rst $(srctree)/tools/convert_misra_doc.py >>> + $(Q)$(PYTHON) $(srctree)/tools/convert_misra_doc.py -i $< -o $*-misra.txt -j $*-misra.json >> >> As far as I know, this is not saying to make that both files are generated by this rule, >> but that this rule can generate both files so nothing is telling make here that calling >> it once is enough I think. > > As said in the description - it specifically has this effect. We're > using this elsewhere already, see e.g. tools/libs/light/Makefile > generating three headers and a C file all in one go. Iirc this is > also explicitly described in make documentation (and contrasted to > the different behavior for non-pattern rules). Then I think the comment suggested by Anthony makes sense to add. Cheers Bertrand > > Jan
--- a/xen/Makefile +++ b/xen/Makefile @@ -746,11 +746,9 @@ cppcheck-version: # documentation file. Also generate a json file with the right arguments for # cppcheck in json format including the list of rules to ignore. # -cppcheck-misra.txt: $(XEN_ROOT)/docs/misra/rules.rst $(srctree)/tools/convert_misra_doc.py - $(Q)$(PYTHON) $(srctree)/tools/convert_misra_doc.py -i $< -o $@ -j $(@:.txt=.json) - -# convert_misra_doc is generating both files. -cppcheck-misra.json: cppcheck-misra.txt +.PRECIOUS: %-misra.json +%-misra.txt %-misra.json: $(XEN_ROOT)/docs/misra/rules.rst $(srctree)/tools/convert_misra_doc.py + $(Q)$(PYTHON) $(srctree)/tools/convert_misra_doc.py -i $< -o $*-misra.txt -j $*-misra.json # Put this in generated headers this way it is cleaned by include/Makefile $(objtree)/include/generated/compiler-def.h:
It has been bothering me for a while that I made a bad suggestion during review: Having cppcheck-misra.json depend on cppcheck-misra.txt does not properly address the multiple targets problem. If cppcheck-misra.json is deleted from the build tree but cppcheck-misra.txt is still there, nothing will re-generate cppcheck-misra.json. With GNU make 4.3 or newer we could use the &: grouped target separator, but since we support older make as well we need to use some other mechanism. Convert the rule to a pattern one (with "cppcheck" arbitrarily chosen as the stem), thus making known to make that both files are created by a single command invocation. Since, as a result, the JSON file is now "intermediate" from make's perspective, prevent it being deleted again by making it a prereq of .PRECIOUS. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- I've not been able to spot where / how cppcheck-misra.txt is used. If it's indeed unused, a perhaps better alternative would be to convert the original rule to specify cppcheck-misra.json as (the only) target. One might then even consider using "-o /dev/null" instead of producing an unused *.txt file.