commit "mboxevent: Rewrite JSON formatting"

[Greg Banks]

Sent from my iPhone

On 15/08/2012, at 2:40, Дилян Палаузов<dilyan.palauzov at aegee.org> wrote:

> Hello Michel,
> 
> according to my understanding, there are no strict rules what goes in libcyrus, libcyrus_min and libcyrus_imap .  I am not aware of a guidelines, describing in which library to include what kind of file. While working on getting Automake/libtool in cyrus-imapd, I moved some files between libcyrus and libcyrus_min according to how I felt the things.

I think we should definitely come up with a written definition of these and encode it in the Cyrus docs. The current situation is way too confused.

Here's my idea of what the splits should be.

The split between libcyrus_imap and the others is that it contains higher level "model" code, i.e. mailboxes, messages, and the like. The others contain "utility" code, strings and the like. There are some grey areas and some arguably misplaced code, eg parseaddr.c.

The split between libcyrus and libcyrus_min is more easily defined: if the master process needs it to link, it must be in libcyrus_min, otherwise it must be in libcyrus.

This strongly minimizes the amount of code in the master process, which is necessary because the master process cannot be upgraded gracefully in place (most of the other processes such as imapd will detect when their binary has been overwritten and exit so that master can restart them, but master itself cannot do this).

By this definition we have a major problem where lib/util.c drags in a lot of unnecessary stuff into master.  That needs fixing, presumably by splitting up util.c.

We also have several other daemons that cannot be gracefully upgraded in place, notably idled. That also needs fixing.

> 
> lib/parseaddr.c is used within libcyrus_sieve and imap/, so having it in the common library libcyrus seems logical.  I do not know, why iostat.c is part of libcyrus.

I think parseaddr should be part of libcyrus_imap; it's clearly a domain model object. I also think libcyrus_sieve should just use more things from libcyrus_imap instead of reimplementing them badly or delegating them to poorly implemented callbacks. My work on the search branch will help move us in that direction by providing a useful message abstraction.


> 
> If nobody else expresses opinion, whether to put xjson in libcyrus or libcyrus_imap, it is up to you.  I just told you my opinion.

My 2c: JSON code is a utility and should go in libcyrus.

> 
> I have no opinion about the minimal required version for json.
> 
> The comment in the Makefile state, that LD_BASIC_ADD is used to link with programs, linking with libcyrus and libcyrus_min, while LD_UTILITY_ADD is used to link with all command line tools (that are not invoked as services from master). -lcrypto is always provided, even if the linked program does not use libcrypto.
> 
> Със здраве
>  Дилян
> 
> 
> 
> On 14.08.2012 16:21, Sébastien Michel wrote:
>>> 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
>> 
> <dilyan_palauzov.vcf>