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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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.

Thanks for the sharp review.



> > ---
> >  xen/Makefile | 14 ++++++++++++--
> >  xen/Rules.mk |  2 ++
> >  2 files changed, 14 insertions(+), 2 deletions(-)
> 
> .gitignore ?

Good point, I'll add sourcelist to .gitignore


> > --- 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?


> > --- 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.


> 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?


> Furthermore - are e.g. header files not counting at all?

I have been wondering about that too. I am not sure, but I have seen
other LOC counts in the past ignoring header files. In any case, I
thought that C and ASM files would be a good start. 

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