libical build improvement

[RJVB (René Bertin)]

As reported on the mailing list, I ran into a libical-related issue when trying to get ktimetracker to work (component of the kdepim4 port). In fact, that process ran into a deadlock when trying to obtain timezone data.

At first I thought a libical background thread was missing because of incomplete initialisation (or exited prematurely). However, tracing code execution on Linux, I noticed that libical was built using BUILTIN_TZDATA on OS X, but not on Linux, presumably leading to icaltimezone_load_builtin_timezone() calling itself and pthread_mutex_lock recursively. Further investigation showed that on Linux (Ubuntu 14.04 LTS), libical is built using cmake, and also includes a timezone related patch.

In this ticket, I include patches to the Portfile to build libical using cmake, which effectively prevents BUILTIN_TZDATA from being defined. I included the timezone crash preventing patch, plus one that ensures that the mutex used by icaltimezone_load_builtin_timezone() is initialised as a recursive mutex, supporting multiple locks by the same thread.

With these patches, ktimetracker is now able to load .ics files correctly.

The patches also change the dylib version naming and the embedded version information to more appropriate values, requiring a rebuild of kdepimlibs4 (something MacPorts couldn't handle on my system because that port is a dependency?!).

[kurthindenburg (Kurt Hindenburg)]

Cc Me!

[kurthindenburg (Kurt Hindenburg)]

This project appears to have been moved to ​https://github.com/libical/libical - just curious, have you check to see if any of the issues have been fixed there?

[RJVB (René Bertin)]

I just did: the fix_timezone patch still applies, suggesting that that particular issue hasn't been addressed. Which isn't particularly amazing; the version number (1.0.0) hasn't changed, and I think the Ubuntu maintainers would have caught up with it if it had.

The same applies for my own patch (making the mutex recursive), but more importantly, the switch to using the cmake build system is specific to MacPorts. The stuff in git (commit comments included) do suggest that the autoconf/configure-based build system should no longer be used.

[kurthindenburg (Kurt Hindenburg)]

Ok, the way they are distributing this is annoying - I wish they would release a new version on github

​https://github.com/libical/libical/issues/168 - for the mutex issue

​http://sourceforge.net/p/freeassociation/bugs/88/ Keith P has a large patch to handle the timezone crash

[winter@…]

Hi, I am the maintainer of libical. We are planning a new libical release within the next couple weeks and I was wondering (hoping) some of you Mac gurus could test it and see if these patches are still needed or not. I'd like to see both system and builtin tzdata cases operational.

The repo is at ​http://github.com/libical/libical and I'd be most grateful if you could test out the master branch.

If I can help out, let me know. I do have an old, slow Mac where I can at least do builds, but not much real testing. -Allen

[mkae (Marko Käning)]

Cc Me!

[kurthindenburg (Kurt Hindenburg)]

What's the status of the 2 bugs comment:5?

I don't know if these have always failed as I haven't tested previous versions:

2/6 Test #2: recur ............................***Failed    0.01 sec
recur: can't open file ../../test-data/recur.txt

Strange as the file is there

    Start 4: testvcal
4/6 Test #4: testvcal .........................***Exception: SegFault  0.35 sec

[RJVB (René Bertin)]

With the version currently in MacPorts (and with my patches), I get only 2 tests:

Running tests...
/Volumes/Debian/MP6/bin/ctest --force-new-ctest-process 
Test project _site-ports_devel_libical/libical/work/libical-1.0
    Start 1: regression
1/2 Test #1: regression .......................   Passed    0.19 sec
    Start 2: timezones
2/2 Test #2: timezones ........................   Passed   31.56 sec

100% tests passed, 0 tests failed out of 2

[RJVB (René Bertin)]

I confirm, with the git/master version ... but with the previous version installed. @ khindenburg, did you *install* the new version before running the tests? If not, the test binaries will pick up the previous library versions, and that's likely to explain the crash...

Running tests...
ctest --force-new-ctest-process 
Test project libical/libical-git-1.0.1
    Start 1: regression
1/6 Test #1: regression .......................***Failed    0.22 sec
    Start 2: recur
2/6 Test #2: recur ............................***Failed    0.00 sec
    Start 3: testmime
3/6 Test #3: testmime .........................   Passed    0.00 sec
    Start 4: testvcal
4/6 Test #4: testvcal .........................***Exception: SegFault  0.68 sec
    Start 5: process
5/6 Test #5: process ..........................   Passed    0.01 sec
    Start 6: timezones
6/6 Test #6: timezones ........................   Passed   35.63 sec

50% tests passed, 3 tests failed out of 6

Total Test time (real) =  36.55 sec

The following tests FAILED:
          1 - regression (Failed)
          2 - recur (Failed)
          4 - testvcal (SEGFAULT)
Errors while running CTest

Process:         testvcal [75532]
Path:            libical/libical-git-1.0.1/src/test/testvcal
Identifier:      testvcal
Version:         ??? (???)
Code Type:       X86-64 (Native)
Parent Process:  ctest [75528]

Date/Time:       2014-09-22 23:42:09.278 +0200
OS Version:      Mac OS X 10.6.8 (10K549)
Report Version:  6
Sleep/Wake UUID: AF061EA4-20CB-4BD0-A593-6B414C5AC951

Interval Since Last Report:          1123910 sec
Crashes Since Last Report:           65
Per-App Crashes Since Last Report:   1
Anonymous UUID:                      BFBD41F3-F309-496D-A0BD-6F8D89D7F1C7

Exception Type:  EXC_BAD_ACCESS (SIGSEGV)
Exception Codes: KERN_INVALID_ADDRESS at 0x0000000000000008
Crashed Thread:  0  Dispatch queue: com.apple.main-thread

Thread 0 Crashed:  Dispatch queue: com.apple.main-thread
0   libicalvcal.1.dylib           	0x00000001000a22a0 vObjectName + 0
1   libicalvcal.1.dylib           	0x00000001000a1ec5 icalvcal_convert_with_defaults + 21
2   testvcal                      	0x0000000100002ec3 main + 35 (testvcal.c:59)
3   testvcal                      	0x0000000100002e94 start + 52

Thread 0 crashed with X86 Thread State (64-bit):
  rax: 0x0000000000000000  rbx: 0x0000000000000000  rcx: 0x0000000000000000  rdx: 0x0000000000000000
  rdi: 0x0000000000000000  rsi: 0x0000000000000000  rbp: 0x0000000000000000  rsp: 0x00007fff5fbfe4e8
   r8: 0x0000000000000000   r9: 0x0000000000000004  r10: 0x00000001000afa2b  r11: 0x00000001000a1fa0
  r12: 0x0000000000000000  r13: 0x0000000000000000  r14: 0x0000000000000000  r15: 0x0000000000000000
  rip: 0x00000001000a22a0  rfl: 0x0000000000010206  cr2: 0x0000000000000008

[RJVB (René Bertin)]

Apply this patch

diff --git src/libicalvcal/icalvcal.c src/libicalvcal/icalvcal.c
index 5263d0e..e046e0f 100644
--- src/libicalvcal/icalvcal.c
+++ src/libicalvcal/icalvcal.c
@@ -171,11 +171,15 @@ icalcomponent* icalvcal_convert_with_defaults (VObject *object,
 					       icalvcal_defaults *defaults)
 {
 
-   char* name =  (char*)vObjectName(object);
+   char* name;
    icalcomponent* container;
    icalcomponent* root;
    icalproperty *prop;
 
+   if( !object ){
+       return NULL;
+   }
+   name =  (char*)vObjectName(object);
    icalerror_check_arg_rz( (object!=0),"Object");
 
    container = icalcomponent_new(ICAL_XROOT_COMPONENT);
diff --git src/test/testvcal.c src/test/testvcal.c
index f984674..518308c 100644
--- src/test/testvcal.c
+++ src/test/testvcal.c
@@ -44,7 +44,7 @@ int main(int argc, char* argv[])
     char* file;
 
     if (argc != 2){
-        file = "../../test-data/user-cal.vcf";
+        file = "../test-data/user-cal.vcf";
     } else {
         file = argv[1];
     }

and then do env DYLD_LIBRARY_PATH=<libical-build-dir>/lib make test and the testvcal crash should have disappeared.

NB: using assert statements to check against null pointers is nice, but ineffective when building in release mode. It's also not a very elegant way of handling errors in a shared library ...

[ksuther@…]

Those two tests are failing because libical is expecting to be built in a subdirectory when the tests are run:

mkdir build && cd build && cmake .. && make && make test

[RJVB (René Bertin)]

Portfile adapted and tested to use cmake.out_of_source yes.

[mkae (Marko Käning)]

Thanks for the info. Will deal with it later.

[mkae (Marko Käning)]

BTW, have these patches been reported upstream already?

[RJVB (René Bertin)]

... ​https://trac.macports.org/ticket/44469#comment:5 ...

[winter@…]

See commit 381029504deb0c7062b649badc9290b35a564b5f in github.com/libical/libical

I committed both patches on this issue to the official libical which will be included with the next release 2.0 due soon. Appreciate macports folks testing the master branch and see if it works now without any patching.

[winter@…]

Heck. On my old Macbook pro running Mavericks and clang 5, I don't even see PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP defined in /Volumes/Xcode/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.9.sdk/usr/include

what pthread.h are you using?

[RJVB (René Bertin)]

No, it's not defined in the platform SDKs, it's in /usr/include/c++/4.2.1/bits/gthr*.h . mdfind -name PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP suggested that, and an fgrep confirmed it.

(I won't be able to test git/master anytime really soon, sorry)

[minfrin]

Just ran into the same problem, is this fix likely to be useable soon?

[RJVB (René Bertin)]

I've updated the Portfile diff to provide a port:libical-devel port following libical git. Using it only requires editing dependent ports to use path:lib/libical.dylib:libical or maybe better, path:lib/libical.1.dylib:libical instead of port:libical.

[mkae (Marko Käning)]

Done in r142531.

[mkae (Marko Käning)]

Ooops, forgot to add the patch files. Done in r142532.

But, it still fails for the libical-devel support!

[mkae (Marko Käning)]

OK, another try in r142533. :)

Would be nice if one could also get version 1.0.1 to build properly, though, as it still fails like this:

:info:build /opt/local/var/macports/build/_Users_marko_WC_MacPorts_ports_devel_libical/libical/work/libical-1.0.1/src/li
bical/icaltimezone.c:53:40: error: use of undeclared identifier 'PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP':info:build static pthread_mutex_t builtin_mutex = PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP;
:info:build                                        ^
:info:build [ 36%] Building C object src/libical/CMakeFiles/ical-static.dir/icaltimezone.c.o

[RJVB (René Bertin)]

I don't understand, it builds perfectly for me (and I checked with the Portfile from svn)

Moreover, the error you cite doesn't seem to correspond to the 1.0.1 source:

  38 
  39 #if defined(HAVE_PTHREAD)
  40 #include <pthread.h>
  41 #if defined(PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP)
  42 // It seems the same thread can attempt to lock builtin_mutex multiple times
  43 // (at least when using builtin tzdata), so make it builtin_mutex recursive:
  44 static pthread_mutex_t builtin_mutex = PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP;
  45 #else
  46 static pthread_mutex_t builtin_mutex = PTHREAD_MUTEX_INITIALIZER;
  47 #endif
  48 #endif
  49 
  50 #if defined(_WIN32)
  51 #if !defined(_WIN32_WCE)
  52 #include <mbstring.h>
  53 #endif
  54 #include <windows.h>
  55 #endif
  56 

Are you sure you did a port clean and everything, and were using the correct source directory?

[mkae (Marko Käning)]

In r142556 I've taken out the patches for the 1.0.1 now and it finally worked fine. :)

[RJVB (René Bertin)]

Indeed, they are counterproductive with 1.0.1 ...

What I don't understand is how they could have gotten applied on your end, with the Portfile from svn ...

[mkae (Marko Käning)]

Well, I simply changed the github verison of libical manually in a local version (not in the committed one) but forgot to omit the patches.

I wasn't talking about libical-devel's version 1.0.1-*!