Re: [Xen-devel] [PATCH 6/6] xen: add cloc target

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 20.04.18 at 01:22, <sstabellini@xxxxxxxxxx> wrote:
> On Thu, 19 Apr 2018, Jan Beulich wrote:
>> >>> On 19.04.18 at 00:15, <sstabellini@xxxxxxxxxx> wrote:
>> > Add a Xen build target to count the lines of code of the source files
>> > built. Uses `cloc' to do the job.
>> > 
>> > Generate the list of source files from the %.o targets, append output
>> > to "sourcelist".
>> > 
>> > Remove sourcelist on clean, and also at the beginning of the build
>> > target to avoid appending to sourcelist on consequence builds. Otherwise
>> > one could imagine sourcelist could become large if the user builds Xen
>> > repeatedly without calling clean.
>> > 
>> > For the cloc target, first clean, then build to make sure all files are
>> > properly accounted (no partial builds).
>> > 
>> > Signed-off-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>> 
>> All fine, but what I'm missing is why we want something like this in the
>> first place.
> 
> I provided an explanation here:
> https://marc.info/?l=xen-devel&m=152417791426130, but I can elaborate
> more if you have questions.

Yeah with that explanation I can see where you're coming from. Some
of that needs to go into the commit message here though.

>> > --- a/xen/Makefile
>> > +++ b/xen/Makefile
>> > @@ -48,7 +48,7 @@ else
>> >  endif
>> >  
>> >  .PHONY: _build
>> > -_build: $(TARGET)$(CONFIG_XEN_INSTALL_SUFFIX)
>> > +_build: clean-sourcelist $(TARGET)$(CONFIG_XEN_INSTALL_SUFFIX)
>> 
>> Both here and ...
>> 
>> > @@ -267,3 +267,13 @@ $(KCONFIG_CONFIG):
>> >  include/config/auto.conf.cmd: ;
>> >  
>> >  -include $(BASEDIR)/include/config/auto.conf.cmd
>> > +
>> > +.PHONY: cloc
>> > +cloc: $(BASEDIR)/sourcelist
>> > +  cloc --list-file=$(BASEDIR)/sourcelist
>> > +
>> > +$(BASEDIR)/sourcelist: clean build
>> 
>> ... here I'm afraid the dependencies aren't right: All dependencies can
>> be handled in parallel by make, i.e. there's no ordering implication from
>> the ordering you provide here.
> 
> I see what you mean. Nasty. Do you have a suggestion on how to better
> handle this kind of thing?

I think you'll need intermediate (pseudo-)targets, or you need to invoke the
second step as command instead of that being a dependency.

>> > --- a/xen/Rules.mk
>> > +++ b/xen/Rules.mk
>> > @@ -190,9 +190,11 @@ _clean_%/: FORCE
>> >    $(MAKE) -f $(BASEDIR)/Rules.mk -C $* clean
>> >  
>> >  %.o: %.c Makefile
>> > +  echo `pwd`/$< >> $(BASEDIR)/sourcelist
>> >    $(CC) $(CFLAGS) -c $< -o $@
>> >  
>> >  %.o: %.S Makefile
>> > +  echo `pwd`/$< >> $(BASEDIR)/sourcelist
>> >    $(CC) $(AFLAGS) -c $< -o $@
>> 
>> For one I'd prefer if this file was written only when actually processing
>> the "cloc" target you add. 
> 
> I can make the echo command conditional on the cloc target using a
> global flag.

A global flag would mean ifdef-ary here, which I'd like to avoid. Instead I
was hoping for you to define a macro which expands to nothing in the
non-cloc target case.

>> And then - is echo guaranteed to produce all
>> its output with a single atomic write? Otherwise you risk producing a
>> complete mess in sourcelist if someone hands -j to make.
> 
> I haven't seen this issue in my tests so far. POSIX guarantees that
> write requests of PIPE_BUF bytes or less shall not be interleaved.
> PIPE_BUF is 4K on Linux and is always greater than 512, which should be
> fine here. Therefore it is down to the echo implementation, as you
> pointed out.
> 
> Honestly, I would prefer to trust the echo implementation to do the
> right thing, and risk a corruption in sourcelist, rather than
> introducing file locks to solve the problem. What is your take on this?

Can't you use a different approach altogether, e.g. grep-ing the
.*.d files once the whole build is done?

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel