commit "mboxevent: Rewrite JSON formatting"

[Sébastien Michel]

> please delete imap/Makefile.in : all the build rules are in /Makefile.am .

It's already done. Sorry, I just forgot to mention it.

>
>>> As libjson supports the .pc format, you can detect libjson in
>>> configure.ac with
>>>    PKG_CHECK_MODULES ([libjson], [json >= 0.10], [check_libjson=yes],
>>> [check_libjson=no])
>>> and remove the configure.ac:AC_CHECK_LIB handling.  Possibly add an
>>> AM_CONDITIONAL([LIBJSON], ["$check_libjson" = "yes"]).  I know currently
>>> cyrus-imapd does not use the
>>> PKG_CHECK_MODULES-routine, but on the other side there are no other
>>> external modules, which can be detected by it.
>>> I do not have recipe to how to trivially #define HAVE_LIBJSON when using
>>> libjson, but this shall not be hard to handle.
>>
>> I've used PKG_CHECK_MODULES and it looks fine. However I have noticed some
>> issues :
>> - By default, the macro will set up two variables, joining the given
>> prefix with the suffixes _CFLAGS and _LIBS.
>> The macro will call AC_SUBST on these variables only with pkg-config 0.24
>> and later
>> What do you prefer between always calling AC_SUBST  or testing the
>> pkg-config version ?
>>
>> - Using PKG_CHECK_MODULES in conditional block would raise a failure.
>> Calling PKG_PROG_PKG_CONFIG seems to be one possible solution :
>> http://www.flameeyes.eu/autotools-mythbuster/pkgconfig/pkg_check_modules.html#idp1027760
>>
>> - some people discourage the use of such macro :
>> http://stackoverflow.com/questions/10220946/pkg-check-modules-considered-harmful
>
>
> I am in favour of PKG_(PROG_)PKG_CONFIG, but if you prefer not to use it,
> then you can leave configure.ac as it is.

OK.
Can we consider that we support only version 0.24 minimum ?

>
>>> Then shift the lib/xjson.c and lib/xjson.h to imap/,
>>
>> xjson.[ch] doesn't depends on any structure/function in imap folder. As a
>> basic utility I think it should be located in lib.
>> Why do you prefer move it here ?
>
>
> Provided that the json code is integrated and used within libcyrus_imap (and
> not libcyrus(_min), then the files have to go under imap/ -- there are all
> libcyrus_imap sources.  While for the future it would be wise, to have the
> source files for libcyrus_imap in a separate directory, for the next major
> release the directory structure will not change.  This has nothing to do
> with being a basic utility or not.
>

I don't understand than you this comment from Makefile.am :
# BASIC is the libraries that every Cyrus program (except master) will
# need to link with.
#
# Note that several places in the code use -lcrypto, e.g. for SHA1 or
# MD5 algorithms, without needing SSL.  Currently we have no way of
# minimally linking such code.
LD_BASIC_ADD = lib/libcyrus.la lib/libcyrus_min.la ${LIBS}

# UTILITY is the libraries that utility programs which use Cyrus'
# mailbox and message handling code need to link with.
LD_UTILITY_ADD = imap/libcyrus_imap.la $(LD_BASIC_ADD) $(COM_ERR_LIBS)

As xjson may be linked with every Cyrus program except master (ie.
imapd, pop3d, lmtpd, cyr_expire, ipurge, ...)
I understand that it should be added in libcyrus.la or lib/libcyrus_min.la
I noticed that some files like iostat.c and parseaddr.c are not used
within lib/ but are integrated within libcyrus.la. Thus, I think I
should also integrate xjson.c within libcyrus.la (not libcyrus_min)
and not move it to imap/ ...

>>> (in the beginning of Makefile.am, for correct CPPFLAGS) AM_CFLAGS = ....
>>> $(libjson_CFLAGS)
>>
>> I don't know no what is better between that or this declaration below :
>> imap_libcyrus_imap_la_CFLAGS += $(JSON_CFLAGS)
>
>
> Having different CFLAGS per target, implies that a single source file can be
> compiled several times for different targets with different -D directives.
> In order to avoid disasters, Automake renames the resulting .o files to be
> very long, and to contain the target, for which the files are compiled.
> This long file names are avoided, if only one (AM_)CFLAGS is used.
> Currently you will not see a difference, since the -fvisibility= switch is
> only half-done, and there are per library CFLAGS, but once I get it right,
> no files in imap/.libs will start with imap_libcyrus_imap_X.o, but be called
> only X.o.
>
Ok

Thanks,
Sébastien