GNOME Bugzilla – Bug 734351
Support giflib-5.1.0
Last modified: 2014-09-19 09:22:34 UTC
giflib-5.1.0 breaks the API as noted here: http://fossies.org/linux/giflib/NEWS This breaks compilation if tracker is compiled with --enable-libgif Easy hack to fix compilation: diff --git a/src/tracker-extract/tracker-extract-gif.c b/src/tracker-extract/tracker-extract-gif.c index 43ba34a..b8a6423 100644 --- a/src/tracker-extract/tracker-extract-gif.c +++ b/src/tracker-extract/tracker-extract-gif.c @@ -680,10 +680,11 @@ tracker_extract_get_metadata (TrackerExtractInfo *info) g_free (uri); - if (DGifCloseFile (gifFile) != GIF_OK) { #if GIFLIB_MAJOR < 5 + if (DGifCloseFile (gifFile) != GIF_OK) { print_gif_error (); #else /* GIFLIB_MAJOR < 5 */ + if (DGifCloseFile (gifFile, &err) != GIF_OK) { gif_error ("Could not close GIF file", gifFile->Error); #endif /* GIFLIB_MAJOR < 5 */ }
(In reply to comment #0) > giflib-5.1.0 breaks the API as noted here: > http://fossies.org/linux/giflib/NEWS > > This breaks compilation if tracker is compiled with --enable-libgif > > Easy hack to fix compilation: > > diff --git a/src/tracker-extract/tracker-extract-gif.c > b/src/tracker-extract/tracker-extract-gif.c > index 43ba34a..b8a6423 100644 > --- a/src/tracker-extract/tracker-extract-gif.c > +++ b/src/tracker-extract/tracker-extract-gif.c > @@ -680,10 +680,11 @@ tracker_extract_get_metadata (TrackerExtractInfo *info) > > g_free (uri); > > - if (DGifCloseFile (gifFile) != GIF_OK) { > #if GIFLIB_MAJOR < 5 > + if (DGifCloseFile (gifFile) != GIF_OK) { > print_gif_error (); > #else /* GIFLIB_MAJOR < 5 */ > + if (DGifCloseFile (gifFile, &err) != GIF_OK) { > gif_error ("Could not close GIF file", gifFile->Error); > #endif /* GIFLIB_MAJOR < 5 */ > } Nice catch, please go ahead and commit this patch! Let me know if you need someone or me to commit it for you.
I have no commit access (and don't really do any gnome development), so someone else has to commit it. However, it should be tested better (e.g. by someone with giflib-5.0). I think the patch should also check for GIFLIB_MINOR >= 1
(In reply to comment #2) > I have no commit access (and don't really do any gnome development), so someone > else has to commit it. > > However, it should be tested better (e.g. by someone with giflib-5.0). I think > the patch should also check for GIFLIB_MINOR >= 1 Hi Martin, I just tested Tracker with giflib 4.1.6 and 5.0.4 and without your patch here and things work fine it seems. I wonder if you're using an older version of Tracker, because I know we've patched this area recently. What version are you testing with?
Hi Martyn, The API change wasn't introduced until giflib-5.1, so it is expected to work without my patch for 5.0.y and 4.1.y. I didn't test my patch with giflib-4.x or giflib-5.0.y, but I would expect at least the latter case to fail, as I misused the existing #ifdef's, which only checks the major versions. Makes sense? // Martin
(In reply to comment #4) > Hi Martyn, Hi, > The API change wasn't introduced until giflib-5.1, so it is expected to work > without my patch for 5.0.y and 4.1.y. I checked out the git repo for gitlib and I am building master, so should I test with a branch called giflib-5.1? > I didn't test my patch with giflib-4.x or giflib-5.0.y, but I would expect at > least the latter case to fail, as I misused the existing #ifdef's, which only > checks the major versions. The existing #ifdefs do check minor versions too, which is why I wondered if you had the latest Tracker to test with, we didn't do this before. > Makes sense? Yes thanks. I assumed "master" was the latest work...
> (In reply to comment #4) >> Hi Martyn, > > Hi, > >> The API change wasn't introduced until giflib-5.1, so it is >> expected to work >> without my patch for 5.0.y and 4.1.y. > > I checked out the git repo for gitlib and I am building master, so > should I > test with a branch called giflib-5.1? Hmm, I actually just used my distro package[1]. When looking at the git repo, I see the change was introduced in commit 116179 [2], so master/HEAD should do. [1] https://www.archlinux.org/packages/extra/x86_64/giflib/ [2] http://sourceforge.net/p/giflib/code/ci/116179a7d4681dda9afcdd43b5fbfb391b44918b >> I didn't test my patch with giflib-4.x or giflib-5.0.y, but I would >> expect at >> least the latter case to fail, as I misused the existing #ifdef's, >> which only >> checks the major versions. > > The existing #ifdefs do check minor versions too, which is why I > wondered if > you had the latest Tracker to test with, we didn't do this before. As far as I can tell from the tracker code [3] and the giflib code [4], the #ifdef's only check major. And when looking at the giflib news/commits, I get confused about when the DGifCloseFile() actually frees the gifFile struct, and when it does not. But I didn't really look that much into it. [3] https://git.gnome.org/browse/tracker/tree/src/tracker-extract/tracker-extract-gif.c#n684 [4] http://sourceforge.net/p/giflib/code/ci/master/tree/lib/gif_lib.h#l14 // Martin
*** Bug 736328 has been marked as a duplicate of this bug. ***
Created attachment 285756 [details] [review] Patch suggestion - supports build with giflib < 5.1 (old API) and >= 5.1 (new API) This patch is what we currently have on the openSUSE packages of tracker 1.1.4
Setting GNOME 3.14 target as tracker is broken at the moment in systems using latest stable giflib (5.1)
(In reply to comment #8) > Created an attachment (id=285756) [details] [review] > Patch suggestion - supports build with giflib < 5.1 (old API) and >= 5.1 (new > API) > > This patch is what we currently have on the openSUSE packages of tracker 1.1.4 Have you tested this patch works with both versions of giflib? I've been unable to do that so far. (In reply to comment #9) > Setting GNOME 3.14 target as tracker is broken at the moment in systems using > latest stable giflib (5.1) I've been hesitating due to my inability to test this patch myself. If Dominique can give me assurance it works and you need this for the 3.14 release Javier, I can apply it in time for the release this week if I can break the hard code freeze ;)
(In reply to comment #10) > (In reply to comment #8) > > Created an attachment (id=285756) [details] [review] [details] [review] > > Patch suggestion - supports build with giflib < 5.1 (old API) and >= 5.1 (new > > API) > > > > This patch is what we currently have on the openSUSE packages of tracker 1.1.4 > > Have you tested this patch works with both versions of giflib? I've been unable > to do that so far. I (build) tested the patch against giflib 5.0 and also 5.1 (which is what I currently run) > (In reply to comment #9) > > Setting GNOME 3.14 target as tracker is broken at the moment in systems using > > latest stable giflib (5.1) > > I've been hesitating due to my inability to test this patch myself. If > Dominique can give me assurance it works and you need this for the 3.14 release > Javier, I can apply it in time for the release this week if I can break the > hard code freeze ;) Uhh.. assurance... such a legal term - what are you suing me for if it breaks? Other than that: as said: it's tested against old and new
(In reply to comment #11) > Uhh.. assurance... such a legal term - what are you suing me for if it breaks? Ha! I just prefer to know patches have been tested before applying them. Depending on the contributor, that's quite variable. Suing? no... just some to point the finger at if we get complaints :P > Other than that: as said: it's tested against old and new Great, Javier, are you the right person to check with on the hard code freeze or should I fire off an email to the release team?
Please fire off, for the sake of transparency and records of decisions.
Comment on attachment 285756 [details] [review] Patch suggestion - supports build with giflib < 5.1 (old API) and >= 5.1 (new API) Committed now we have release team approval. Thanks for the patch! :)
This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report. Thanks all.