[ovs-dev] [PATCH] automake: Cleanup to allow make distcheck to succeed.

Ben Pfaff blp at nicira.com
Mon Jul 20 18:31:13 UTC 2015


On Sat, Jul 18, 2015 at 05:05:54PM -0400, Aaron Conole wrote:
> Ben Pfaff <blp at nicira.com> writes:
> 
> > Can you explain the following change?  What effect does it have, and why
> > is it needed only in one fork of the "if"?
> >
> >> --- a/Makefile.am
> >> +++ b/Makefile.am
> >> @@ -44,7 +44,7 @@ endif
> >>  if WIN32
> >>  psep=";"
> >>  else
> >> -psep=":"
> >> +psep=:
> >>  endif
> 
> Make does not accept quoted strings for interpretation. In this case,
> that means that when psep is used in a target, it will be ":" instead of
> the expected : which can be confusing to read (or worse) the underlying 
> shell commands in the build log.

Make doesn't interpret these quotes, the shell does, and it drops them.
Does this fix an actual problem or is this some kind of style issue?

> > In the two cases where this patch "chmod"s source directories so that
> > header files can be generated, I think it would be better to generate
> > the header files in build directories instead.
> 
> I will look into how to do this - I agree, that change would be far more
> elegant; I'm not sure if it would be a big change to the ovs build
> system as it stands or if it will be simple additional include directory.

I don't think it woudl be a big change.

> > I don't understand why this adds ofp-errors.c and ofp-msgs.c to
> > EXTRA_DIST.  These files should be distributed already because they are
> > in lib_libopenvswitch_la_SOURCES.
> 
> I agree; it's confusing to me as well, since the ofp-errors.c and
> ofp-msgs.c aren't so different from (for example) ofp-actions.c or
> any of the other libopenvswitch deps. However, it was something I needed
> to do to get the build to work - I'll dig around a bit more.

OK, I'd like to have that explanation to include in the commit message.

> > I don't understand the following change, can you explain?
> >
> >>  .ovsidl.c:
> >> -	$(AM_V_GEN)$(OVSDB_IDLC) c-idl-source $< > $@.tmp && mv $@.tmp $@
> >> +	$(AM_V_GEN)$(OVSDB_IDLC) c-idl-source $< > $@.tmp && mv -f $@.tmp $@
> >>  .ovsidl.h:
> >> -	$(AM_V_GEN)$(OVSDB_IDLC) c-idl-header $< > $@.tmp && mv $@.tmp $@
> >> +	$(AM_V_GEN)$(OVSDB_IDLC) c-idl-header $< > $@.tmp && mv -f $@.tmp $@
> >>  
> >>  BUILT_SOURCES += $(OVSIDL_BUILT)
> >
> 
> If I recall correctly, these files were generated during the dist
> and (by distcheck target) marked as read-only. The mv -f will do a 
> force move. I will try to dig around further and see what the 
> underlying cause for the read-only flag on these files is (or even if
> there's an ordering issue or something else in there).
> 
> I definitely would like to see distcheck working from the travis-ci side so
> that future changes can catch breakage / regression.

Yes, it would be best to have it working.

It seems to me that this single commit ought to be broken up into
multiple, each of which has a commit message that explains what it
fixes.

Thanks,

Ben.



More information about the dev mailing list