bug#17790: build failure with giflib-5.1.0

bug#17790: build failure with giflib-5.1.0

[Makoto Fujiwara]

Package: emacs
Severity: important

Hi, should be already fixed, I am presume, but I needed
attached remedy to build,

Thanks a lot, 
---
Makoto Fujiwara, 
Chiba, Japan, Narita Airport and Disneyland prefecture.

$NetBSD$

mage.c:7417:7: error: too few arguments to function 'DGifCloseFile'
emacs-current/work/.buildlink/include/gif_lib.h:183:9:
note: declared here

--- ./src/image.c.orig	2014-06-14 13:15:41.000000000 +0900
+++ ./src/image.c	2014-06-15 08:39:42.000000000 +0900
@@ -7409,13 +7409,13 @@
 	}
 #endif
     }
-
+  int * return_value;
   /* Before reading entire contents, check the declared image size. */
   if (!check_image_size (f, gif->SWidth, gif->SHeight))
     {
       image_error ("Invalid image size (see `max-image-size')", Qnil, Qnil);
-      fn_DGifCloseFile (gif);
-      return 0;
+      fn_DGifCloseFile (gif, return_value);
+      return return_value;
     }
 
   /* Read entire contents.  */
@@ -7423,8 +7423,8 @@
   if (rc == GIF_ERROR || gif->ImageCount <= 0)
     {
       image_error ("Error reading `%s'", img->spec, Qnil);
-      fn_DGifCloseFile (gif);
-      return 0;
+      fn_DGifCloseFile (gif, return_value);
+      return return_value;
     }
 
   /* Which sub-image are we to display?  */
@@ -7435,8 +7435,8 @@
       {
 	image_error ("Invalid image number `%s' in image `%s'",
 		     image_number, img->spec);
-	fn_DGifCloseFile (gif);
-	return 0;
+	fn_DGifCloseFile (gif, return_value);
+	return return_value;
       }
   }
 
@@ -7453,8 +7453,8 @@
   if (!check_image_size (f, width, height))
     {
       image_error ("Invalid image size (see `max-image-size')", Qnil, Qnil);
-      fn_DGifCloseFile (gif);
-      return 0;
+      fn_DGifCloseFile (gif, return_value);
+      return return_value;
     }
 
   /* Check that the selected subimages fit.  It's not clear whether
@@ -7471,16 +7471,16 @@
 	     && 0 <= subimg_left && subimg_left <= width - subimg_width))
 	{
 	  image_error ("Subimage does not fit in image", Qnil, Qnil);
-	  fn_DGifCloseFile (gif);
-	  return 0;
+	  fn_DGifCloseFile (gif, return_value);
+	  return return_value;
 	}
     }
 
   /* Create the X image and pixmap.  */
   if (!image_create_x_image_and_pixmap (f, img, width, height, 0, &ximg, 0))
     {
-      fn_DGifCloseFile (gif);
-      return 0;
+      fn_DGifCloseFile (gif, return_value);
+      return return_value;
     }
 
   /* Clear the part of the screen image not covered by the image.
@@ -7650,7 +7650,7 @@
 			    Fcons (make_number (gif->ImageCount),
 				   img->lisp_data));
 
-  fn_DGifCloseFile (gif);
+  fn_DGifCloseFile (gif, return_value);
 
   /* Maybe fill in the background field while we have ximg handy. */
   if (NILP (image_spec_value (img->spec, QCbackground, NULL)))

bug#17790: GIFlib-5.1.0 or former conditional

[Makoto Fujiwara]

Thank you, I am now testing with following patch.
(OK with giflib-5.1.0 and 5.0.6, on NetBSD/amd64 6.1.4)

Thanks a lot,
---
Makoto Fujiwara, 
Chiba, Japan, Narita Airport and Disneyland prefecture.

$NetBSD$

mage.c:7417:7: error: too few arguments to function 'DGifCloseFile'
emacs-current/work/.buildlink/include/gif_lib.h:183:9:
note: declared here

--- src/image.c.orig	2014-06-15 09:25:05.000000000 +0900
+++ src/image.c	2014-06-17 08:46:30.000000000 +0900
@@ -7410,12 +7410,20 @@ gif_load (struct frame *f, struct image 
 #endif
     }
 
