Automake Support for cyrus-imapd 2.5

[Greg Banks]

On Tue, Apr 17, 2012, at 03:09 PM, Dilyan Palauzov wrote:
> >> > commit "Prepend the path in #include to .et ->  .h files"
> >> > [...] I don't see why you would change
> >> >
> >> > -#include "imap_err.h"
> >> > +#include "imap/imap_err.h"
> >> >
> >> > without changing any of the other #include lines around it.
> >> >
> >
> >> >
> >> > Generally, I think it would be a good thing to have a clear consistent
> >> > and documented policy for how we #include headers.  We don't really have
> >> > that now, and I don't think this commit gets us any closer.
> >>
> >> These files
> >> are prefixed with the path, as in #include "imap/imap_err.h", so that
> >> the build works (this was my conclusion, when I was trying to build with
> >> VPATH).
> >
> > Why make the way in which files are #include'd different depending on
> > whether they are built sources or not?  That seems very confusing.  I
> > would very much prefer to have every #include line use the same format,
> > either
> >
> > #include "imap/imap_err.h"  /* relative path down from
> > top_{build|src}dir */
> >
> > OR
> >
> > #include "imap_err.h"  /* filename only, rely on lots of -I options */
> >
> > but not confusing admixtures of both.  I don't care which one, just that
> > it's consistent.  It would also be nice if the convention was documented
> > in doc/internal/hacking.
> > If you were experiencing problems with build order due to built headers
> > not being available when a .c file was being compiled, then that's what
> > BUILT_SOURCES is for.
> 
> I prefer to keep (AM_)CPPFLAGS minimalistic and have everthing  
> relative to the top_dirs.  Automake includes automatically -I.  
> -I$(srcdir) and -I(the directory with config.h = top_builddir), so it  
> is not necessary to add those explicitly (even if it is done for  
> top?builddir).  

Yes, I agree so far.

> Specifying the path to generated and included *.h  
> files is the minimal effort required to keep AM_CPPFLAGS as small as  
> possible (top_dir and top_dir/lib).

I have two problems with this statement.

Firstly, I don't see why generated .h files need to be treated any
differently.  If they're mentioned in BUILT_SOURCES then they get built
before everything else in the 'all' target, so by the time any other .c
file is compiled or the dependencies are extracted the generated .h
already exists and is found correctly.

Secondly, lib/ is not the only directory from which .h files are
included.  I did a quick count based on gcc-generated .dep files and
found

3578 headers included into .c files, comprising
1456 included from "."
174 include from top_dir into files in other directories (e.g. config.h)
1872 included from top_dir/lib into files in other directories
61 included from top_dir/imap into files in other directories
14 included from top_dir/sieve into files in other directories
1 included from top_dir/master into files in other directories

> >> > commit "lib/Makefile.in: Reorder LIBCYR(M)_(OBJS, HDRS) alphabetically"
> >> >
> >> > Looks good.  While you're doing that, how about splitting them up one
> >> > per line?  We have all sorts of headaches doing git rebases of commits
> >> > which add or remove .c files because of this.
> >>
> >> Okay, we can split them one per line, if you want.  But you can also put
> >> them all on the same line.  Having them all on the same line, it is
> >> quite easy to find where the change is.
> >
> > Ah, but *finding* is not the problem; my text editor has a working
> > search function.
> >
> > The problem is when you're rebasing a commit that adds a single source
> > file, and the upstream changes you're rebasing around add or remove
> > another source file.  With multiple source files per line, you are far
> > more likely to get spurious context damage which prevents the git merge
> > from succeeding, which then requires a manual merge conflict fix, which
> > is annoying slow and error-prone.  When Bron and I were actively working
> > on the conversations branch we would suffer through dozens of these
> > spurious merge conflicts per week.  See commit
> > 3aa2792f402a27687cf0ecdcd38654716436ec0c
> 
> The problem would probably not have appeared, if all the files were on  
> a single, very long line.

On the contrary, such an arrangement is the worst possible case because
every change to the list of files is a merge conflict waiting to happen.
 It would have resulted in conflicts every single time we had to rebase
around a commit which added a source file, rather than about half of
them.  

> > +master: master.o masterconf.o cyrusMasterMIB.o
> >
> > the master program needs to depend on libcyrus.a in some way.  For
> > example, in imapd/Makefile.in we do
> 
> In Makefile.am the dependencies are right.

Ok.

-- 
Greg.