GNOME Bugzilla – Bug 727826
check: update internal libcheck copy from 0.9.8 to 0.9.12
Last modified: 2014-12-18 10:33:20 UTC
The current version of libcheck is 0.9.12. The internal libcheck in gstreamer ought to be updated.
Created attachment 273799 [details] [review] Update internal libcheck to 0.9.12 gst-indented libcheck 0.9.12, applied gstreamer-specific patches and copied it to libs/gst/check and made it build.
Hmm, we should push this. Would it make sense to have the 'gstreamer-specific patches' as patches in the codebase to ease updating?
Yes, we can do it in 1.5.x , after careful review please. Didn't see any pressing need to get it in for 1.4
Note that there is also bug #712724. Can someone merge these two and provide a grand unified patch? :)
I took a serious whack at this one and did a patch-series to ease review.
Created attachment 290821 [details] [review] Proposed patch 1/4.
Created attachment 290822 [details] [review] Proposed patch 2/4.
Created attachment 290823 [details] [review] Proposed patch 3/4.
Created attachment 290824 [details] [review] Proposed patch 4/4.
These four patches have been compile tested on Linux (Debian/testing), Windows 7 and OSX 10.9.5 using cerbero. On Linux all tests PASS when run normally, 3 fail when run using CK_FORK=no (but they fail prior to this patch set). No additional failures seem to appear when running in valgrind with or without CK_FORK=no. I ran all tests in -base, -good, -bad, -ugly and rtsp-server too and nothing new seems to fail after this patch set. Both non-Linux architectures need additional patches to actually run the check tests (win7 lacks sleep() in unistd.h which the elements/multiqueue test uses, while osx linker complains if it is linking using -pthreads as outlined in m4/ax-pthreads.m4), but after applying these several most tests in GStreamer core PASS. I never tested valgrind on either of these architectures.
Created attachment 290853 [details] [review] Proposed patch 4/4. Noticed that I used the wrong kind of comment token in configure.ac, but this is now fixed.
Created attachment 291413 [details] [review] Proposed patch 4/4. Updated according to slomo's review comments: unnecessary arguments to AX_PTHREAD() and comment to indicate why arguments cannot be used.
Patches 1+2 should be squashed IMHO, I don't see the point in keeping them separate. Same for all the trailing white space stuff underneath libcheck/* from Patch 3 that should just be squashed into the same commit. Patch 3: the changes in libs/gst/check/gstcheck.[ch] (gstreamer code) should be separate from those in libcheck/*. The copyright fix you can probably just re-apply with 'git cherry-pick $previous_commit' :) Not sure if we need to fix up trailing white spaces in gstreamer headers/code as part of this? (I'm sure there are more..) Patch 4: - All that clutter in configure.ac should probably be moved into m4/check-checks.m4 ? - test "x$ac_cv_func_clock_gettime" == "xyes" : this should be a single = for compatibility reasons IIRC - why is "ASSERT_{CRITICAL,WARNING}() calls new _ck_assert_failed() instead of _fail_unless()" part of this patch and not a separate patch? (or done at all? is it cleanup? does _fail_unless() not work any more?) - #include <stdint.h> -> #include "_stdint.h" ?
> Patches 1+2 should be squashed IMHO, I don't see the point in keeping them > separate. Same for all the trailing white space stuff underneath libcheck/* > from Patch 3 [details] that should just be squashed into the same commit. Patches 1+2 have been squashed. > Patch 3 [details]: the changes in libs/gst/check/gstcheck.[ch] (gstreamer > code) should be separate from those in libcheck/*. The copyright fix you can > probably just re-apply with 'git cherry-pick $previous_commit' :) Not sure if > we need to fix up trailing white spaces in gstreamer headers/code as part of > this? (I'm sure there are more..) I skipped all the other whitespace fixes are you adviced and also included the FSF update in the 1+2 patch. > Patch 4 [details]: > - All that clutter in configure.ac should probably be moved into > m4/check-checks.m4 ? that makes sense given AG_GST_CHECK_CHECKS which I wasn't aware of. I moved as much stuff as possible into that macro, but I still need to muck about with that AX_PTHREAD thingy in order to get HAVE_PTHREAD accessible in the checks in m4/check-checks.m4 > - test "x$ac_cv_func_clock_gettime" == "xyes" : this should be a single = for > compatibility reasons IIRC sh's test used = for equality testing, not ==. I know this. :-/ > - why is "ASSERT_{CRITICAL,WARNING}() calls new _ck_assert_failed() instead > of _fail_unless()" part of this patch and not a separate patch? (or done at > all? is it cleanup? does _fail_unless() not work any more?) no, it doesn't work any more. in check 0.9.9 _fail_unless() was replaced by the new _ck_assert_msg() that in check 0.9.12 was renamed _ck_assert_failed(). I updated the commit message with this information to give more context as none of it was obvious as you aptly point out in the review comment. I wanted to combine all the gstreamer-specific changes into a single commit so it is easy to find those the next time check is updated so this is my reasoning for wanting to keep it together with the other changes. > - #include <stdint.h> -> #include "_stdint.h" ? ah, the good old AX_CREATE_STDINT_H that creates the _stdint.h, who knew of it? fixed.
Created attachment 291779 [details] [review] Proposed patch 1/2.
Created attachment 291780 [details] [review] Proposed patch 2/2.
Merged, let's see if the CI is also happy with this. Thanks for the patches and your patience :)
Apparently there is something wrong with the patches as they are. This one appears to complain about CoreServices.h being missing. That is indeed the case for iOS, but not for OSX in general: https://ci.gstreamer.net/job/cerbero-ios-universal-7.1/1402/console These appear to complain about a missing _stdint.h. slomo https://ci.gstreamer.net/job/cerbero-osx-x86-64-1010/1125/console https://ci.gstreamer.net/job/cerbero-fedora-20/1668/console https://ci.gstreamer.net/job/cerbero-android/2255/
Created attachment 292241 [details] [review] Proposed fix for problematic iOS. This has been tested on OSX only. It seems to work fine, but I have not been able to test for iOS.
Created attachment 292250 [details] [review] Proposed fix for indirect inclusion of _stdint.h from gstcheck.h. In order to avoid requiring every project that uses gstcheck to add AX_CREATE_STDINT_H to their configure.ac. I hope this is ok, it is kind of an ugly hack. :-/
Comment on attachment 292250 [details] [review] Proposed fix for indirect inclusion of _stdint.h from gstcheck.h. This was pushed by Thibault it seems. I think it's fine.
*** Bug 712724 has been marked as a duplicate of this bug. ***