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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 20 Apr 2018, Jan Beulich wrote:
> >>> 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?

Actually, it seems possible. It feels a bit haskish to me, but it has
the benefit of having no impacts whatsoever to the normal build. I'll
change approach to what you suggested.

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