+#if (GIFLIB_MAJOR == 5) && (GIFLIB_MINOR > 0)  
+  int * return_value_p;
+#endif
   /* Before reading entire contents, check the declared image size. */
   if (!check_image_size (f, gif->SWidth, gif->SHeight))
     {
       image_error ("Invalid image size (see `max-image-size')", Qnil, Qnil);
+#if (GIFLIB_MAJOR == 5) && (GIFLIB_MINOR > 0)
+      fn_DGifCloseFile (gif, return_value_p);
+      return *return_value_p;
+#else
       fn_DGifCloseFile (gif);
       return 0;
+#endif
     }
 
   /* Read entire contents.  */
@@ -7423,8 +7431,13 @@ gif_load (struct frame *f, struct image 
   if (rc == GIF_ERROR || gif->ImageCount <= 0)
     {
       image_error ("Error reading `%s'", img->spec, Qnil);
+#if (GIFLIB_MAJOR == 5) && (GIFLIB_MINOR > 0)
+      fn_DGifCloseFile (gif, return_value_p);
+      return *return_value_p;
+#else
       fn_DGifCloseFile (gif);
       return 0;
+#endif
     }
 
   /* Which sub-image are we to display?  */
@@ -7435,8 +7448,13 @@ gif_load (struct frame *f, struct image 
       {
 	image_error ("Invalid image number `%s' in image `%s'",
 		     image_number, img->spec);
+#if (GIFLIB_MAJOR == 5) && (GIFLIB_MINOR > 0)
+      fn_DGifCloseFile (gif, return_value_p);
+      return *return_value_p;
+#else
 	fn_DGifCloseFile (gif);
 	return 0;
+#endif
       }
   }
 
@@ -7453,8 +7471,13 @@ gif_load (struct frame *f, struct image 
   if (!check_image_size (f, width, height))
     {
       image_error ("Invalid image size (see `max-image-size')", Qnil, Qnil);
+#if (GIFLIB_MAJOR == 5) && (GIFLIB_MINOR > 0)
+      fn_DGifCloseFile (gif, return_value_p);
+      return *return_value_p;
+#else
       fn_DGifCloseFile (gif);
       return 0;
+#endif
     }
 
   /* Check that the selected subimages fit.  It's not clear whether
@@ -7471,16 +7494,26 @@ gif_load (struct frame *f, struct image 
 	     && 0 <= subimg_left && subimg_left <= width - subimg_width))
 	{
 	  image_error ("Subimage does not fit in image", Qnil, Qnil);
+#if (GIFLIB_MAJOR == 5) && (GIFLIB_MINOR > 0)
+	  fn_DGifCloseFile (gif, return_value_p);
+	  return *return_value_p;
+#else
 	  fn_DGifCloseFile (gif);
 	  return 0;
+#endif
 	}
     }
 
   /* Create the X image and pixmap.  */
   if (!image_create_x_image_and_pixmap (f, img, width, height, 0, &ximg, 0))
     {
+#if (GIFLIB_MAJOR == 5) && (GIFLIB_MINOR > 0)
+      fn_DGifCloseFile (gif, return_value_p);
+      return *return_value_p;
+#else
       fn_DGifCloseFile (gif);
       return 0;
+#endif
     }
 
   /* Clear the part of the screen image not covered by the image.
@@ -7650,7 +7683,11 @@ gif_load (struct frame *f, struct image 
 			    Fcons (make_number (gif->ImageCount),
 				   img->lisp_data));
 
+#if (GIFLIB_MAJOR == 5) && (GIFLIB_MINOR > 0)
+  fn_DGifCloseFile (gif, return_value_p);
+#else
   fn_DGifCloseFile (gif);
+#endif
 
   /* Maybe fill in the background field while we have ximg handy. */
   if (NILP (image_spec_value (img->spec, QCbackground, NULL)))

bug#17790: GIFlib-5.1.0 or former conditional

[Andreas Schwab]

Makoto Fujiwara <makoto@ki.nu> writes:

> +#if (GIFLIB_MAJOR == 5) && (GIFLIB_MINOR > 0)  

What about libgif6?  Please remove the extra parens.

> +  int * return_value_p;

This is not a predicate, please remove _p.

> +#endif
>    /* Before reading entire contents, check the declared image size. */
>    if (!check_image_size (f, gif->SWidth, gif->SHeight))
>      {
>        image_error ("Invalid image size (see `max-image-size')", Qnil, Qnil);
> +#if (GIFLIB_MAJOR == 5) && (GIFLIB_MINOR > 0)
> +      fn_DGifCloseFile (gif, return_value_p);
> +      return *return_value_p;

What is the meaning of the return value?  Is it compatible with the
intented bool value?

> @@ -7650,7 +7683,11 @@ gif_load (struct frame *f, struct image 
>  			    Fcons (make_number (gif->ImageCount),
>  				   img->lisp_data));
>  
> +#if (GIFLIB_MAJOR == 5) && (GIFLIB_MINOR > 0)
> +  fn_DGifCloseFile (gif, return_value_p);

What happens with the return value?

Also, you didn't change all uses of DGifCloseFile, see line 7253:

DEF_IMGLIB_FN (int, DGifCloseFile, (GifFileType *));

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

bug#17790: GIFlib-5.1.0 or former conditional

[Eli Zaretskii]

> From: Andreas Schwab <schwab@suse.de>
> Date: Tue, 17 Jun 2014 09:05:00 +0200
> Cc: 17790@debbugs.gnu.org
> 
> Makoto Fujiwara <makoto@ki.nu> writes:
> 
> > +#if (GIFLIB_MAJOR == 5) && (GIFLIB_MINOR > 0)  
> 
> What about libgif6?  Please remove the extra parens.
> 
> > +  int * return_value_p;
> 
> This is not a predicate, please remove _p.
> 
> > +#endif
> >    /* Before reading entire contents, check the declared image size. */
> >    if (!check_image_size (f, gif->SWidth, gif->SHeight))
> >      {
> >        image_error ("Invalid image size (see `max-image-size')", Qnil, Qnil);
> > +#if (GIFLIB_MAJOR == 5) && (GIFLIB_MINOR > 0)
> > +      fn_DGifCloseFile (gif, return_value_p);
> > +      return *return_value_p;
> 
> What is the meaning of the return value?  Is it compatible with the
> intented bool value?

No, it isn't.  Also, no other image loading method returns values
specific to some image type to its caller, so I don't think it's a
good idea.

Moreover, in most of the places where we call DGifCloseFile, we don't
really care about any errors it encounters, because we are already in
the context of a more significant error.  So it is better to simply
disregard the errors there.

I propose a slightly different changeset below.  Unless someone sees
some problem with it, I will commit soon.  (I guess this should go to
the release branch, right? Stefan? Glenn?)

Finally, a question to Eric: can we rely on DGifCloseFile to accept
NULL as the second argument, like the other APIs do?  This is not
documented, but the source clearly says that the behavior is the same
as with the other APIs.

Here's the suggested patch:

=== modified file 'src/image.c'
--- src/image.c	2014-05-04 18:51:32 +0000
+++ src/image.c	2014-06-17 14:50:54 +0000
@@ -7255,7 +7255,11 @@ gif_image_p (Lisp_Object object)
 #ifdef WINDOWSNT
 
 /* GIF library details.  */
+#if 5 < GIFLIB_MAJOR + (1 <= GIFLIB_MINOR)
+DEF_IMGLIB_FN (int, DGifCloseFile, (GifFileType *, int *));
+#else
 DEF_IMGLIB_FN (int, DGifCloseFile, (GifFileType *));
+#endif
 DEF_IMGLIB_FN (int, DGifSlurp, (GifFileType *));
 #if GIFLIB_MAJOR < 5
 DEF_IMGLIB_FN (GifFileType *, DGifOpen, (void *, InputFunc));
@@ -7325,6 +7329,19 @@ gif_read_from_memory (GifFileType *file,
   return len;
 }
 
+static int
+gif_close (GifFileType *gif, int *err)
+{
+  int retval;
+
+#if 5 < GIFLIB_MAJOR + (1 <= GIFLIB_MINOR)
+  retval = fn_DGifCloseFile (gif, err);
+#else
+  retval = fn_DGifCloseFile (gif);
+  if (err)
+    *err = gif->Error;
+#endif
+}
 
 /* Load GIF image IMG for use on frame F.  Value is true if
    successful.  */
@@ -7419,7 +7436,7 @@ gif_load (struct frame *f, struct image 
   if (!check_image_size (f, gif->SWidth, gif->SHeight))
     {
       image_error ("Invalid image size (see `max-image-size')", Qnil, Qnil);
-      fn_DGifCloseFile (gif);
+      gif_close (gif, NULL);
       return 0;
     }
 
@@ -7428,7 +7445,7 @@ gif_load (struct frame *f, struct image 
   if (rc == GIF_ERROR || gif->ImageCount <= 0)
     {
       image_error ("Error reading `%s'", img->spec, Qnil);
-      fn_DGifCloseFile (gif);
+      gif_close (gif, NULL);
       return 0;
     }
 
@@ -7440,7 +7457,7 @@ gif_load (struct frame *f, struct image 
       {
 	image_error ("Invalid image number `%s' in image `%s'",
 		     image_number, img->spec);
-	fn_DGifCloseFile (gif);
+	gif_close (gif, NULL);
 	return 0;
       }
   }
@@ -7458,7 +7475,7 @@ gif_load (struct frame *f, struct image 
   if (!check_image_size (f, width, height))
     {
       image_error ("Invalid image size (see `max-image-size')", Qnil, Qnil);
-      fn_DGifCloseFile (gif);
+      gif_close (gif, NULL);
       return 0;
     }
 
@@ -7476,7 +7493,7 @@ gif_load (struct frame *f, struct image 
 	     && 0 <= subimg_left && subimg_left <= width - subimg_width))
 	{
 	  image_error ("Subimage does not fit in image", Qnil, Qnil);
-	  fn_DGifCloseFile (gif);
+	  gif_close (gif, NULL);
 	  return 0;
 	}
     }
@@ -7484,7 +7501,7 @@ gif_load (struct frame *f, struct image 
   /* Create the X image and pixmap.  */
   if (!image_create_x_image_and_pixmap (f, img, width, height, 0, &ximg, 0))
     {
-      fn_DGifCloseFile (gif);
+      gif_close (gif, NULL);
       return 0;
     }
 
@@ -7655,7 +7672,18 @@ gif_load (struct frame *f, struct image 
 			    Fcons (make_number (gif->ImageCount),
 				   img->lisp_data));
 
-  fn_DGifCloseFile (gif);
+  if (gif_close (gif, &gif_err) == GIF_ERROR)
+    {
+#if 5 <= GIFLIB_MAJOR
+      char *error_text = fn_GifErrorString (gif_err);
+
+      if (error_text)
+	image_error ("Error closing `%s': %s",
+		     img->spec, build_string (error_text));
+#else
+      image_error ("Error closing `%s'", img->spec);
+#endif
+    }
 
   /* Maybe fill in the background field while we have ximg handy. */
   if (NILP (image_spec_value (img->spec, QCbackground, NULL)))

=== modified file 'lisp/term/w32-win.el'
--- lisp/term/w32-win.el	2014-04-11 07:02:28 +0000
+++ lisp/term/w32-win.el	2014-06-17 14:51:18 +0000
@@ -251,13 +251,16 @@ This returns an error if any Emacs frame
        ;; libraries according to the version of giflib we were
        ;; compiled against.  (If we were compiled without GIF support,
        ;; libgif-version's value is -1.)
-       (if (>= libgif-version 50000)
-	   ;; Yes, giflib 5.x uses 6 as the major version of the API,
-	   ;; thus "libgif-6.dll" below (giflib 4.x used 5 as the
-	   ;; major API version).
-	   ;; giflib5.dll is from the lua-files project.
-	   '(gif "libgif-6.dll" "giflib5.dll")
-	 '(gif "libgif-5.dll" "giflib4.dll" "libungif4.dll" "libungif.dll"))
+       (if (>= libgif-version 50100)
+	   ;; Yes, giflib 5.0 uses 6 as the major version of the API,
+	   ;; and giflib 5.1 uses 7, thus "libgif-7.dll" and
+	   ;; "libgif-6.dll" below (giflib 4.x used 5 as the major API
+	   ;; version).  giflib5.dll is from the lua-files project,
+	   ;; and gif.dll is from luapower.
+	   '(gif "libgif-7.dll")
+	 (if (>= libgif-version 50000)
+	     '(gif "libgif-6.dll" "giflib5.dll" "gif.dll")
+	 '(gif "libgif-5.dll" "giflib4.dll" "libungif4.dll" "libungif.dll")))
        '(svg "librsvg-2-2.dll")
        '(gdk-pixbuf "libgdk_pixbuf-2.0-0.dll")
        '(glib "libglib-2.0-0.dll")

bug#17790: GIFlib-5.1.0 or former conditional

[Stefan Monnier]

> some problem with it, I will commit soon.  (I guess this should go to
> the release branch, right? Stefan? Glenn?)

Yes.


        Stefan

bug#17790: GIFlib-5.1.0 or former conditional

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Andreas Schwab <schwab@suse.de>,  "Eric S. Raymond" <esr@thyrsus.com>,  17790@debbugs.gnu.org
> Date: Tue, 17 Jun 2014 12:18:02 -0400
> 
> > some problem with it, I will commit soon.  (I guess this should go to
> > the release branch, right? Stefan? Glenn?)
> 
> Yes.

Committed as emacs-24 revision 17254.

bug#17790: GIFlib-5.1.0 or former conditional

[Ken Brown]

On 6/18/2014 11:17 AM, Eli Zaretskii wrote:
>> From: Stefan Monnier <monnier@iro.umontreal.ca>
>> Cc: Andreas Schwab <schwab@suse.de>,  "Eric S. Raymond" <esr@thyrsus.com>,  17790@debbugs.gnu.org
>> Date: Tue, 17 Jun 2014 12:18:02 -0400
>>
>>> some problem with it, I will commit soon.  (I guess this should go to
>>> the release branch, right? Stefan? Glenn?)
>>
>> Yes.
> 
> Committed as emacs-24 revision 17254.

This breaks the Cygwin build as follows:

image.c: In function ‘gif_close’:
image.c:7342:15: error: ‘GifFileType’ has no member named ‘Error’
     *err = gif->Error;
               ^
image.c: In function ‘gif_load’:
image.c:7675:24: error: ‘gif_err’ undeclared (first use in this function)
   if (gif_close (gif, &gif_err) == GIF_ERROR)
                        ^
image.c:7675:24: note: each undeclared identifier is reported only once for each function it appears in
image.c:7684:7: error: too few arguments to function ‘image_error’
       image_error ("Error closing `%s'", img->spec);
       ^
image.c:664:1: note: declared here
 image_error (const char *format, Lisp_Object arg1, Lisp_Object arg2)
 ^

bug#17790: GIFlib-5.1.0 or former conditional

[Eli Zaretskii]

> Date: Wed, 18 Jun 2014 12:20:38 -0400
> From: Ken Brown <kbrown@cornell.edu>
> 
> On 6/18/2014 11:17 AM, Eli Zaretskii wrote:
> >> From: Stefan Monnier <monnier@iro.umontreal.ca>
> >> Cc: Andreas Schwab <schwab@suse.de>,  "Eric S. Raymond" <esr@thyrsus.com>,  17790@debbugs.gnu.org
> >> Date: Tue, 17 Jun 2014 12:18:02 -0400
> >>
> >>> some problem with it, I will commit soon.  (I guess this should go to
> >>> the release branch, right? Stefan? Glenn?)
> >>
> >> Yes.
> > 
> > Committed as emacs-24 revision 17254.
> 
> This breaks the Cygwin build as follows:

Please try again.  (And I sincerely suggest to upgrade to a newer
version of giflib.)

bug#17790: GIFlib-5.1.0 or former conditional

[Ken Brown]

On 6/18/2014 12:36 PM, Eli Zaretskii wrote:
>> Date: Wed, 18 Jun 2014 12:20:38 -0400
>> From: Ken Brown <kbrown@cornell.edu>
>>
>> On 6/18/2014 11:17 AM, Eli Zaretskii wrote:
>>>> From: Stefan Monnier <monnier@iro.umontreal.ca>
>>>> Cc: Andreas Schwab <schwab@suse.de>,  "Eric S. Raymond" <esr@thyrsus.com>,  17790@debbugs.gnu.org
>>>> Date: Tue, 17 Jun 2014 12:18:02 -0400
>>>>
>>>>> some problem with it, I will commit soon.  (I guess this should go to
>>>>> the release branch, right? Stefan? Glenn?)
>>>>
>>>> Yes.
>>>
>>> Committed as emacs-24 revision 17254.
>>
>> This breaks the Cygwin build as follows:
>
> Please try again.  (And I sincerely suggest to upgrade to a newer
> version of giflib.)

Thanks, that fixed it.

Ken