GNOME Bugzilla – Bug 522639
libcamel dependency
Last modified: 2009-05-23 21:44:29 UTC
Looks like we require libcamel only for the following: /* Fall back to RFC 2822 date parsing */ return camel_header_decode_date (date_str, NULL); Considering that the RFC probably won't be changing it might be ok to copy this function into totem. Thoughts?
I didn't manage to do it at the time without copying half of libcamel. You're welcome to try...
This one line of code however seems to pull in the entire evolution-data-server which seems a little silly. Even using an entire library for just that one function seems a little whack, but I suppose that could be acceptable. Any chance this will get changed?
The function is hard to implement, which is why I'm using a library. Unless someone reimplements the functionality separately, I think you should bring your concerns to your distro's packagers.
I suppose copy pasting it from libcamel isn't an option :p Well I always thought Gentoo tried to minimize/separate dependencies as much as possible. I haven't found where to get libcamel separately, it seems to be part of eds, something you apparently can't do without. I understand the difficulty in this progress, and your doing a great job so far. But from a architectural point of few, what does a Playlist manager for a audio/video player have to do with an e-mail application backend? I could understand it in an IM application, but architecturally it doesn't make sense. From what I saw in the code, it's a fall back for when the ISO version fails e.g. Try iso approach first, then The RFC one. The ISO seems to be dating from 1988 I belive, so the first obvious question is, how often does the fall through case get involved? If it never happens, or only extremely rarely, wouldn't it be an idea to just drop it all together? On that note, I'm unsure as to how this feature is being used, time/date in the playlist? P.S. I'm not trying to be disrespectful or offensive only helpful.
This is used to parse dates in Podcasts. RSS feeds use the "old" standard, Atom feeds and OPML feeds use the new one.
So we can't do without, fair enough. And as mentioned in the original bug report, copying and minimizing that one function isn't an option? No other library implements that rfc either?
Not that I found.
So I went hunting, and found many implementations raging from perl, via python, to even haskell and pod. Many scripting languages that I didn't even know off have modules for them. It's hard to imagine there is not C implementation for it, other then eds. I did find that git uses rfc822 date/time parsing, so i'm gonna look at their code now, and see how they do it.
It's very likely that git only parses dates it generates itself, whereas libcamel knows how to parse all kind of crap.
FWIW, I've been trying to separate Camel from libedataserver at least logically, even if we don't wind up shipping them separately. Haven't gotten much of a response from the other Evo developers though. See bug #424744.
I'm looking through the date conversion function, and I think it's only a matter of copying 2 small functions. I'll see if I can work something out. BTW the function in question has the following header comment: /* convert a date to time_t representation */ /* this is an awful mess oh well */ time_t camel_header_decode_date(const char *in, int *saveoffset)
Go on, give it a try... You'll end up pulling half of libcamel. As for the comment, that's a bit expected when you have to handle all the crap coming from broken clients.
Created attachment 107572 [details] Just throwing some stuff together I did have to add broken-date-parser.c/h no point in copying those files entirely. The proposal to logically seperate camel from the rest as pointed out above needs to look into e_mktime_utc, the only reference in there I found. It's also the only modification I made to that file, added a simple function for it and removed the header reference to the libedataserver. code: tm->tm_isdst = -1; tt = mktime (tm); There are some more define's included int he edataserver part, which I suppose could be included, but for this proof of concept shouldn't be needed. I also removed all the d() and d2() stuff from the code in cameless.c (I know it would have been easier/smarter to make the define for it) Compared to the whole camel thing, this is a very small fraction of it. I didn't do extensive testing, but it seems to be working. Of course this should/could maybe be severely trimmed, since we don't need all the share-ability in a few of these functions. Also the table at the beginning should be put back into the single file it was in, for the simple reason that it's a generated table, and to keep it 'as is' making it very easy to update when a new one comes out. It may not be pretty, but it beats pulling a huge dependency that libedataserver is for a mere 891 lines of code. This is less then 1% of the 91497 lines libcamel is, and incomparably less then what libedataserver is. Just copy broken-date-parser.* over, add the aforementioned change, compile with gcc -o cameless broken-date-parser.c cameless.c `pkg-config --cflags --libs glib-2.0` and it can be thrown some sample data (which I don't know much about to be fair).
Please provide a patch, and something that's updatable from libcamel. I'm not interested in having to cut'n'paste manually from libcamel. One of the points of using libcamel is that I don't have to fix the bugs, or follow the fixes.
There's also a number of date parsing self-tests in test-parser.
moving libcamel out of e-d-s would reduce the overkill, but that discussion may take a few more years... sigh.
It should be possible to use librfc822 from maildrop. rfc822_parsedt() is the function needed. See docs at: http://www.courier-mta.org/maildrop/rfc822.html If somebody wants to check the library is able to parse the dates listed in test-parser.c. I've filed a bug against Fedora to get the library exported (it's not installed by default in the package): https://bugzilla.redhat.com/show_bug.cgi?id=459580
Attached a patch to do the libcamel dependency optional. My main interest on this topic is to use totem-pl-parser in tracker for maemo (and upstream). Brief summary of the modifications: totem-plparser.pc.in: Dependency of libtracker optional and added a property to know if the library is compiled with libcamel or not (useful for rhythmbox to know if podcast are supported) configure.in plparse/Makefile.am: Added autotools stuff to check libcamel and the proper flags and libs plparse/totem-pl-parser.h plparse/totem-pl-parser.c: ifdefs for the parsing date function depending on libcamel availability. plparse/test-parser.c: ifdefs to remove the code testing date parsing if libcamel is not available. Please review.
Created attachment 121414 [details] [review] Optional dependency of libcamel
[I'm not a totm-pl-parser maint, but] I just saw this configure snipplet and have a comment on it: +AC_ARG_ENABLE(camel, + AS_HELP_STRING([--enable-camel=@<:@no/yes/auto@:>@], + [Use libcamel to parse dates (if available).]), , + [enable_camel=auto]) + +if test "x$enable_camel" != "xno" ; then + PKG_CHECK_MODULES(LIBCAMEL, + camel-1.2, + [have_camel=yes], + [have_camel=no]) + + if test "x$have_camel" = "xyes" ; then + AC_SUBST(LIBCAMEL_CFLAGS) + AC_SUBST(LIBCAMEL_LIBS) + AC_SUBST(LIBCAMEL, camel-1.2) + AC_SUBST(USELIBCAMEL, yes) + AC_DEFINE(HAVE_CAMEL, 1, [Camel available in the system]) + else Automagic dependencies are bad, since they create unpredictable results. IMHO this should be enabled by default and error out if camel isn't found, and you'd need to pass --disable-camel if you know what you're doing.
Created attachment 121419 [details] [review] Optional dependency of libcamel (now "yes" by default, and no "auto" option) New version of the patch, removing the "auto" option in configure.in and using by default "yes" (as suggested by Christian).
+ This can affect seriously the podcast handling. It doesn't seriously affect it, it breaks it. +#ifdef HAVE_CAMEL guint64 totem_pl_parser_parse_date (const char *date_str, gboolean debug); +#endif You can't do that in a public header. #ifdef HAVE_CAMEL Please remove the ifdef HAVE_CAMEL in test-parser.c Also, for the change in configure.ac, check the libcamel option first, then simply add/remove libcamel to the PKG_CHECK_MODULES call for TOTEM_PLPARSER. No need to modify Makefile.am then.
Created attachment 121439 [details] [review] Optional libcamel with last comments from Bastien Applied the suggested changes on the patch. Now it only modifies: * configure.in: Adding the check for libcamel * plparser/totem-pl-parser.c (totem_pl_parser_parse_date) g_warning indicating a call to the function without libcamel support. * totem-plparser.pc.in: Added variable to know if the library is compiled with libcamel support and to set dynamically the library dependencies.
Applied with slight changes. The configure option is now: --disable-camel-i-know-what-im-doing Disable libcamel (Unsupported, breaks Podcast support). And every Podcast parsing function will fail with a big fat warning, and an error. Could you please file a separate bug for the Rhythmbox check as well? Any further hacking should be done on reducing libcamel dependencies. 2008-11-05 Bastien Nocera <hadess@hadess.net> * configure.in: * plparse/totem-pl-parser-podcast.c (totem_pl_parser_add_rss), (totem_pl_parser_add_itpc), (totem_pl_parser_add_zune), (totem_pl_parser_add_atom), (totem_pl_parser_add_xml_feed), (totem_pl_parser_add_itms), (totem_pl_parser_add_opml): * plparse/totem-pl-parser-podcast.h: * plparse/totem-pl-parser.c (totem_pl_parser_parse_date): * totem-plparser.pc.in: Adapt patch from Ivan Frade <ivan.frade@nokia.com> to make libcamel optional for some "embedded" platforms. This is absolutely unsupported and any general purpose distribution shipping with libcamel disabled will be kicked in the knackers forcefully (Closes: #522639)
Just an idea, not sure if its workable or not. Would it be possible to move the date parsing function that camel uses into libical (they are both date related although I'm not sure its an exact match) when evolution moves to using the upstream libical. That way totem would only need to depend on the much smaller libical, and e-d-s already would be depending on it anyway. That would split it out into a single upstream location that's a smaller dep and still a single point for maintenance?
(In reply to comment #25) > Just an idea, not sure if its workable or not. Would it be possible to move the > date parsing function that camel uses into libical (they are both date related > although I'm not sure its an exact match) when evolution moves to using the > upstream libical. That way totem would only need to depend on the much smaller > libical, and e-d-s already would be depending on it anyway. That would split it > out into a single upstream location that's a smaller dep and still a single > point for maintenance? Where's that elusive upstream libical, and are you sure it provides parsing for RFC822 dates?
The upstream libical is http://freeassociation.sourceforge.net/ according to the Fedora package and I don't believe it currently provides it (although it might provide an equivalent). Evolution is moving from its included copy and pushing its changes upstream. I was wondering whether this date function could also be pushed upstream to that library allowing the dependency to move as well. Matthew Barnes would be the best person to answer that question though.
(In reply to comment #27) > I was wondering whether this date function could also be pushed upstream > to that library allowing the dependency to move as well. Well, if Bastien couldn't pull out that Camel function without dragging in half the library I'm not sure it'd be any easier for the libical guys. I've lost track of how the E-D-S merge to upstream libical is going. Bug #541209 is tracking the effort but hasn't been updated in awhile. Maybe post an inquiry there or at <evolution-hackers@gnome.org>. Personally, I think it's toss up which will happen first: E-D-S ditches its libical fork or Camel breaks free of E-D-S. We do have more control over the Camel situation...