Bug 196580 - pngconf.h defines PNG_ASSEMBLER_CODE_SUPPORTED even when it isn't (x86_64, ppc)
Summary: pngconf.h defines PNG_ASSEMBLER_CODE_SUPPORTED even when it isn't (x86_64, ppc)
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: libpng
Version: rawhide
Hardware: All
OS: Linux
high
high
Target Milestone: ---
Assignee: Matthias Clasen
QA Contact:
URL:
Whiteboard:
Depends On:
Blocks: 196010
TreeView+ depends on / blocked
 
Reported: 2006-06-25 09:11 UTC by Hans de Goede
Modified: 2007-11-30 22:11 UTC (History)
2 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2006-07-27 17:52:19 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)
patch attempting to fix this (1.33 KB, patch)
2006-07-02 13:26 UTC, Hans de Goede
no flags Details | Diff
patch fixing the reported problem (2.07 KB, patch)
2006-07-02 19:39 UTC, Hans de Goede
no flags Details | Diff
specfile patch adding the patch fixing this to the spec, including changelog entry and release bump (1007 bytes, patch)
2006-07-02 19:41 UTC, Hans de Goede
no flags Details | Diff

Description Hans de Goede 2006-06-25 09:11:40 UTC
During compilation on x86_64 libpng's configure adds
-DPNG_NO_ASSEMBLER_CODE to LIBPNG_DEFINES in the Makefile.

Unfortunatly because this is added as a commandline argument during compilation
only by configure, PNG_NO_ASSEMBLER_CODE doesnot get defined in
/usr/include/pngconf.h

PNG_NO_ASSEMBLER_CODE not being defined in turn causes /usr/include/pngconf.h
to define PNG_ASSEMBLER_CODE_SUPPORTED even on platforms where asm support is
not available.

I found this out by "convert" from ImageMagick not working, actually none of the
ImageMagick components in RawHide are currently capable of accessing png files
because of this. The png code in ImageMagick wants to call png_get_asm_flags
when compiled on platforms which define PNG_ASSEMBLER_CODE_SUPPORTED. However
libpng doesnot contain this symbol on x86_64 because it was compiled with 
-DPNG_NO_ASSEMBLER_CODE.

This is unfortunatly not detected during build time because ImageMagick has its
input format modules in .so files.

The ImageMagick problem has been reported in bug 196010.

I've set the Severity and Priority to high because "convert" is used during the
build of several FE packages to convert images for the use as desktop icons,
currently these packages fail to build because of this bug.

Last but not least this must be fixed in a multilib compatible way! I think this
could best be fixed by adding the following lines at the top of pngconf.h:

#ifndef __i386__
#define PNG_NO_ASSEMBLER_CODE
#endif

That way this file can be identical for i386 and x86_64 and do the right thing
for both archs.

Comment 1 Hans de Goede 2006-07-01 12:29:30 UTC
Erm,

Can we see a fix for this soon? Its rather anoying not to have ImageMagick
working on x86_64. Would a sane patch help?


Comment 2 Matthias Clasen 2006-07-02 11:28:37 UTC
A patch is always appreciated

Comment 3 Hans de Goede 2006-07-02 13:26:55 UTC
Created attachment 131852 [details]
patch attempting to fix this

I've come up with the attached patch, which unfortunatly does not work.

It conditionaly defines PNG_NO_ASSEMBLER_CODE when included from an
application, and it does nothing when compiling libpng. In order to determine
wether libpng is being compiled or not it uses the define PNG_CONFIGURE_LIBPNG.
So far so good.

However ./configure tries to compile a libpng file with assembler in it during
./configure to see if assembler support should be enabled. The problem is that
when ./configure does this test it doesn't define PNG_CONFIGURE_LIBPNG (it
can't because there is no config.h at that moment) thus our conditionaly
defining of PNG_NO_ASSEMBLER_CODE springs into action, which causes the compile
to succeed, so ./configure think it can use assembler. Now when the real
compile starts, PNG_CONFIGURE_LIBPNG is defined, so this patch effectivly does
nothing (as designed) however because ./configure has not detected that
assembler will fail ./configure hasn't added -DPNG_NO_ASSEMBLER_CODE to the
CFLAGS either. Thus it tries to compile the assem code on x86_64 -> error!

There are a couple of ways to fix this:
1) Don't apply our patch untill after ./configure is run.
2) Add -DPNG_CONFIGURE_LIBPNG to the compiler flags when testing
   if assembler is there while not adding -DHAVE_CONFIG_H which should
   disable our added code during the configure check too
3) Make the definining of PNG_NO_ASSEMBLER_CODE in pngconf.h only conditional
on
   the platform used and no longer on PNG_CONFIGURE_LIBPNG, and remove the then

   useless assembler check from ./configure

I personally prefer 2, with 3 being a decent alternative. Let me know what you
want and I'll brew up a patch.

Comment 4 Hans de Goede 2006-07-02 19:39:17 UTC
Created attachment 131857 [details]
patch fixing the reported problem

Improved version fixing the issue mentioned in comment #3 the issue is fixed
with method 2) described in comment #3

Comment 5 Hans de Goede 2006-07-02 19:41:12 UTC
Created attachment 131858 [details]
specfile patch adding the patch fixing this to the spec, including changelog entry and release bump

And to make life even easier here is a specfile patch adding the patch fixing
this to the spec, including changelog entry and release bump.

Comment 6 Hans de Goede 2006-07-02 19:44:36 UTC
I forgot to mention that I've recompiled libpng with these changes, installed
the new version including the new -devel and rebuild ImageMagick against the new
libpng.

With the newly build ImageMagick convert on png files works as expected (iow bug
196010 is fixed). This is all ofcourse on a x86_64 rawhide install.



Comment 7 Hans de Goede 2006-07-09 08:27:13 UTC
ping?


Comment 8 Hans de Goede 2006-07-25 10:59:27 UTC
Erm, ping again? This is a _real_ bug with a rather straightforward fix. I can
mkae an ever simpeler / cleaner patch which works around the mentioned
./configure problem in a cleaner way if you want.



Comment 9 Matthias Clasen 2006-07-27 17:52:19 UTC
Fixed in 1.2.10-6


Note You need to log in before you can comment on or make changes to this bug.