commit "mboxevent: Rewrite JSON formatting"

[Дилян Палаузов]

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.

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.

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.

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
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: dilyan_palauzov.vcf
Type: text/x-vcard
Size: 380 bytes
Desc: not available
Url : http://lists.andrew.cmu.edu/pipermail/cyrus-devel/attachments/20120814/eb1eb08f/attachment.vcf