commit "mboxevent: Rewrite JSON formatting"

[Michel Sébastien]

Hello Dilyan,

I've just finished to rewrite the event notifications code according to Greg's comments.
I'm now working to take into account yours and I have already a first implementation (see http://git.cyrusimap.org/cyrus-imapd/commit/?h=ticket/3605&id=fb785dbf1f31c93728d7e077ff2112e404e29eee)


> you have put the support for -ljson in libcyrus_min .
>
> According to my understanding, in libcyrus_min come only the files needed by the master process (as it does not link with libcyrus).
> If you use libjson in the imap/ folder, and not all command line utilities need the functionality from json,
> then I would at your place shift the handling of json from libcyrus_min to libcyrus_imap., so that the library is not loaded unnecessary.
Done
I noticed that the prefix value is usually provided in uppercase

> 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

> 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 ?

> intergrate xjson.c conditionally with libcyrus_imap with Makefile.am (or unconditionally, if the files are compiled even if libjson is not found, in this case the above AM_CONDITIONAL is not necessary):
> if LIBJSON
> imap_libcyrus_imap_la_SOURCES += imap/xjson.c imap/xjson.h imap_libcyrus_imap_la_LIBADD += $(libjson_LIBS) endif
Done

> (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)

Cheers,

Sébastien

-----Message d'origine-----
De : Дилян Палаузов [mailto:dilyan.palauzov at aegee.org]
Envoyé : dimanche 1 juillet 2012 16:37
À : Michel Sébastien
Objet : commit "mboxevent: Rewrite JSON formatting"

Hello Sebastien,

you have put the support for -ljson in libcyrus_min .

According to my understanding, in libcyrus_min come only the files needed by the master process (as it does not link with libcyrus).  If you use libjson in the imap/ folder, and not all command line utilities need the functionality from json, then I would at your place shift the handling of json from libcyrus_min to libcyrus_imap., so that the library is not loaded unnecessary.

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.

Then shift the lib/xjson.c and lib/xjson.h to imap/,

intergrate xjson.c conditionally with libcyrus_imap with Makefile.am (or unconditionally, if the files are compiled even if libjson is not found, in this case the above AM_CONDITIONAL is not necessary):
if LIBJSON
imap_libcyrus_imap_la_SOURCES += imap/xjson.c imap/xjson.h imap_libcyrus_imap_la_LIBADD += $(libjson_LIBS) endif

(in the beginning of Makefile.am, for correct CPPFLAGS) AM_CFLAGS = ....
$(libjson_CFLAGS)

Please delete in the branch ticket/3605 the file imap/Makefile.in added by commit "added support for event notifications on message store based of RFC 5423" from August 2011 (I know there was no automake support then, but now there is).

Please add imap/mboxevent.h to imap_libcyrus_imap_SOURCES, otherwise the file will not get into the tarball.

Със здраве
   Дилян


Ce message et les pièces jointes sont confidentiels et réservés à l'usage exclusif de ses destinataires. Il peut également être protégé par le secret professionnel. Si vous recevez ce message par erreur, merci d'en avertir immédiatement l'expéditeur et de le détruire. L'intégrité du message ne pouvant être assurée sur Internet, la responsabilité d'Atos ne pourra être recherchée quant au contenu de ce message. Bien que les meilleurs efforts soient faits pour maintenir cette transmission exempte de tout virus, l'expéditeur ne donne aucune garantie à cet égard et sa responsabilité ne saurait être recherchée pour tout dommage résultant d'un virus transmis.

This e-mail and the documents attached are confidential and intended solely for the addressee; it may also be privileged. If you receive this e-mail in error, please notify the sender immediately and destroy it. As its integrity cannot be secured on the Internet, the Atos liability cannot be triggered for the message content. Although the sender endeavours to maintain a computer virus-free network, the sender does not warrant that this transmission is virus-free and will not be liable for any damages resulting from any virus transmitted.