On Mon, Apr 16, 2012, at 09:57 AM, Дилян Палаузов 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.
>
> The point is, that some source files are always in $(srcdir), while
> others might be in $(srcdir) or in $(builddir). Let's say if the files
> are not in $(srcdir), they are generated in $(builddir).
Yes, with you so far.
> 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.
> The #include policy, which I have followed was: CPPFLAGS =
> $(top_srcdir), $(top_builddir), $(top_srcdir)/lib and
> $(top_builddir)/lib + external dependencies.
>
> The reason to include both $(top_srcdir) and $(top_builddir) is that
> some headers might be in srcdir, but others might be generated in
> $(builddir).
Agreed.
> The reason to include lib/ is that those files get at the end installed
> under /usr/include/cyrus . My logic is, the files which depend on files
> in installed in /usr/include/cyrus, need CPPFLAGS=-I pointing to the
> place where the headers files are. This might be $(srcdir)/lib, but it
> might be /usr/cyrus/include .
I would very much hope that no part of the Cyrus build process ever
includes anything from /usr/cyrus/include. That would be very wrong.
>
> > commit "Move def. Makefile.in:(PACKAGE/VERSION) to configure.ac:AC_INIT"
> >
> > Looks good, but here
> >
> > +AC_INIT([cyrus-imapd], [2.5],
> > [http://bugzilla.cyrusimap.org],,[http://www.cyrusimap.org])
> >
> > it might be nice to have the version on it's own line, as we know that
> > the version string will be modified once for every release.
>
> You have to put the version anyway on some line. Currently it is on the
> AC_INIT line, you can move it to another line, but I do not see reasons
> to choose a line different from AC_INIT.
Ah, let me explain what I meant more clearly. Instead of
AC_INIT([cyrus-imapd], [2.5], [url],,[url])
how about
AC_INIT([cyrus-imapd],
[2.5],
[url],,[url])
so that the commit that bumps the version for every release looks like
-[2.5.0],
+[2.5.1],
instead of
-AC_INIT([cyrus-imapd], [2.5.0], [url],,[url])
+AC_INIT([cyrus-imapd], [2.5.1], [url],,[url])
> > 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.
>
> If you check the final Makefile.am, you will see that nearly everything
> is ordered alphabetically. First are the files from the imap directory,
> then the files from imtest, lib, man, master, netnews, notify... Within
> each directory, the targets are also orderted: first idled, them imap,
> ipurge, lmtpd. For each target, the sources files are also ordered
> alphabetically.
>
> This makes it very easy to find something, whatever you are looking for.
An excellent idea.
> 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
> > +LIBCYRM_OBJS = assert.o hash.o imapopts.o libconfig.o mappedfile.o
> > mpool.o \
> > + retry.o signals.o strhash.o util.o xmalloc.o xstrlcat.o
> > xstrlcpy.o \
> > + @IPV6_OBJS@ map_@WITH_MAP@.o
> >
> > map_@WITH_MAP@.o is out of alphabetical order.
>
> Yes, but I put the @..@ files temporary at the end. In the final
> Makefile.am the order is alphabetical.
Ok. I'll stop checking the order until I get to the final state.
> > commit "lib/prot.h: Add #include "config.h", remove from LIBCYR_HRDS"
> > [...]
>
> I add #include config.h to lib/prot.h and remove lib/prot.h from
> LIBCYR_HDRS .
Sorry, I misread that :(
> > commit "remove inserting of files in both libcyrus and libcyrus_min"
> >
> > I find this change confusing but I can't see anything wrong with it.
> > However as long as it's a static library I think you could definitely go
> > one step further and eliminate libcyrus_min entirely.
>
> What is confusing?
Many many changes all at once. Generally, you've been very good with
keeping the commits small and readable, but this one was harder.
> > 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.
>
> That is not true. The master program is the only program, that depends
> only on libcyrus_min.a, but not on libcyrus.a . This was so before my
> changes.
Ok, let me rephrase that...master needs to depend on whichever library
provides it with strarray_*() functions, xmalloc(), etc. The Makefile
after this commit has master/master depending only on the .o files in
the master/ directory. If I change lib/strarray.c, master will
(incorrectly) not be relinked.
> > commit "lib/cyrusdb_twoskip.c: undef VERSION from config.h"
> >
> > Isn't it PACKAGE_VERSION now?
>
> It is PACKAGE_VERSION, but config.h for some reason still defines
> VERSION.
Hmm, backwards compatibility probably.
--
Greg.
