Hello,
on Greg's first comments:
commit "imap/Makefile.in: add .c.snmp: recipe"
The old makefile will rebuild pushstats.c if the snmpgen script changes.
The new one won't.
I fixed this.
commit "lib/mkchartable.pl move output from stderr to stdout"
In mkchartable.pl, you need to handle the open() failing.
Also, mkchartable.pl can fail partway through, e.g. if one of the
charset/*.t files is not readable. In the old makefile, such a failure
would trigger this code
- || (rm -f chartable.c && exit 1)
which would break the build and avoid half-creating a chartable.c so
that the next attempt to build was also (correctly) fail. In the new
makefile there is no such safeguard: a partly finished chartable.c would
be created and the next attempt to build would (incorrectly) not fail at
this point.
I fixed this now.
> 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). 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).
> 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. I prefer to have them on one line (even not
split with \)
commit "lib/Makefile.in: expand ACL and AUTH"
commit "lib/Makefile.am: move strarray from libcyrus.a to
libcyrus_min.a"
Looks good, but here
+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.
Greetings
Dilian
