GNOME Bugzilla – Bug 541209
Merge libical changes upstream and make it as a external dependency for GNOME modules
Last modified: 2009-01-22 07:52:20 UTC
Hi.. After some discussions with Chen and Jony, I'm proposing to move the current location of libical within the e-d-s hierarchy from e-d-s/calendar/libical to e-d-s/libical. Moving this would allow for libical to be compiled before the calendar directory gets compiled. Also, libical can be selectively installed (even if one chooses not to compile calendar). WHY do I want to move this? -> While working on the MAPI branch, I came across this interesting use-case: The server would return the data for an event (in some format other than iCal). This is handled by code in servers/mapi. This data is then marshaled to the calendar backend where it is converted to an icalcomponent. The server would return data in the same way for events in a user's mailbox (aka meeting requests). Before rendering this message, it is necessary to convert the event data to an ical string, which implies, I need to duplicate the data->icalcomponent conversion code in camel/providers/mapi :-/ [plus with the added constraint that I can't link to libical to make use of functions like icalcomponent_as_ical_string()] This can be avoided if I had the conversion code in a common area - i.e. servers/mapi and link servers/mapi to libical. This implies - I need to compile libical before I compile servers/* While I've explained using the mapi use-case as an example, I'm quite sure it'd be most logical for servers/* to be interacting with the corresponding calendar backend and camel provider in the same way :-) Does this have any SIDE-EFFECTS? -> Other than changing the path for static linking in all the calendar backends, none. Patch on its way. CC'ing all maintainers / developers [these are all the folks I know / have heard of :-)]. Guys.. do you have any objections?
looks like I missed adding Ross to the CC list.
Created attachment 113842 [details] [review] Changes needed to support the move. This patch also fixes the link path in libecal/libedata-cal/backends etc which statically link to libical.
If more than one library is static linking to our libical, then we should probably install it as a shared library (although not as /usr/lib/libical.so as we patch it) and dynamically link to it instead. Note that upstream libical development has restarted so in the long-term we should try and merge the changes there (a lot of them have already been merged in) and drop the local fork.
Suman, Ross tells the same as what I have told you. But we need to see how active the libical upstream community is. If it active and make sense, Im for it.
I just saw the ChangeLog (http://freeassociation.svn.sourceforge.net/viewvc/freeassociation/trunk/libical/ChangeLog?view=markup) file in the upstream community at http://freeassociation.sourceforge.net . Last six months changes are missing there. Ross, Do we want to propose these missing changes to be merged there and add libical as an external dependency ?
Chen/Suman there are two things. (1) Using upstream version (2) Making libical a dynmanic linked lib. Even if we aren't doing (1), as Ross says, since more than one is linking to libical, we must make it a dyn linkable lib (2) If the last commit was 6 months old, then I would say, no for this (1).
This anyways need to be done in a way to dyn link it.
Am ok with doing 2 first. Am trying to get the email id of the upstream maintainer to get him added in CC. I know his name is dothebart.
(In reply to comment #2) > This patch also fixes the link path in libecal/libedata-cal/backends etc which > statically link to libical. This seems to be mis-leading. The backends link to libecal, which in-turn links to libical. [So, we really don't have multiple libical objects in memory at the same time.] The patch fixes the INCLUDES path for all the backends.. and the link path in libecal (In reply to comment #7) > This anyways need to be done in a way to dyn link it. after seeing bug #347895 and friends, I can't seem to think of any solution other than moving libical up and above e-d-s to fix this issue..
(In reply to comment #5) > Ross, Do we want to propose these missing > changes to be merged there and add libical as an external dependency ? Pushing changes upstream always is a good idea, now that libical seems to be in development again. A new external dependency needs approval, though.
This all seems pretty timely to me..I was just considering this issue I'm a new fedora volunteer and may be offering to take on the packaging of libical. As I've been researching it - it looks like most projects that depend on it have chosen to fork a copy of it. I think it would make sense to move back to having one copy. All projects I've found that depend on it (except one) seem to have chosen to fork. The two well-known names that have forked are: Evolution and Sunbird (I still need to look at source control for sunbird to confirm this) Citadel is the only package I know of which relies on libical as a separate package. (any additional info on packages which use it is welcome :-) In addition to packaging for Fedora, I would also be willing to merge the gnome copy of libical back to the owner's sourceforge repository for libical. Alternatively, we could ask the owner if he would like to make the gnome repository libical's new official home. This would avoid some merging (I don't know what if any changes sunbird has made) Is anyone contacting the owner of libical yet? I'd like to get this moving along and am tempted to email him today.
Additional wrinkle... As things stand right now, it does not seem to be possible to compile libical standalone from the gnome svn repository. After a bit of futzing (my autoconf / automake knowledge is a bit rusty) I do a make and get the following error after it does some building... cannot stat `.deps/icalderivedparameter.Tpo': No such file or directory It's possible my build steps are incorrect - advice welcome.
Just emailed dothebart suggesting he visit this bug and become involved in the disucussion. Someone on irc suggested he would be the most appropriate, of those listed in the libical AUTHORS file, to comment on this.
Created attachment 114633 [details] [review] Dynamically link to libecal. libedata-cal etc., (In reply to comment #7) > This anyways need to be done in a way to dyn link it. there you go Srini.. :-) all the backends now link dynamically to libecal et al.
Jeff, currently it is not possible to build libical as a separate module and we have not spent any effort as evolution-data-server was the only module using it inside GNOME. I hope this can be done while merging the changes to upstream. I had one comment from dothebart in one of my blogs. So I will blog about this bug today and our interest in merging the changes to upstream. BTW, I could not find his email id in the CC list. Is it possible to add him to CC list if you have his ID?
latest changes to freeassociation.sf.net libical aren't visible in CVS anymore since we switched to SVN. How does moving the fork around relate to upstream libical in special? or are you thinking about trying to separate it completely from the rest of your source so you could easily switch to upstream libical alltogether? -- dothebart
I have communicated with both main coders for libical (dothebart and art) by email - Art said the following in his reply to me when I asked him what I should let the gnome group know. >Simply tell them that we are quite serious about this goal, and are > willing to work together with downstream packages to make it happen. > > -- Art
Just to be clear - the goal mentioned is merging forks of the libical code back into the official copy.
This is awesome news and we should seriously look at doing this at the immediate possible timeframe. 2.26 should be a good time for it, mean time the merging should happen well. Chen, can you take this forward?
(In reply to comment #16) > latest changes to freeassociation.sf.net libical aren't visible in CVS anymore > since we switched to SVN. > How does moving the fork around relate to upstream libical in special? or are > you thinking about trying to separate it completely from the rest of your > source so you could easily switch to upstream libical alltogether? > -- dothebart > Yes, we are thinking of removing the fork maintained in svn.gnome.org and pick up the packages from upstream. This means that libical would be added as a external dependency (http://live.gnome.org/TwoPointTwentythree/ExternalDependencies) in GNOME. I agree with srini, we can prolly do this up for 2.26 release. Is there any link to see the latest svn sources maintained upstream ? Jeff, thanks a lot for co-ordinating the effort!!!
(In reply to comment #14) > Created an attachment (id=114633) [edit] > Dynamically link to libecal. libedata-cal etc., > > (In reply to comment #7) > > This anyways need to be done in a way to dyn link it. > > there you go Srini.. :-) all the backends now link dynamically to libecal et > al. > Suman, The backends already dynamically link to libecal. Libical is not dynamically linked with the patch. So what I have understood now is that mapi provider needs to link to libecal and not just libical and it needs some part of the parsing code to be shared between calendar backend and mailer provider. So the probable solution might be to have a directory something like groupware providers under which calendar, mailer and addressbook code can reside and share some utility code, if you want the provider code to reside under EDS. Else, have the mapi provider as a external plugin. So moving libical to top-level directory is not needed anymore. We can prolly use this bug to track the merging of libical changes to mainstream and removing the fork from GNOME.
(In reply to comment #20) > Is there any link to see the latest svn sources maintained upstream ? > > Jeff, thanks a lot for co-ordinating the effort!!! > You might try http://freeassociation.svn.sourceforge.net/svnroot/freeassociation/ Or simply svn co the tree from that same location and browse it on your own computer. We would very much like to see Evolution (and other projects) switch to upstream libical, so we can avoid needless duplication of effort in gathering and merging patches from half a dozen different projects.
FYI: upstream libical 0.32 has been released.
Created attachment 123927 [details] [review] upstream-libical-for-e-d-s.diff Changes required in e-d-s to compile with libical upstream.
(In reply to comment #23) > FYI: upstream libical 0.32 has been released. In the above patch, I've use 0.42 as the minimum required version. I can't seem to find a tarball anywhere (needed when proposing as an external dep for GNOME). Can you point me to the tarball?
Created attachment 123934 [details] [review] upstream-libical-for-evo.diff Changes required in evolution to compile with libical upstream.
Created attachment 123936 [details] [review] upstream-libical-for-exch.diff Changes required in evolution-exchange to compile with libical upstream.
Created attachment 123937 [details] [review] upstream-libical-for-mapi.diff Changes required in evolution-mapi to compile with libical upstream.
Good job Suman. Chen can we have a review at this, so that, if fine, we can go ahead and take up with upstream libical and removing the fork?
Created attachment 123938 [details] [review] libical-no-abort-on-icalerror.diff The only change I needed to make in libical was this patch. Need a review from the upstream maintainers for this one. (trivial patch though :-) ) Bharath would be submitting another patch which would take care of issues when building libical on Windows.
These were the 3 patches in the fork that will help building it on Windows. http://tinyurl.com/5nfayy http://tinyurl.com/5nh46w http://tinyurl.com/6ge9se Will report if any more issues.
Suman: it is not necessary to patch libical to achieve that functionality. libical exports the global symbol "icalerror_errors_are_fatal" which it is legal to change from your application. Somewhere in e-d-s's initialization routines, just do icalerror_errors_are_fatal = 0; and you will achieve the same effect. We would welcome a discussion on the libical mailing list regarding whether the default value is the appropriate one.
Created attachment 123992 [details] [review] evolution-proc-no-abort-on-icalerror.diff Set icalerror_errors_are_fatal = 0 at runtime for all 4 processes - e-d-s, evolution, exchange-storage and alarm-notify.
comment #33: > Set icalerror_errors_are_fatal = 0 at runtime for all 4 processes - e-d-s, > evolution, exchange-storage and alarm-notify. What about other programs using libecal? Do they also have to set this? This looks like a change of the interface semantic. Wouldn't it be better to make this part of the libecal initialization? This disables the usages of the "fatal" mode in upstream libical, but that's a limitation that libecal users have to live with.
Created attachment 124495 [details] [review] evolution-cal-no-abort-on-icalerror.diff (In reply to comment #34) > What about other programs using libecal? Do they also have to set this? > > This looks like a change of the interface semantic. Wouldn't it be better to > make this part of the libecal initialization? Yes.. I assumed e-d-s would be running anyway.. my bad :|
The patches looks good. On the last two patches which set the icalerror_errors_are_fatal. I would like the change to be in libical. It would be good to have this as a configurable option in libical with the right default value. Suman, please start a discussion in the libical mailing list regarding the same. Else we would have to set this in all processes and also in ECal and ECalComponent as we use a both libecal and libical api's.
@comment #25: the official 0.42 libical .tar.gz is available via SF: http://sourceforge.net/project/showfiles.php?group_id=16077 @comment #36: we discussed the icalerror_errors_are_fatal with the upstream developers and came to the conclusion that it is okay to leave it at fatal for upstream libical and change it to non-fatal in libecal. A remark should be added to a suitable document (if there is nothing better I suggest the libecal changelog) that warns GNOME developers who want to migrate away from libecal to libical about the difference. Suman, when (time and Evolution release) do you think you can finalize the transition?
Patrick, I need to ask for external dependency on the release-team for libical and once they approve that, we can do it immediately. My bad, I have been on vacation since a week and delayed the request. I would do it today/tomorrow. Thanks.
Created attachment 125464 [details] [review] Win32 porting * src/libical/icalrecur.c: No need to define intptr_t on Win32, it is defined in the mingw headers. * src/libical/vsnprintf.c: No reason not to include config.h also on Win32. * src/libical/icaltz-util.c: Implement byteorder macros on Win32. I use MinGW for porting. and this patch fixed the issues it complained of. Let me know if it misses something
Which version of libical do I need, exact 0.42 or 0.42 and higher?
(In reply to comment #40) > Which version of libical do I need, exact 0.42 or 0.42 and higher? I would suggest 0.43 http://downloads.sourceforge.net/freeassociation/libical-0.43.tar.gz That is what we will be using when we drop the fork. FYI - The GNOME r-t will be meeting this weekend to discuss proposals for new external dependencies. If libical is approved as an external dep, the next unstable release _will_ depend on libical-0.43
According to http://mail.gnome.org/archives/desktop-devel-list/2009-January/msg00064.html you proposed 0.42. Please clarify as follow-up on d-d-l, not here. Thanks.
The libical fork in evolution-data-server has been dropped. Relevant commits: + e-d-s http://svn.gnome.org/viewvc/evolution-data-server?view=revision&revision=9958 + evo http://svn.gnome.org/viewvc/evolution?view=revision&revision=37113 + exch http://svn.gnome.org/viewvc/evolution-exchange?view=revision&revision=1859 + mapi http://svn.gnome.org/viewvc/evolution-mapi?view=revision&revision=72 The changes made to the above modules include using the "_r" counterpart for the following APIs: + icalproperty_as_ical_string () + icalvalue_as_ical_string () + icalcomponent_as_ical_string () + icalparameter_as_ical_string () + icaldurationtype_as_ical_string () + icalenum_reqstat_code () + icallangbind_property_eval_string () + icallangbind_quote_as_ical () + icalmime_text_end_part () + icalperiodtype_as_ical_string () + icalproperty_enum_to_string () + icalproperty_get_parameter_as_string () + icalproperty_get_value_as_string () + icalproperty_get_property_name () + icalrecurrencetype_as_string () + icaltime_as_ical_string () + icalreqstattype_as_string () + icalvalue_binary_as_ical_string () + icalvalue_int_as_ical_string () + icalvalue_utcoffset_as_ical_string () + icalvalue_string_as_ical_string () + icalvalue_recur_as_ical_string () + icalvalue_text_as_ical_string () + icalvalue_attach_as_ical_string () + icalvalue_duration_as_ical_string () + icalvalue_date_as_ical_string () + icalvalue_datetime_as_ical_string () + icalvalue_float_as_ical_string () + icalvalue_geo_as_ical_string () + icalvalue_datetimeperiod_as_ical_string () + icalvalue_period_as_ical_string () + icalvalue_trigger_as_ical_string () + icalvalue_as_ical_string () Goes without saying that patch reviews should advise of these changes where needed. The changes made to the modules ensure that there is no double-free'ing of memory. However, where applicable, we can switch back to original API (without the "_r" suffix) _and_ remove the "free that portion of memory" code. I will commit another patch to all the above modules (and update this bug itself) which will enable ical_errors_are_fatal during runtime. This mode will help identify other bugs in the code. It will be configurable and be disabled by default. However, I would suggest all developers to compile with fatality enabled :-) Thanks to everyone who helped in dropping the GNOME fork of libical. Happy coding :-)
(In reply to comment #43) > The libical fork in evolution-data-server has been dropped. Relevant commits: > > + e-d-s > http://svn.gnome.org/viewvc/evolution-data-server?view=revision&revision=9958 > + evo http://svn.gnome.org/viewvc/evolution?view=revision&revision=37113 > + exch > http://svn.gnome.org/viewvc/evolution-exchange?view=revision&revision=1859 > + mapi http://svn.gnome.org/viewvc/evolution-mapi?view=revision&revision=72 Some more D-Day build failures reported and fixed by Milan Crha in these commits: + evo http://svn.gnome.org/viewvc/evolution?view=revision&revision=37116 + mapi http://svn.gnome.org/viewvc/evolution-mapi?view=revision&revision=73
(In reply to comment #43) > The libical fork in evolution-data-server has been dropped. Relevant commits: Another build failure reported and fixed by Ross Burton in this commit: + e-d-s http://svn.gnome.org/viewvc/evolution-data-server?view=revision&revision=9963