On 2013-03-15 04:10, Andrew Hughes wrote:
----- Original Message -----
Hi,
Updated webrev at:
http://cr.openjdk.java.net/~omajid/webrevs/system-giflib/01/
The webrev borrows some idioms that Andrew Hughes pointed out, and
uses
system headers for giflib too.
I checked this by building --with-libgif=system and also by removing
the
src/share/native/sun/awt/giflib dir and using --with-giflib=system.
This looks better, though I'd still like to see it using a more
standard --enable format. That then removes the need to have that
third error case. I'll post a webrev to fix both this and zlib once
committed.
Agree, ok from me.
On 03/08/2013 12:26 PM, Phil Race wrote:
If I understand correctly, this removes the directory containing
the JDK's copy of giflib sources from the set of locations to be
compiled etc, and replaces it with just a link line pointer to use
"libgif" which is then expected to be on the default linker path,
ie in /usr/lib.
I think this is fine except I would guard USE_EXTERNAL_LIBGIF
with an expectation that this is at least "not" windows, since
I'm pretty sure that option would always be a mistake there.
The updated webrev does that. I don't have a windows machine to test
this on, unfortunately.
This does cause some duplication. Is this really a major issue? It's
not the default and, if I was someone on Windows trying to get system
giflib working, this would just get in the way. We had to remove something
similar in the zlib case that blocked it if not on Mac OS X.
This part is weird to me. Sure, trying to set --with-giflib=system on
windows is wrong, but that's why configure is verifying that it's
actually there and that the toolchain is able to compile and link
against it. I just tried the patch on windows, and configure fails at
finding giflib, even if it's installed in cygwin, as expected. I do not
think we should be hand holding to this extent in the makefiles. If we
really want to guard this on windows, do it in configure, with a helpful
error message, rather then silently ignoring the option in make. But as
pointed out, it's really not necessary.
/Erik
