[PATCH FOR REVIEW] System Zlib Support
Hi all, In OpenJDK 8, some support has already been added for using the system installation of zlib (see the thread http://mail.openjdk.java.net/pipermail/core-libs-dev/2012-July/010967.html), which is very similar to the support we've had in IcedTea for the last five years (wow, has it really been that long?). This is great news for us, as it's less work we have to do in upstreaming the patch (though 7 still needs to be dealt with). As is, the following webrev: http://cr.openjdk.java.net/~andrew/syslibs/zlib/webrev.01/ just fixes a few minor issues to match our existing setup, and fixes a bug found when testing the existing support. In detail, the webrev: * Replaces the hardcoded use of "-lz" with $(ZLIB_LIBS) and $(ZLIB_CFLAGS), now set in make_jdk_generic_profile.sh. * Replaces "zlib.h" usage with <zlib.h> (mainly to reduce difference, searching '.' has no effect either way) * Stops uLong being defined if SYSTEM_ZLIB is set, even if we're not on Mac OS X. Without this fix, the build fails. Ok for the build forest? If so, can I please have a bug ID for this? Thanks, -- Andrew :) Free Java Software Engineer Red Hat, Inc. (http://www.redhat.com) PGP Key: 248BDC07 (https://keys.indymedia.org/) Fingerprint = EC5A 1F5E C0AD 1D15 8F1F 8F91 3B96 A578 248B DC07
On 31/07/2012 00:24, Andrew Hughes wrote:
Ok for the build forest? Once reviewed then I think jdk8/tl would be better as that's the route that the changes to the zip code take.
-Alan.
----- Original Message -----
On 31/07/2012 00:24, Andrew Hughes wrote:
Ok for the build forest? Once reviewed then I think jdk8/tl would be better as that's the route that the changes to the zip code take.
Ok, no problem. I just suggest build as most of the changes are to the build machinery.
-Alan.
-- Andrew :) Free Java Software Engineer Red Hat, Inc. (http://www.redhat.com) PGP Key: 248BDC07 (https://keys.indymedia.org/) Fingerprint = EC5A 1F5E C0AD 1D15 8F1F 8F91 3B96 A578 248B DC07
----- Original Message -----
Hi all,
In OpenJDK 8, some support has already been added for using the system installation of zlib (see the thread http://mail.openjdk.java.net/pipermail/core-libs-dev/2012-July/010967.html), which is very similar to the support we've had in IcedTea for the last five years (wow, has it really been that long?).
This is great news for us, as it's less work we have to do in upstreaming the patch (though 7 still needs to be dealt with). As is, the following webrev:
http://cr.openjdk.java.net/~andrew/syslibs/zlib/webrev.01/
just fixes a few minor issues to match our existing setup, and fixes a bug found when testing the existing support. In detail, the webrev:
* Replaces the hardcoded use of "-lz" with $(ZLIB_LIBS) and $(ZLIB_CFLAGS), now set in make_jdk_generic_profile.sh. * Replaces "zlib.h" usage with <zlib.h> (mainly to reduce difference, searching '.' has no effect either way) * Stops uLong being defined if SYSTEM_ZLIB is set, even if we're not on Mac OS X. Without this fix, the build fails.
Ok for the build forest? If so, can I please have a bug ID for this?
Thanks, -- Andrew :)
Free Java Software Engineer Red Hat, Inc. (http://www.redhat.com)
PGP Key: 248BDC07 (https://keys.indymedia.org/) Fingerprint = EC5A 1F5E C0AD 1D15 8F1F 8F91 3B96 A578 248B DC07
Any update on this? Submission to tl, rather than build, now planned, as suggested by Alan. -- Andrew :) Free Java Software Engineer Red Hat, Inc. (http://www.redhat.com) PGP Key: 248BDC07 (https://keys.indymedia.org/) Fingerprint = EC5A 1F5E C0AD 1D15 8F1F 8F91 3B96 A578 248B DC07
Hi Andrew, I do have a ref# for this enhancement #7110151. Though my original idea is to also have a configurable mechanism to switch the zlib usage during the jvm startup (-D option for example). Let's use this one for your patch. I will take a look your patch this week. -Sherman On 08/02/2012 03:18 AM, Andrew Hughes wrote:
----- Original Message -----
Hi all,
In OpenJDK 8, some support has already been added for using the system installation of zlib (see the thread http://mail.openjdk.java.net/pipermail/core-libs-dev/2012-July/010967.html), which is very similar to the support we've had in IcedTea for the last five years (wow, has it really been that long?).
This is great news for us, as it's less work we have to do in upstreaming the patch (though 7 still needs to be dealt with). As is, the following webrev:
http://cr.openjdk.java.net/~andrew/syslibs/zlib/webrev.01/
just fixes a few minor issues to match our existing setup, and fixes a bug found when testing the existing support. In detail, the webrev:
* Replaces the hardcoded use of "-lz" with $(ZLIB_LIBS) and $(ZLIB_CFLAGS), now set in make_jdk_generic_profile.sh. * Replaces "zlib.h" usage with<zlib.h> (mainly to reduce difference, searching '.' has no effect either way) * Stops uLong being defined if SYSTEM_ZLIB is set, even if we're not on Mac OS X. Without this fix, the build fails.
Ok for the build forest? If so, can I please have a bug ID for this?
Thanks, -- Andrew :)
Free Java Software Engineer Red Hat, Inc. (http://www.redhat.com)
PGP Key: 248BDC07 (https://keys.indymedia.org/) Fingerprint = EC5A 1F5E C0AD 1D15 8F1F 8F91 3B96 A578 248B DC07
Any update on this? Submission to tl, rather than build, now planned, as suggested by Alan.
----- Original Message -----
Hi Andrew,
I do have a ref# for this enhancement #7110151. Though my original idea is to also have a configurable mechanism to switch the zlib usage during the jvm startup (-D option for example). Let's use this one for your patch. I will take a look your patch this week.
How would that work if we don't compile the in-tree version?
-Sherman
On 08/02/2012 03:18 AM, Andrew Hughes wrote:
----- Original Message -----
Hi all,
In OpenJDK 8, some support has already been added for using the system installation of zlib (see the thread http://mail.openjdk.java.net/pipermail/core-libs-dev/2012-July/010967.html), which is very similar to the support we've had in IcedTea for the last five years (wow, has it really been that long?).
This is great news for us, as it's less work we have to do in upstreaming the patch (though 7 still needs to be dealt with). As is, the following webrev:
http://cr.openjdk.java.net/~andrew/syslibs/zlib/webrev.01/
just fixes a few minor issues to match our existing setup, and fixes a bug found when testing the existing support. In detail, the webrev:
* Replaces the hardcoded use of "-lz" with $(ZLIB_LIBS) and $(ZLIB_CFLAGS), now set in make_jdk_generic_profile.sh. * Replaces "zlib.h" usage with<zlib.h> (mainly to reduce difference, searching '.' has no effect either way) * Stops uLong being defined if SYSTEM_ZLIB is set, even if we're not on Mac OS X. Without this fix, the build fails.
Ok for the build forest? If so, can I please have a bug ID for this?
Thanks, -- Andrew :)
Free Java Software Engineer Red Hat, Inc. (http://www.redhat.com)
PGP Key: 248BDC07 (https://keys.indymedia.org/) Fingerprint = EC5A 1F5E C0AD 1D15 8F1F 8F91 3B96 A578 248B DC07
Any update on this? Submission to tl, rather than build, now planned, as suggested by Alan.
-- Andrew :) Free Java Software Engineer Red Hat, Inc. (http://www.redhat.com) PGP Key: 248BDC07 (https://keys.indymedia.org/) Fingerprint = EC5A 1F5E C0AD 1D15 8F1F 8F91 3B96 A578 248B DC07
On 08/02/2012 11:13 AM, Andrew Hughes wrote:
----- Original Message -----
Hi Andrew,
I do have a ref# for this enhancement #7110151. Though my original idea is to also have a configurable mechanism to switch the zlib usage during the jvm startup (-D option for example). Let's use this one for your patch. I will take a look your patch this week.
How would that work if we don't compile the in-tree version?
The "original" thought is to still bundle the jdk version of zlib, but simply not use it if the underlying platform has its own version installed, or the user prefers to specify a "better" one (like the zlib in Intel's IPP, if you are licensed to use one when on intel's platform) -Sherman
-Sherman
On 08/02/2012 03:18 AM, Andrew Hughes wrote:
----- Original Message -----
Hi all,
In OpenJDK 8, some support has already been added for using the system installation of zlib (see the thread http://mail.openjdk.java.net/pipermail/core-libs-dev/2012-July/010967.html), which is very similar to the support we've had in IcedTea for the last five years (wow, has it really been that long?).
This is great news for us, as it's less work we have to do in upstreaming the patch (though 7 still needs to be dealt with). As is, the following webrev:
http://cr.openjdk.java.net/~andrew/syslibs/zlib/webrev.01/
just fixes a few minor issues to match our existing setup, and fixes a bug found when testing the existing support. In detail, the webrev:
* Replaces the hardcoded use of "-lz" with $(ZLIB_LIBS) and $(ZLIB_CFLAGS), now set in make_jdk_generic_profile.sh. * Replaces "zlib.h" usage with<zlib.h> (mainly to reduce difference, searching '.' has no effect either way) * Stops uLong being defined if SYSTEM_ZLIB is set, even if we're not on Mac OS X. Without this fix, the build fails.
Ok for the build forest? If so, can I please have a bug ID for this?
Thanks, -- Andrew :)
Free Java Software Engineer Red Hat, Inc. (http://www.redhat.com)
PGP Key: 248BDC07 (https://keys.indymedia.org/) Fingerprint = EC5A 1F5E C0AD 1D15 8F1F 8F91 3B96 A578 248B DC07
Any update on this? Submission to tl, rather than build, now planned, as suggested by Alan.
----- Original Message -----
On 08/02/2012 11:13 AM, Andrew Hughes wrote:
----- Original Message -----
Hi Andrew,
I do have a ref# for this enhancement #7110151. Though my original idea is to also have a configurable mechanism to switch the zlib usage during the jvm startup (-D option for example). Let's use this one for your patch. I will take a look your patch this week.
How would that work if we don't compile the in-tree version?
The "original" thought is to still bundle the jdk version of zlib, but simply not use it if the underlying platform has its own version installed, or the user prefers to specify a "better" one (like the zlib in Intel's IPP, if you are licensed to use one when on intel's platform)
Ok, I thought that's what you were aiming at. That wouldn't solve the problem for us. We can't include a version of zlib in OpenJDK. It violates distribution policies and means security updates have to be applied in two places. We've been carrying patches for zlib (and jpeg, png, etc.) for years to avoid this. Our build actually deletes the source files before building to ensure this doesn't happen.
-Sherman
-Sherman
On 08/02/2012 03:18 AM, Andrew Hughes wrote:
----- Original Message -----
Hi all,
In OpenJDK 8, some support has already been added for using the system installation of zlib (see the thread http://mail.openjdk.java.net/pipermail/core-libs-dev/2012-July/010967.html), which is very similar to the support we've had in IcedTea for the last five years (wow, has it really been that long?).
This is great news for us, as it's less work we have to do in upstreaming the patch (though 7 still needs to be dealt with). As is, the following webrev:
http://cr.openjdk.java.net/~andrew/syslibs/zlib/webrev.01/
just fixes a few minor issues to match our existing setup, and fixes a bug found when testing the existing support. In detail, the webrev:
* Replaces the hardcoded use of "-lz" with $(ZLIB_LIBS) and $(ZLIB_CFLAGS), now set in make_jdk_generic_profile.sh. * Replaces "zlib.h" usage with<zlib.h> (mainly to reduce difference, searching '.' has no effect either way) * Stops uLong being defined if SYSTEM_ZLIB is set, even if we're not on Mac OS X. Without this fix, the build fails.
Ok for the build forest? If so, can I please have a bug ID for this?
Thanks, -- Andrew :)
Free Java Software Engineer Red Hat, Inc. (http://www.redhat.com)
PGP Key: 248BDC07 (https://keys.indymedia.org/) Fingerprint = EC5A 1F5E C0AD 1D15 8F1F 8F91 3B96 A578 248B DC07
Any update on this? Submission to tl, rather than build, now planned, as suggested by Alan.
-- Andrew :) Free Java Software Engineer Red Hat, Inc. (http://www.redhat.com) PGP Key: 248BDC07 (https://keys.indymedia.org/) Fingerprint = EC5A 1F5E C0AD 1D15 8F1F 8F91 3B96 A578 248B DC07
On 02/08/2012 19:31, Andrew Hughes wrote:
:
Ok, I thought that's what you were aiming at. That wouldn't solve the problem for us. We can't include a version of zlib in OpenJDK. It violates distribution policies and means security updates have to be applied in two places. We've been carrying patches for zlib (and jpeg, png, etc.) for years to avoid this. Our build actually deletes the source files before building to ensure this doesn't happen.
As you know, we can't delete the zlib code as we still need it for other platforms, Windows mostly. From a brief scan of your patch (not a detailed review, too busy at the moment) then it looks like you've added a SYSTEM_ZLIB option to select (at build-time) whether to compile the version in the jdk repository or not. This approach make sense to me and should be separated from the changes needed to use an alternatively named zlib (which I think is what Intel IPP will require, assuming it can't be picked up by setting LD_LIBRARY_PATH). In any case, I think both efforts require 7188852 and I see Sherman has a review out on that. -Alan.
----- Original Message -----
On 02/08/2012 19:31, Andrew Hughes wrote:
:
Ok, I thought that's what you were aiming at. That wouldn't solve the problem for us. We can't include a version of zlib in OpenJDK. It violates distribution policies and means security updates have to be applied in two places. We've been carrying patches for zlib (and jpeg, png, etc.) for years to avoid this. Our build actually deletes the source files before building to ensure this doesn't happen.
As you know, we can't delete the zlib code as we still need it for other platforms, Windows mostly.
Yes, that wasn't the suggestion. I was just pointing out that we delete it as a pre-build step, so any attempt to build it is going to fail :-)
From a brief scan of your patch (not a detailed review, too busy at the moment) then it looks like you've added a SYSTEM_ZLIB option to select (at build-time) whether to compile the version in the jdk repository or not. This approach make sense to me and should be separated from the changes needed to use an alternatively named zlib (which I think is what Intel IPP will require, assuming it can't be picked up by setting LD_LIBRARY_PATH).
It's what we've had for years, and was added orthogonally to 8 in: changeset: 5118:d45bc4307996 user: michaelm date: Tue Mar 06 20:34:38 2012 +0000 summary: 7113349: Initial changeset for Macosx port to jdk though that version is broken (at least on GNU/Linux) without the change to defines.h I posted in my patch earlier this week. If IPP is binary compatible, surely setting ZLIB_LIBS/ZLIB_CFLAGS would be sufficient? I don't know how different they are.
In any case, I think both efforts require 7188852 and I see Sherman has a review out on that.
Yes, I've just tried applying it this morning. Will test and report back.
-Alan.
-- Andrew :) Free Java Software Engineer Red Hat, Inc. (http://www.redhat.com) PGP Key: 248BDC07 (https://keys.indymedia.org/) Fingerprint = EC5A 1F5E C0AD 1D15 8F1F 8F91 3B96 A578 248B DC07
On 03/08/2012 16:58, Andrew Hughes wrote:
: It's what we've had for years, and was added orthogonally to 8 in:
changeset: 5118:d45bc4307996 user: michaelm date: Tue Mar 06 20:34:38 2012 +0000 summary: 7113349: Initial changeset for Macosx port to jdk
though that version is broken (at least on GNU/Linux) without the change to defines.h I posted in my patch earlier this week. Right, that change was for Mac and it didn't have the goal to enable it on Linux.
BTW: Looking at your patch then I suspect it will cause problems on other platforms as it only sets ZLIB_LIBS in jdk_generic_profile.sh. If folks aren't using this script to setup their environment then I'm sure there will be a problem on Mac at least. I don't have time to spend on it but I suspect Defs-macosx.gmk will need to be updated to make it the default as it does now.
If IPP is binary compatible, surely setting ZLIB_LIBS/ZLIB_CFLAGS would be sufficient? I don't know how different they are.
I believe it is binary compatible but I think Sherman is thinking a runtime knob rather than a build-time option. -Alan.
----- Original Message -----
On 03/08/2012 16:58, Andrew Hughes wrote:
: It's what we've had for years, and was added orthogonally to 8 in:
changeset: 5118:d45bc4307996 user: michaelm date: Tue Mar 06 20:34:38 2012 +0000 summary: 7113349: Initial changeset for Macosx port to jdk
though that version is broken (at least on GNU/Linux) without the change to defines.h I posted in my patch earlier this week. Right, that change was for Mac and it didn't have the goal to enable it on Linux.
BTW: Looking at your patch then I suspect it will cause problems on other platforms as it only sets ZLIB_LIBS in jdk_generic_profile.sh. If folks aren't using this script to setup their environment then I'm sure there will be a problem on Mac at least. I don't have time to spend on it but I suspect Defs-macosx.gmk will need to be updated to make it the default as it does now.
I think that's what Sherman just hit. I thought the documented way to build was using jdk_generic_profile.sh? I can add a check to Defs-macosx.gmk.
If IPP is binary compatible, surely setting ZLIB_LIBS/ZLIB_CFLAGS would be sufficient? I don't know how different they are. I believe it is binary compatible but I think Sherman is thinking a runtime knob rather than a build-time option.
-Alan.
-- Andrew :) Free Java Software Engineer Red Hat, Inc. (http://www.redhat.com) PGP Key: 248BDC07 (https://keys.indymedia.org/) Fingerprint = EC5A 1F5E C0AD 1D15 8F1F 8F91 3B96 A578 248B DC07
----- Original Message -----
----- Original Message -----
On 03/08/2012 16:58, Andrew Hughes wrote:
: It's what we've had for years, and was added orthogonally to 8 in:
changeset: 5118:d45bc4307996 user: michaelm date: Tue Mar 06 20:34:38 2012 +0000 summary: 7113349: Initial changeset for Macosx port to jdk
though that version is broken (at least on GNU/Linux) without the change to defines.h I posted in my patch earlier this week. Right, that change was for Mac and it didn't have the goal to enable it on Linux.
BTW: Looking at your patch then I suspect it will cause problems on other platforms as it only sets ZLIB_LIBS in jdk_generic_profile.sh. If folks aren't using this script to setup their environment then I'm sure there will be a problem on Mac at least. I don't have time to spend on it but I suspect Defs-macosx.gmk will need to be updated to make it the default as it does now.
I think that's what Sherman just hit.
I thought the documented way to build was using jdk_generic_profile.sh? I can add a check to Defs-macosx.gmk.
http://cr.openjdk.java.net/~andrew/syslibs/zlib/webrev.02/ is an updated version which checks if ZLIB_LIBS is set on Solaris, GNU/Linux and MacOS X and sets it to -lz if not. I wasn't sure what to do with Windows but something can be added there if necessary. -- Andrew :) Free Java Software Engineer Red Hat, Inc. (http://www.redhat.com) PGP Key: 248BDC07 (https://keys.indymedia.org/) Fingerprint = EC5A 1F5E C0AD 1D15 8F1F 8F91 3B96 A578 248B DC07
Andrew, I just pushed 7188852. Is it possible for you to update your webrev with this change? I would like to try a full build (and regression tests) on your final bits to make sure we don't break the build. -Sherman On 08/03/2012 11:33 AM, Andrew Hughes wrote:
----- Original Message -----
----- Original Message -----
On 03/08/2012 16:58, Andrew Hughes wrote:
: It's what we've had for years, and was added orthogonally to 8 in:
changeset: 5118:d45bc4307996 user: michaelm date: Tue Mar 06 20:34:38 2012 +0000 summary: 7113349: Initial changeset for Macosx port to jdk
though that version is broken (at least on GNU/Linux) without the change to defines.h I posted in my patch earlier this week. Right, that change was for Mac and it didn't have the goal to enable it on Linux.
BTW: Looking at your patch then I suspect it will cause problems on other platforms as it only sets ZLIB_LIBS in jdk_generic_profile.sh. If folks aren't using this script to setup their environment then I'm sure there will be a problem on Mac at least. I don't have time to spend on it but I suspect Defs-macosx.gmk will need to be updated to make it the default as it does now.
I think that's what Sherman just hit.
I thought the documented way to build was using jdk_generic_profile.sh? I can add a check to Defs-macosx.gmk.
http://cr.openjdk.java.net/~andrew/syslibs/zlib/webrev.02/
is an updated version which checks if ZLIB_LIBS is set on Solaris, GNU/Linux and MacOS X and sets it to -lz if not.
I wasn't sure what to do with Windows but something can be added there if necessary.
----- Original Message -----
Andrew,
I just pushed 7188852. Is it possible for you to update your webrev with this change? I would like to try a full build (and regression tests) on your final bits to make sure we don't break the build.
Do you mean the ZLIB_LIBS changes? I already published an updated webrev;
-Sherman
On 08/03/2012 11:33 AM, Andrew Hughes wrote:
----- Original Message -----
----- Original Message -----
On 03/08/2012 16:58, Andrew Hughes wrote:
: It's what we've had for years, and was added orthogonally to 8 in:
changeset: 5118:d45bc4307996 user: michaelm date: Tue Mar 06 20:34:38 2012 +0000 summary: 7113349: Initial changeset for Macosx port to jdk
though that version is broken (at least on GNU/Linux) without the change to defines.h I posted in my patch earlier this week. Right, that change was for Mac and it didn't have the goal to enable it on Linux.
BTW: Looking at your patch then I suspect it will cause problems on other platforms as it only sets ZLIB_LIBS in jdk_generic_profile.sh. If folks aren't using this script to setup their environment then I'm sure there will be a problem on Mac at least. I don't have time to spend on it but I suspect Defs-macosx.gmk will need to be updated to make it the default as it does now.
I think that's what Sherman just hit.
I thought the documented way to build was using jdk_generic_profile.sh? I can add a check to Defs-macosx.gmk.
http://cr.openjdk.java.net/~andrew/syslibs/zlib/webrev.02/
is an updated version which checks if ZLIB_LIBS is set on Solaris, GNU/Linux and MacOS X and sets it to -lz if not.
I wasn't sure what to do with Windows but something can be added there if necessary.
-- Andrew :) Free Java Software Engineer Red Hat, Inc. (http://www.redhat.com) PGP Key: 248BDC07 (https://keys.indymedia.org/) Fingerprint = EC5A 1F5E C0AD 1D15 8F1F 8F91 3B96 A578 248B DC07
----- Original Message -----
Andrew,
I just pushed 7188852. Is it possible for you to update your webrev with this change? I would like to try a full build (and regression tests) on your final bits to make sure we don't break the build.
Do you mean the ZLIB_LIBS changes? I already published an updated webrev: http://cr.openjdk.java.net/~andrew/syslibs/zlib/webrev.02/ Is there something more needed? If not, let me know when you're happy with the patch and I'll push it.
-Sherman
On 08/03/2012 11:33 AM, Andrew Hughes wrote:
----- Original Message -----
----- Original Message -----
On 03/08/2012 16:58, Andrew Hughes wrote:
: It's what we've had for years, and was added orthogonally to 8 in:
changeset: 5118:d45bc4307996 user: michaelm date: Tue Mar 06 20:34:38 2012 +0000 summary: 7113349: Initial changeset for Macosx port to jdk
though that version is broken (at least on GNU/Linux) without the change to defines.h I posted in my patch earlier this week. Right, that change was for Mac and it didn't have the goal to enable it on Linux.
BTW: Looking at your patch then I suspect it will cause problems on other platforms as it only sets ZLIB_LIBS in jdk_generic_profile.sh. If folks aren't using this script to setup their environment then I'm sure there will be a problem on Mac at least. I don't have time to spend on it but I suspect Defs-macosx.gmk will need to be updated to make it the default as it does now.
I think that's what Sherman just hit.
I thought the documented way to build was using jdk_generic_profile.sh? I can add a check to Defs-macosx.gmk.
http://cr.openjdk.java.net/~andrew/syslibs/zlib/webrev.02/
is an updated version which checks if ZLIB_LIBS is set on Solaris, GNU/Linux and MacOS X and sets it to -lz if not.
I wasn't sure what to do with Windows but something can be added there if necessary.
-- Andrew :) Free Java Software Engineer Red Hat, Inc. (http://www.redhat.com) PGP Key: 248BDC07 (https://keys.indymedia.org/) Fingerprint = EC5A 1F5E C0AD 1D15 8F1F 8F91 3B96 A578 248B DC07
On 03/08/2012 19:33, Andrew Hughes wrote:
: http://cr.openjdk.java.net/~andrew/syslibs/zlib/webrev.02/
is an updated version which checks if ZLIB_LIBS is set on Solaris, GNU/Linux and MacOS X and sets it to -lz if not.
I wasn't sure what to do with Windows but something can be added there if necessary. Thanks for the update, it looks right to me now. To double check I did a quick build+test on all platforms with latest jdk8/tl + your patch and I don't see any issues.
Now I'm wondering whether we should just bite the bullet and default SYSTEM_ZLIB to true on Linux, maybe Solaris too (Sherman - can you think of any reasons not to do this? It would avoid needing to put in a means to switch zlib at startup as it could be done simply with LD_LIBRARY_PATH). -Alan.
On 8/5/2012 2:00 PM, Alan Bateman wrote:
On 03/08/2012 19:33, Andrew Hughes wrote:
: http://cr.openjdk.java.net/~andrew/syslibs/zlib/webrev.02/
is an updated version which checks if ZLIB_LIBS is set on Solaris, GNU/Linux and MacOS X and sets it to -lz if not.
I wasn't sure what to do with Windows but something can be added there if necessary. Thanks for the update, it looks right to me now. To double check I did a quick build+test on all platforms with latest jdk8/tl + your patch and I don't see any issues.
Now I'm wondering whether we should just bite the bullet and default SYSTEM_ZLIB to true on Linux, maybe Solaris too (Sherman - can you think of any reasons not to do this? It would avoid needing to put in a means to switch zlib at startup as it could be done simply with LD_LIBRARY_PATH).
-Alan.
I'm still on a very old ubuntu (9.1) so I might be wrong. Does the pkg-config --cflags/libs assume the zlib-dev or some similar dev package to be installed? pkg-config says I don't have it installed, so the cflags does not get set correctly. It appears at least one ubuntu12 machine has the same situation. So I guess at least we will have to add something into the "build readme" to add this package, if it is not installed by default. I don't have a Solaris machine for a while, so just wonder if the zlib always get installed by default installation these days? -Sherman
----- Original Message -----
On 8/5/2012 2:00 PM, Alan Bateman wrote:
On 03/08/2012 19:33, Andrew Hughes wrote:
: http://cr.openjdk.java.net/~andrew/syslibs/zlib/webrev.02/
is an updated version which checks if ZLIB_LIBS is set on Solaris, GNU/Linux and MacOS X and sets it to -lz if not.
I wasn't sure what to do with Windows but something can be added there if necessary. Thanks for the update, it looks right to me now. To double check I did a quick build+test on all platforms with latest jdk8/tl + your patch and I don't see any issues.
Now I'm wondering whether we should just bite the bullet and default SYSTEM_ZLIB to true on Linux, maybe Solaris too (Sherman - can you think of any reasons not to do this? It would avoid needing to put in a means to switch zlib at startup as it could be done simply with LD_LIBRARY_PATH).
-Alan.
I'm still on a very old ubuntu (9.1) so I might be wrong. Does the pkg-config --cflags/libs assume the zlib-dev or some similar dev package to be installed? pkg-config says I don't have it installed, so the cflags does not get set correctly.
You'll need zlib-dev both for pkg-config and the actual build, as you'll need the zlib headers. CFLAGS is usually empty anyway but pkgconfig will also provide the "-lz" for ZLIB_LIBS.
It appears at least one ubuntu12 machine has the same situation. So I guess at least we will have to add something into the "build readme" to add this package, if it is not installed by default.
Probably. This is pretty standard for building anything on a binary distribution, as binaries are split away from development headers, so it's not anything out of the ordinary. Headers for other libraries are already a requirement.
I don't have a Solaris machine for a while, so just wonder if the zlib always get installed by default installation these days?
I'll defer to those better informed on this one :-)
-Sherman
-- Andrew :) Free Java Software Engineer Red Hat, Inc. (http://www.redhat.com) PGP Key: 248BDC07 (https://keys.indymedia.org/) Fingerprint = EC5A 1F5E C0AD 1D15 8F1F 8F91 3B96 A578 248B DC07
Hi Andrew, I meant if we are going to put SYSTEM_ZLIB=true as default for linux as Alan suggested, we might need to update the build document as well to include zlib-dev as the "necessary" package to build jdk on linux. Currently the SYSTEM_ZLIB=true is set in make/common/Defs-macosx.gmk. ZLIB_VERSION = 1.2.5 is setin make/common/Defs.gmk. I'm not good at Makfile structure, just wonder why not put the ZLIB_LIBS setting into same place as well, it might help the future maintenance. I'm not sure in Defs.gmk or three copies in Defs-<os>.gmk, though. Personally, I would just put it in Defs.gmk, together with the ZLIB_VERSION. The rest looks fine to me. -Sherman On 08/06/2012 05:16 AM, Andrew Hughes wrote:
----- Original Message -----
On 8/5/2012 2:00 PM, Alan Bateman wrote:
On 03/08/2012 19:33, Andrew Hughes wrote:
: http://cr.openjdk.java.net/~andrew/syslibs/zlib/webrev.02/
is an updated version which checks if ZLIB_LIBS is set on Solaris, GNU/Linux and MacOS X and sets it to -lz if not.
I wasn't sure what to do with Windows but something can be added there if necessary. Thanks for the update, it looks right to me now. To double check I did a quick build+test on all platforms with latest jdk8/tl + your patch and I don't see any issues.
Now I'm wondering whether we should just bite the bullet and default SYSTEM_ZLIB to true on Linux, maybe Solaris too (Sherman - can you think of any reasons not to do this? It would avoid needing to put in a means to switch zlib at startup as it could be done simply with LD_LIBRARY_PATH).
-Alan. I'm still on a very old ubuntu (9.1) so I might be wrong. Does the pkg-config --cflags/libs assume the zlib-dev or some similar dev package to be installed? pkg-config says I don't have it installed, so the cflags does not get set correctly. You'll need zlib-dev both for pkg-config and the actual build, as you'll need the zlib headers. CFLAGS is usually empty anyway but pkgconfig will also provide the "-lz" for ZLIB_LIBS.
It appears at least one ubuntu12 machine has the same situation. So I guess at least we will have to add something into the "build readme" to add this package, if it is not installed by default.
Probably. This is pretty standard for building anything on a binary distribution, as binaries are split away from development headers, so it's not anything out of the ordinary. Headers for other libraries are already a requirement.
I don't have a Solaris machine for a while, so just wonder if the zlib always get installed by default installation these days?
I'll defer to those better informed on this one :-)
-Sherman
----- Original Message -----
Hi Andrew,
I meant if we are going to put SYSTEM_ZLIB=true as default for linux as Alan suggested, we might need to update the build document as well to include zlib-dev as the "necessary" package to build jdk on linux.
Ok. I think that's a separate issue which warrants a separate bug & changeset. Let's have this one work on fixing the build so SYSTEM_ZLIB actually works on GNU/Linux first. At the moment, enabling SYSTEM_ZLIB=true breaks the build on anything other than MacOS/X, which seems like a bug to me.
Currently the SYSTEM_ZLIB=true is set in make/common/Defs-macosx.gmk.
Ok, so it's the default there already. That explains the defines.h logic.
ZLIB_VERSION = 1.2.5 is setin make/common/Defs.gmk.
What's the relevance of this version? My system install is 1.2.7.
I'm not good at Makfile structure, just wonder why not put the ZLIB_LIBS setting into same place as well, it might help the future maintenance. I'm not sure in Defs.gmk or three copies in Defs-<os>.gmk, though. Personally, I would just put it in Defs.gmk, together with the ZLIB_VERSION.
There's more than just three Defs-<os>.gmk files. There's also Windows and embedded, neither of which I can build and I doubt "-lz" works on Windows for linking zlib. I've used MacOS X/*BSD, Solaris and GNU/Linux enough to know it will work there.
The rest looks fine to me.
Thanks :-)
-Sherman
On 08/06/2012 05:16 AM, Andrew Hughes wrote:
----- Original Message -----
On 8/5/2012 2:00 PM, Alan Bateman wrote:
On 03/08/2012 19:33, Andrew Hughes wrote:
: http://cr.openjdk.java.net/~andrew/syslibs/zlib/webrev.02/
is an updated version which checks if ZLIB_LIBS is set on Solaris, GNU/Linux and MacOS X and sets it to -lz if not.
I wasn't sure what to do with Windows but something can be added there if necessary. Thanks for the update, it looks right to me now. To double check I did a quick build+test on all platforms with latest jdk8/tl + your patch and I don't see any issues.
Now I'm wondering whether we should just bite the bullet and default SYSTEM_ZLIB to true on Linux, maybe Solaris too (Sherman - can you think of any reasons not to do this? It would avoid needing to put in a means to switch zlib at startup as it could be done simply with LD_LIBRARY_PATH).
-Alan. I'm still on a very old ubuntu (9.1) so I might be wrong. Does the pkg-config --cflags/libs assume the zlib-dev or some similar dev package to be installed? pkg-config says I don't have it installed, so the cflags does not get set correctly. You'll need zlib-dev both for pkg-config and the actual build, as you'll need the zlib headers. CFLAGS is usually empty anyway but pkgconfig will also provide the "-lz" for ZLIB_LIBS.
It appears at least one ubuntu12 machine has the same situation. So I guess at least we will have to add something into the "build readme" to add this package, if it is not installed by default.
Probably. This is pretty standard for building anything on a binary distribution, as binaries are split away from development headers, so it's not anything out of the ordinary. Headers for other libraries are already a requirement.
I don't have a Solaris machine for a while, so just wonder if the zlib always get installed by default installation these days?
I'll defer to those better informed on this one :-)
-Sherman
-- Andrew :) Free Java Software Engineer Red Hat, Inc. (http://www.redhat.com) PGP Key: 248BDC07 (https://keys.indymedia.org/) Fingerprint = EC5A 1F5E C0AD 1D15 8F1F 8F91 3B96 A578 248B DC07
On 08/06/2012 11:26 AM, Andrew Hughes wrote:
----- Original Message -----
Hi Andrew,
I meant if we are going to put SYSTEM_ZLIB=true as default for linux as Alan suggested, we might need to update the build document as well to include zlib-dev as the "necessary" package to build jdk on linux. Ok. I think that's a separate issue which warrants a separate bug& changeset. Let's have this one work on fixing the build so SYSTEM_ZLIB actually works on GNU/Linux first. At the moment, enabling SYSTEM_ZLIB=true breaks the build on anything other than MacOS/X, which seems like a bug to me.
It's a separate issue. For your current patch, since it does not set the SYSTEM_ZLIB=true for linux, it's not an issue at all. My apology for my naive question, is zlib (the library) always installed by default by most linux distributors?
Currently the SYSTEM_ZLIB=true is set in make/common/Defs-macosx.gmk. Ok, so it's the default there already. That explains the defines.h logic.
ZLIB_VERSION = 1.2.5 is setin make/common/Defs.gmk. What's the relevance of this version? My system install is 1.2.7.
That's the version bundled with jdk. Yes, 1.2.7 is the latest, out May/2012. I might try to upgrade the bundled one to the latest version at later stage of jdk8. The "relevance" is that they are all used for "zlib build", so it would be better to put them together/close for future maintenance, though some comment might be necessary to clarify one is for "bundled" version, on is for the link options for os zlib. -sherman
----- Original Message -----
On 08/06/2012 11:26 AM, Andrew Hughes wrote:
----- Original Message -----
Hi Andrew,
I meant if we are going to put SYSTEM_ZLIB=true as default for linux as Alan suggested, we might need to update the build document as well to include zlib-dev as the "necessary" package to build jdk on linux. Ok. I think that's a separate issue which warrants a separate bug& changeset. Let's have this one work on fixing the build so SYSTEM_ZLIB actually works on GNU/Linux first. At the moment, enabling SYSTEM_ZLIB=true breaks the build on anything other than MacOS/X, which seems like a bug to me.
It's a separate issue. For your current patch, since it does not set the SYSTEM_ZLIB=true for linux, it's not an issue at all.
As we override the default anyway, I'd prefer not to be the one pushing for it if it causes problems down the road :-) My apology for my naive
question, is zlib (the library) always installed by default by most linux distributors?
No apology required. It's not that naive a question as there are plenty of distributions out there, so giving a completely accurate answer is impossible. There's more than likely someone who doesn't :-) I would say it's extremely unlikely that the library's not installed. Looking at the dependency tree, it's a requirement of perl, nss, libpng, freetype, openssh, firefox, kdelibs & cairo to name a few. You're not going to get much of a GUI environment going without it, that's for sure. As you pointed out earlier in the thread, binary distributions tend to separate the library and the development headers / pkgconfig files. It's much less likely that zlib-devel (or whatever it gets called) is installed on a standard system, but for someone doing builds, it becomes more likely especially as it's a dependency for other development packages such as the one for libpng.
Currently the SYSTEM_ZLIB=true is set in make/common/Defs-macosx.gmk. Ok, so it's the default there already. That explains the defines.h logic.
ZLIB_VERSION = 1.2.5 is setin make/common/Defs.gmk. What's the relevance of this version? My system install is 1.2.7.
That's the version bundled with jdk. Yes, 1.2.7 is the latest, out May/2012. I might try to upgrade the bundled one to the latest version at later stage of jdk8. The "relevance" is that they are all used for "zlib build", so it would be better to put them together/close for future maintenance, though some comment might be necessary to clarify one is for "bundled" version, on is for the link options for os zlib.
Ah, ok. So it's irrelevant if SYSTEM_ZLIB=true. I was worried the idea might be for the build to do a version check like it does with Freetype. That's been a pain in the past. So is the patch ok for me to push? And do we have a bug ID if so? Sorry if you've already mentioned one, but I've lost track in this thread :-)
-sherman
-- Andrew :) Free Java Software Engineer Red Hat, Inc. (http://www.redhat.com) PGP Key: 248BDC07 (https://keys.indymedia.org/) Fingerprint = EC5A 1F5E C0AD 1D15 8F1F 8F91 3B96 A578 248B DC07
Hi, Please make sure that the jdk/test/tools/* tests are run and validated. Both the java launcher and the pack200 tools are dependent on zlib entry points for inflate/deflate and other things. Kumar
----- Original Message -----
On 08/06/2012 11:26 AM, Andrew Hughes wrote:
----- Original Message -----
Hi Andrew,
I meant if we are going to put SYSTEM_ZLIB=true as default for linux as Alan suggested, we might need to update the build document as well to include zlib-dev as the "necessary" package to build jdk on linux. Ok. I think that's a separate issue which warrants a separate bug& changeset. Let's have this one work on fixing the build so SYSTEM_ZLIB actually works on GNU/Linux first. At the moment, enabling SYSTEM_ZLIB=true breaks the build on anything other than MacOS/X, which seems like a bug to me. It's a separate issue. For your current patch, since it does not set the SYSTEM_ZLIB=true for linux, it's not an issue at all. As we override the default anyway, I'd prefer not to be the one pushing for it if it causes problems down the road :-)
My apology for my naive
question, is zlib (the library) always installed by default by most linux distributors? No apology required. It's not that naive a question as there are plenty of distributions out there, so giving a completely accurate answer is impossible. There's more than likely someone who doesn't :-)
I would say it's extremely unlikely that the library's not installed. Looking at the dependency tree, it's a requirement of perl, nss, libpng, freetype, openssh, firefox, kdelibs& cairo to name a few. You're not going to get much of a GUI environment going without it, that's for sure.
As you pointed out earlier in the thread, binary distributions tend to separate the library and the development headers / pkgconfig files. It's much less likely that zlib-devel (or whatever it gets called) is installed on a standard system, but for someone doing builds, it becomes more likely especially as it's a dependency for other development packages such as the one for libpng.
Currently the SYSTEM_ZLIB=true is set in make/common/Defs-macosx.gmk. Ok, so it's the default there already. That explains the defines.h logic.
ZLIB_VERSION = 1.2.5 is setin make/common/Defs.gmk. What's the relevance of this version? My system install is 1.2.7. That's the version bundled with jdk. Yes, 1.2.7 is the latest, out May/2012. I might try to upgrade the bundled one to the latest version at later stage of jdk8. The "relevance" is that they are all used for "zlib build", so it would be better to put them together/close for future maintenance, though some comment might be necessary to clarify one is for "bundled" version, on is for the link options for os zlib.
Ah, ok. So it's irrelevant if SYSTEM_ZLIB=true. I was worried the idea might be for the build to do a version check like it does with Freetype. That's been a pain in the past.
So is the patch ok for me to push? And do we have a bug ID if so? Sorry if you've already mentioned one, but I've lost track in this thread :-)
-sherman
----- Original Message -----
On 08/06/2012 11:26 AM, Andrew Hughes wrote:
----- Original Message -----
Hi Andrew,
I meant if we are going to put SYSTEM_ZLIB=true as default for linux as Alan suggested, we might need to update the build document as well to include zlib-dev as the "necessary" package to build jdk on linux. Ok. I think that's a separate issue which warrants a separate bug& changeset. Let's have this one work on fixing the build so SYSTEM_ZLIB actually works on GNU/Linux first. At the moment, enabling SYSTEM_ZLIB=true breaks the build on anything other than MacOS/X, which seems like a bug to me.
It's a separate issue. For your current patch, since it does not set the SYSTEM_ZLIB=true for linux, it's not an issue at all. My apology for my naive question, is zlib (the library) always installed by default by most linux distributors?
Currently the SYSTEM_ZLIB=true is set in make/common/Defs-macosx.gmk. Ok, so it's the default there already. That explains the defines.h logic.
ZLIB_VERSION = 1.2.5 is setin make/common/Defs.gmk. What's the relevance of this version? My system install is 1.2.7.
That's the version bundled with jdk. Yes, 1.2.7 is the latest, out May/2012. I might try to upgrade the bundled one to the latest version at later stage of jdk8. The "relevance" is that they are all used for "zlib build", so it would be better to put them together/close for future maintenance, though some comment might be necessary to clarify one is for "bundled" version, on is for the link options for os zlib.
-sherman
Just checking on the status of this. Am I ok to push: http://cr.openjdk.java.net/~andrew/syslibs/zlib/webrev.02/ as #7110151 to tl? Once that's in and system zlib support is working, we can decide whether to turn it on by default. Thanks, -- Andrew :) Free Java Software Engineer Red Hat, Inc. (http://www.redhat.com) PGP Key: 248BDC07 (https://keys.indymedia.org/) Fingerprint = EC5A 1F5E C0AD 1D15 8F1F 8F91 3B96 A578 248B DC07
Hi Andrew Alan mentioned the other day that the build (with your latest patch, with zlib-dev) failed at pack200 when he tried to turn it on on linux. Can your double check on your system? -Sherman On 08/09/2012 06:31 AM, Andrew Hughes wrote:
Just checking on the status of this.
Am I ok to push:
http://cr.openjdk.java.net/~andrew/syslibs/zlib/webrev.02/
as #7110151 to tl?
Once that's in and system zlib support is working, we can decide whether to turn it on by default.
Thanks,
----- Original Message -----
Hi Andrew
Alan mentioned the other day that the build (with your latest patch, with zlib-dev) failed at pack200 when he tried to turn it on on linux. Can your double check on your system?
What kind of failure? All webrevs I've posted pass a complete build here, and we've been using system zlib for years as previously mentioned, so I'm surprised by this.
-Sherman
On 08/09/2012 06:31 AM, Andrew Hughes wrote:
Just checking on the status of this.
Am I ok to push:
http://cr.openjdk.java.net/~andrew/syslibs/zlib/webrev.02/
as #7110151 to tl?
Once that's in and system zlib support is working, we can decide whether to turn it on by default.
Thanks,
-- Andrew :) Free Java Software Engineer Red Hat, Inc. (http://www.redhat.com) PGP Key: 248BDC07 (https://keys.indymedia.org/) Fingerprint = EC5A 1F5E C0AD 1D15 8F1F 8F91 3B96 A578 248B DC07
On 10/08/2012 18:40, Andrew Hughes wrote:
----- Original Message -----
Hi Andrew
Alan mentioned the other day that the build (with your latest patch, with zlib-dev) failed at pack200 when he tried to turn it on on linux. Can your double check on your system?
What kind of failure? All webrevs I've posted pass a complete build here, and we've been using system zlib for years as previously mentioned, so I'm surprised by this.
I grabbed your patch a few days ago and did a build+test job on all platforms (Linux/Solaris/Mac/Windows) and didn't see any issues. This was of course using the defaults, I didn't set SYSTEM_ZLIB explicitly. However I did see an issue on Ubuntu 12.0.4 (32-bit) with SYSTEM_ZLIB=true. Attached is the tail of the log. I haven't had time to look at it but I do see that libzip.so is built as expected (ldd shows it is linked to libz). The build failure is with unpack200 as it uses the the zlib functions. Building with SYSTEM_ZLIB=false is fine on this system. -Alan rm -f ../../../../../build/linux-i586/bin/unpack200 rm -f ../../../../../build/linux-i586/tmp/sun/com.sun.java.util.jar.pack/unpack/mapfile-vers /bin/cp mapfile-vers-unpack200 ../../../../../build/linux-i586/tmp/sun/com.sun.java.util.jar.pack/unpack/mapfile-vers /usr/bin/gcc -O2 -DPRODUCT -fPIC -DCC_NOEX -W -Wall -Wno-unused -Wno-parentheses -fno-omit-frame-pointer -D_LITTLE_ENDIAN -DFULL -DSYSTEM_ZLIB -DNDEBUG -DARCH='"i586"' -Di586 -DLINUX -DRELEASE='"1.8.0-internal"' -D_LARGEFILE64_SOURCE -D_GNU_SOURCE -D_REENTRANT -I. -I../../../../../build/linux-i586/tmp/sun/com.sun.java.util.jar.pack/unpack/CClassHeaders -I../../../../../src/closed/share/javavm/export -I../../../../../src/solaris/javavm/export -I../../../../../src/share/javavm/export -I../../../../../src/share/native/common -I../../../../../src/solaris/native/common -I../../../../../src/share/native/com/sun/java/util/jar/pack -I../../../../../src/solaris/native/com/sun/java/util/jar/pack -Xlinker -O1 -Xlinker -version-script=mapfile-vers -Wl,--hash-style=both -Xlinker -z -Xlinker origin -Xlinker -rpath -Xlinker \$ORIGIN -Xlinker -z -Xlinker defs -L../../../../../build/linux-i586/lib/i386 -Wl,-soname=libunpack.so -lz -lc ../../../../../build/linux-i586/tmp/sun/com.sun.java.util.jar.pack/unpack-cmd/obj/bands.o ../../../../../build/linux-i586/tmp/sun/com.sun.java.util.jar.pack/unpack-cmd/obj/bytes.o ../../../../../build/linux-i586/tmp/sun/com.sun.java.util.jar.pack/unpack-cmd/obj/coding.o ../../../../../build/linux-i586/tmp/sun/com.sun.java.util.jar.pack/unpack-cmd/obj/unpack.o ../../../../../build/linux-i586/tmp/sun/com.sun.java.util.jar.pack/unpack-cmd/obj/utils.o ../../../../../build/linux-i586/tmp/sun/com.sun.java.util.jar.pack/unpack-cmd/obj/zip.o ../../../../../build/linux-i586/tmp/sun/com.sun.java.util.jar.pack/unpack-cmd/obj/main.o -Wl,-Bstatic -lstdc++ -lgcc -Wl,-Bdynamic -o ../../../../../build/linux-i586/tmp/sun/com.sun.java.util.jar.pack/unpack/unpack200 ../../../../../build/linux-i586/tmp/sun/com.sun.java.util.jar.pack/unpack-cmd/obj/zip.o: In function `jar::deflate_bytes(bytes&, bytes&)': zip.cpp:(.text+0x79b): undefined reference to `deflateInit2_' zip.cpp:(.text+0x828): undefined reference to `deflate' zip.cpp:(.text+0x842): undefined reference to `deflateEnd' zip.cpp:(.text+0x854): undefined reference to `deflateEnd' zip.cpp:(.text+0x87c): undefined reference to `deflate' ../../../../../build/linux-i586/tmp/sun/com.sun.java.util.jar.pack/unpack-cmd/obj/zip.o: In function `jar::addJarEntry(char const*, bool, int, bytes&, bytes&)': zip.cpp:(.text+0x8d2): undefined reference to `crc32' zip.cpp:(.text+0x9d1): undefined reference to `crc32' zip.cpp:(.text+0x9ee): undefined reference to `crc32' ../../../../../build/linux-i586/tmp/sun/com.sun.java.util.jar.pack/unpack-cmd/obj/zip.o: In function `gunzip::free()': zip.cpp:(.text+0xb33): undefined reference to `inflateEnd' ../../../../../build/linux-i586/tmp/sun/com.sun.java.util.jar.pack/unpack-cmd/obj/zip.o: In function `read_input_via_gzip(unpacker*, void*, long long, long long)': zip.cpp:(.text+0xbfe): undefined reference to `inflate' ../../../../../build/linux-i586/tmp/sun/com.sun.java.util.jar.pack/unpack-cmd/obj/zip.o: In function `gunzip::start(int)': zip.cpp:(.text+0xf35): undefined reference to `inflateInit2_' collect2: ld returned 1 exit status
----- Original Message -----
On 10/08/2012 18:40, Andrew Hughes wrote:
----- Original Message -----
Hi Andrew
Alan mentioned the other day that the build (with your latest patch, with zlib-dev) failed at pack200 when he tried to turn it on on linux. Can your double check on your system?
What kind of failure? All webrevs I've posted pass a complete build here, and we've been using system zlib for years as previously mentioned, so I'm surprised by this.
I grabbed your patch a few days ago and did a build+test job on all platforms (Linux/Solaris/Mac/Windows) and didn't see any issues. This was of course using the defaults, I didn't set SYSTEM_ZLIB explicitly.
However I did see an issue on Ubuntu 12.0.4 (32-bit) with SYSTEM_ZLIB=true. Attached is the tail of the log. I haven't had time to look at it but I do see that libzip.so is built as expected (ldd shows it is linked to libz). The build failure is with unpack200 as it uses the the zlib functions. Building with SYSTEM_ZLIB=false is fine on this system.
-Alan
rm -f ../../../../../build/linux-i586/bin/unpack200 rm -f ../../../../../build/linux-i586/tmp/sun/com.sun.java.util.jar.pack/unpack/mapfile-vers /bin/cp mapfile-vers-unpack200 ../../../../../build/linux-i586/tmp/sun/com.sun.java.util.jar.pack/unpack/mapfile-vers /usr/bin/gcc -O2 -DPRODUCT -fPIC -DCC_NOEX -W -Wall -Wno-unused -Wno-parentheses -fno-omit-frame-pointer -D_LITTLE_ENDIAN -DFULL -DSYSTEM_ZLIB -DNDEBUG -DARCH='"i586"' -Di586 -DLINUX -DRELEASE='"1.8.0-internal"' -D_LARGEFILE64_SOURCE -D_GNU_SOURCE -D_REENTRANT -I. -I../../../../../build/linux-i586/tmp/sun/com.sun.java.util.jar.pack/unpack/CClassHeaders -I../../../../../src/closed/share/javavm/export -I../../../../../src/solaris/javavm/export -I../../../../../src/share/javavm/export -I../../../../../src/share/native/common -I../../../../../src/solaris/native/common -I../../../../../src/share/native/com/sun/java/util/jar/pack -I../../../../../src/solaris/native/com/sun/java/util/jar/pack -Xlinker -O1 -Xlinker -version-script=mapfile-vers -Wl,--hash-style=both -Xlinker -z -Xlinker origin -Xlinker -rpath -Xlinker \$ORIGIN -Xlinker -z -Xlinker defs -L../../../../../build/linux-i586/lib/i386 -Wl,-soname=libunpack.so -lz -lc ../../../../../build/linux-i586/tmp/sun/com.sun.java.util.jar.pack/unpack-cmd/obj/bands.o ../../../../../build/linux-i586/tmp/sun/com.sun.java.util.jar.pack/unpack-cmd/obj/bytes.o ../../../../../build/linux-i586/tmp/sun/com.sun.java.util.jar.pack/unpack-cmd/obj/coding.o ../../../../../build/linux-i586/tmp/sun/com.sun.java.util.jar.pack/unpack-cmd/obj/unpack.o ../../../../../build/linux-i586/tmp/sun/com.sun.java.util.jar.pack/unpack-cmd/obj/utils.o ../../../../../build/linux-i586/tmp/sun/com.sun.java.util.jar.pack/unpack-cmd/obj/zip.o ../../../../../build/linux-i586/tmp/sun/com.sun.java.util.jar.pack/unpack-cmd/obj/main.o -Wl,-Bstatic -lstdc++ -lgcc -Wl,-Bdynamic -o ../../../../../build/linux-i586/tmp/sun/com.sun.java.util.jar.pack/unpack/unpack200 ../../../../../build/linux-i586/tmp/sun/com.sun.java.util.jar.pack/unpack-cmd/obj/zip.o: In function `jar::deflate_bytes(bytes&, bytes&)': zip.cpp:(.text+0x79b): undefined reference to `deflateInit2_' zip.cpp:(.text+0x828): undefined reference to `deflate' zip.cpp:(.text+0x842): undefined reference to `deflateEnd' zip.cpp:(.text+0x854): undefined reference to `deflateEnd' zip.cpp:(.text+0x87c): undefined reference to `deflate' ../../../../../build/linux-i586/tmp/sun/com.sun.java.util.jar.pack/unpack-cmd/obj/zip.o: In function `jar::addJarEntry(char const*, bool, int, bytes&, bytes&)': zip.cpp:(.text+0x8d2): undefined reference to `crc32' zip.cpp:(.text+0x9d1): undefined reference to `crc32' zip.cpp:(.text+0x9ee): undefined reference to `crc32' ../../../../../build/linux-i586/tmp/sun/com.sun.java.util.jar.pack/unpack-cmd/obj/zip.o: In function `gunzip::free()': zip.cpp:(.text+0xb33): undefined reference to `inflateEnd' ../../../../../build/linux-i586/tmp/sun/com.sun.java.util.jar.pack/unpack-cmd/obj/zip.o: In function `read_input_via_gzip(unpacker*, void*, long long, long long)': zip.cpp:(.text+0xbfe): undefined reference to `inflate' ../../../../../build/linux-i586/tmp/sun/com.sun.java.util.jar.pack/unpack-cmd/obj/zip.o: In function `gunzip::start(int)': zip.cpp:(.text+0xf35): undefined reference to `inflateInit2_' collect2: ld returned 1 exit status
Ok, the obvious difference between my output and yours is you seem to be linking statically via gcc rather than dynamically with g++. /usr/bin/g++ -O2 -DPRODUCT -fPIC -DCC_NOEX -W -Wall \ -Wno-unused -Wno-parentheses -fno-omit-frame-pointer -D_LITTLE_ENDIAN -g \ -DFULL -DSYSTEM_ZLIB -DNDEBUG -DARCH='"amd64"' -Damd64 -DLINUX -DRELEASE='"1.8.0-internal"' \ -D_LARGEFILE64_SOURCE -D_GNU_SOURCE -D_REENTRANT -D_LP64=1 -I. \ -I/home/andrew/builder/awt/tmp/sun/com.sun.java.util.jar.pack/unpack/CClassHeaders \ -I../../../../../src/solaris/javavm/export -I../../../../../src/share/javavm/export \ -I../../../../../src/share/native/common -I../../../../../src/solaris/native/common \ -I../../../../../src/share/native/com/sun/java/util/jar/pack \ -I../../../../../src/solaris/native/com/sun/java/util/jar/pack \ -Xlinker -O1 -Xlinker -version-script=mapfile-vers -Xlinker -z -Xlinker origin \ -Xlinker -rpath -Xlinker \$ORIGIN -Xlinker -z -Xlinker defs -L/home/andrew/builder/awt/lib/amd64\ -Wl,-soname=libunpack.so -lz -lc \ /home/andrew/builder/awt/tmp/sun/com.sun.java.util.jar.pack/unpack-cmd/obj64/bands.o \ /home/andrew/builder/awt/tmp/sun/com.sun.java.util.jar.pack/unpack-cmd/obj64/bytes.o \ /home/andrew/builder/awt/tmp/sun/com.sun.java.util.jar.pack/unpack-cmd/obj64/coding.o \ /home/andrew/builder/awt/tmp/sun/com.sun.java.util.jar.pack/unpack-cmd/obj64/unpack.o \ /home/andrew/builder/awt/tmp/sun/com.sun.java.util.jar.pack/unpack-cmd/obj64/utils.o \ /home/andrew/builder/awt/tmp/sun/com.sun.java.util.jar.pack/unpack-cmd/obj64/zip.o \ /home/andrew/builder/awt/tmp/sun/com.sun.java.util.jar.pack/unpack-cmd/obj64/main.o \ -lstdc++ -o /home/andrew/builder/awt/tmp/sun/com.sun.java.util.jar.pack/unpack/unpack200 /bin/cp /home/andrew/builder/awt/tmp/sun/com.sun.java.util.jar.pack/unpack/unpack200 \ /home/andrew/builder/awt/bin/unpack200 Checking for mapfile use in: /home/andrew/builder/awt/bin/unpack200 Library loads for: /home/andrew/builder/awt/bin/unpack200 linux-vdso.so.1 (0x00007ffff315c000) libz.so.1 => /lib64/libz.so.1 (0x00007fd609351000) libstdc++.so.6 => /usr/lib/gcc/x86_64-pc-linux-gnu/4.7.1/libstdc++.so.6 (0x00007fd60904a000) libm.so.6 => /lib64/libm.so.6 (0x00007fd608d56000) libc.so.6 => /lib64/libc.so.6 (0x00007fd6089ac000) libgcc_s.so.1 => /usr/lib/gcc/x86_64-pc-linux-gnu/4.7.1/libgcc_s.so.1 (0x00007fd608796000) /lib64/ld-linux-x86-64.so.2 (0x00007fd609567000) RUNPATH for: /home/andrew/builder/awt/bin/unpack200 0x0000000000000001 (NEEDED) Shared library: [libz.so.1] 0x0000000000000001 (NEEDED) Shared library: [libstdc++.so.6] 0x0000000000000001 (NEEDED) Shared library: [libm.so.6] 0x0000000000000001 (NEEDED) Shared library: [libc.so.6] 0x0000000000000001 (NEEDED) Shared library: [libgcc_s.so.1] 0x000000000000000f (RPATH) Library rpath: [$ORIGIN] 0x000000000000001d (RUNPATH) Library runpath: [$ORIGIN] Do you have a static libz.a installed (from zlib1g-dev) and all dependent static libraries? I suspect you'll get the same failure with or without my patch (though you'll probably need the change to defines.h to get this far...) I'd guess you have STATIC_CXX set to true (the default apparently):
From Compiler-gcc.gmk:
STATIC_CXX = true ifeq ($(STATIC_CXX),true) # g++ always dynamically links libstdc++, even we use "-Wl,-Bstatic -lstdc++" # We need to use gcc to statically link the C++ runtime. gcc and g++ use # the same subprocess to compile C++ files, so it is OK to build using gcc. CXX = $(COMPILER_PATH)gcc else CXX = $(COMPILER_PATH)g++ endif
From Defs-linux.gmk:
ifeq ($(STATIC_CXX),true) override LIBCXX = -Wl,-Bstatic -lstdc++ -lgcc -Wl,-Bdynamic else override LIBCXX = -lstdc++ endif -- Andrew :) Free Java Software Engineer Red Hat, Inc. (http://www.redhat.com) PGP Key: 248BDC07 (https://keys.indymedia.org/) Fingerprint = EC5A 1F5E C0AD 1D15 8F1F 8F91 3B96 A578 248B DC07
On 14/08/2012 13:36, Andrew Hughes wrote:
:
Do you have a static libz.a installed (from zlib1g-dev) and all dependent static libraries? I think you're right about the static linking but this was just a quick test to see if SYSTEM_ZLIB=true worked with everything else as default.
pack200 has C++ so that explains why we see it there and not when building libzip. $ dpkg -s zlib1g-dev Package: zlib1g-dev Status: install ok installed Multi-Arch: same Priority: optional Section: libdevel Installed-Size: 366 Maintainer: Ubuntu Developers <ubuntu-devel-discuss@lists.ubuntu.com> Architecture: i386 Source: zlib Version: 1:1.2.3.4.dfsg-3ubuntu4 Provides: libz-dev Depends: zlib1g (= 1:1.2.3.4.dfsg-3ubuntu4), libc6-dev | libc-dev Conflicts: zlib1-dev Description: compression library - development zlib is a library implementing the deflate compression method found in gzip and PKZIP. This package includes the development support files. Homepage: http://zlib.net/ Original-Maintainer: Mark Brown <broonie@debian.org> $ apt-file search libz.a lib64z1-dev: /usr/lib64/libz.a zlib1g-dev: /usr/lib/i386-linux-gnu/libz.a $ ls -l /usr/lib/i386-linux-gnu/libz.a -rw-r--r-- 1 root root 98772 Nov 10 2011 /usr/lib/i386-linux-gnu/libz.a
I suspect you'll get the same failure with or without my patch (though you'll probably need the change to defines.h to get this far...)
SYSTEM_ZLIB is currently Mac only so I wouldn't expect to have got very far without your changes.
I'd guess you have STATIC_CXX set to true (the default apparently):
I didn't specify any other build options so it's using the default. BTW: I should mention that I don't have any issues with the patch proposed as it works as it does now because SYSTEM_ZLIB is false. It may be that there is follow-up to allow SYSTEM_ZLIB=true and static linking to work together. -Alan.
----- Original Message -----
On 14/08/2012 13:36, Andrew Hughes wrote:
:
Do you have a static libz.a installed (from zlib1g-dev) and all dependent static libraries? I think you're right about the static linking but this was just a quick test to see if SYSTEM_ZLIB=true worked with everything else as default.
Ah right.
pack200 has C++ so that explains why we see it there and not when building libzip.
Right. BTW, it appears gcc 4.5 and later have a new option -static-libstdc++. I don't know if that would work better than the options currently used. http://gcc.gnu.org/gcc-4.5/changes.html
$ dpkg -s zlib1g-dev Package: zlib1g-dev Status: install ok installed Multi-Arch: same Priority: optional Section: libdevel Installed-Size: 366 Maintainer: Ubuntu Developers <ubuntu-devel-discuss@lists.ubuntu.com> Architecture: i386 Source: zlib Version: 1:1.2.3.4.dfsg-3ubuntu4 Provides: libz-dev Depends: zlib1g (= 1:1.2.3.4.dfsg-3ubuntu4), libc6-dev | libc-dev Conflicts: zlib1-dev Description: compression library - development zlib is a library implementing the deflate compression method found in gzip and PKZIP. This package includes the development support files. Homepage: http://zlib.net/ Original-Maintainer: Mark Brown <broonie@debian.org>
$ apt-file search libz.a lib64z1-dev: /usr/lib64/libz.a zlib1g-dev: /usr/lib/i386-linux-gnu/libz.a
$ ls -l /usr/lib/i386-linux-gnu/libz.a -rw-r--r-- 1 root root 98772 Nov 10 2011 /usr/lib/i386-linux-gnu/libz.a
I suspect you'll get the same failure with or without my patch (though you'll probably need the change to defines.h to get this far...)
SYSTEM_ZLIB is currently Mac only so I wouldn't expect to have got very far without your changes.
Yes, the defines.h change I refer to is the one that removes the Mac clause. It fails earlier without that change (I've been hitting it on most builds now I have SYSTEM_ZLIB set), so the patch is an improvement, even if it doesn't work in all cases.
I'd guess you have STATIC_CXX set to true (the default apparently):
I didn't specify any other build options so it's using the default.
An odd default, though I can understand why, given OpenJDK's history.
BTW: I should mention that I don't have any issues with the patch proposed as it works as it does now because SYSTEM_ZLIB is false. It may be that there is follow-up to allow SYSTEM_ZLIB=true and static linking to work together.
Can I take that as it's good to push? :-D
-Alan.
Thanks, -- Andrew :) Free Java Software Engineer Red Hat, Inc. (http://www.redhat.com) PGP Key: 248BDC07 (https://keys.indymedia.org/) Fingerprint = EC5A 1F5E C0AD 1D15 8F1F 8F91 3B96 A578 248B DC07
Yes, you are good to go. Thanks! -Sherman On 8/14/2012 6:13 AM, Andrew Hughes wrote:
----- Original Message -----
On 14/08/2012 13:36, Andrew Hughes wrote:
:
Do you have a static libz.a installed (from zlib1g-dev) and all dependent static libraries? I think you're right about the static linking but this was just a quick test to see if SYSTEM_ZLIB=true worked with everything else as default.
Ah right.
pack200 has C++ so that explains why we see it there and not when building libzip.
Right. BTW, it appears gcc 4.5 and later have a new option -static-libstdc++. I don't know if that would work better than the options currently used.
http://gcc.gnu.org/gcc-4.5/changes.html
$ dpkg -s zlib1g-dev Package: zlib1g-dev Status: install ok installed Multi-Arch: same Priority: optional Section: libdevel Installed-Size: 366 Maintainer: Ubuntu Developers <ubuntu-devel-discuss@lists.ubuntu.com> Architecture: i386 Source: zlib Version: 1:1.2.3.4.dfsg-3ubuntu4 Provides: libz-dev Depends: zlib1g (= 1:1.2.3.4.dfsg-3ubuntu4), libc6-dev | libc-dev Conflicts: zlib1-dev Description: compression library - development zlib is a library implementing the deflate compression method found in gzip and PKZIP. This package includes the development support files. Homepage: http://zlib.net/ Original-Maintainer: Mark Brown <broonie@debian.org>
$ apt-file search libz.a lib64z1-dev: /usr/lib64/libz.a zlib1g-dev: /usr/lib/i386-linux-gnu/libz.a
$ ls -l /usr/lib/i386-linux-gnu/libz.a -rw-r--r-- 1 root root 98772 Nov 10 2011 /usr/lib/i386-linux-gnu/libz.a
I suspect you'll get the same failure with or without my patch (though you'll probably need the change to defines.h to get this far...) SYSTEM_ZLIB is currently Mac only so I wouldn't expect to have got very far without your changes. Yes, the defines.h change I refer to is the one that removes the Mac clause. It fails earlier without that change (I've been hitting it on most builds now I have SYSTEM_ZLIB set), so the patch is an improvement, even if it doesn't work in all cases.
I'd guess you have STATIC_CXX set to true (the default apparently): I didn't specify any other build options so it's using the default.
An odd default, though I can understand why, given OpenJDK's history.
BTW: I should mention that I don't have any issues with the patch proposed as it works as it does now because SYSTEM_ZLIB is false. It may be that there is follow-up to allow SYSTEM_ZLIB=true and static linking to work together.
Can I take that as it's good to push? :-D
-Alan.
Thanks,
----- Original Message -----
Yes, you are good to go.
Thanks! Pushed: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/35e024c6a62c
Thanks! -Sherman
On 8/14/2012 6:13 AM, Andrew Hughes wrote:
----- Original Message -----
On 14/08/2012 13:36, Andrew Hughes wrote:
:
Do you have a static libz.a installed (from zlib1g-dev) and all dependent static libraries? I think you're right about the static linking but this was just a quick test to see if SYSTEM_ZLIB=true worked with everything else as default.
Ah right.
pack200 has C++ so that explains why we see it there and not when building libzip.
Right. BTW, it appears gcc 4.5 and later have a new option -static-libstdc++. I don't know if that would work better than the options currently used.
http://gcc.gnu.org/gcc-4.5/changes.html
$ dpkg -s zlib1g-dev Package: zlib1g-dev Status: install ok installed Multi-Arch: same Priority: optional Section: libdevel Installed-Size: 366 Maintainer: Ubuntu Developers <ubuntu-devel-discuss@lists.ubuntu.com> Architecture: i386 Source: zlib Version: 1:1.2.3.4.dfsg-3ubuntu4 Provides: libz-dev Depends: zlib1g (= 1:1.2.3.4.dfsg-3ubuntu4), libc6-dev | libc-dev Conflicts: zlib1-dev Description: compression library - development zlib is a library implementing the deflate compression method found in gzip and PKZIP. This package includes the development support files. Homepage: http://zlib.net/ Original-Maintainer: Mark Brown <broonie@debian.org>
$ apt-file search libz.a lib64z1-dev: /usr/lib64/libz.a zlib1g-dev: /usr/lib/i386-linux-gnu/libz.a
$ ls -l /usr/lib/i386-linux-gnu/libz.a -rw-r--r-- 1 root root 98772 Nov 10 2011 /usr/lib/i386-linux-gnu/libz.a
I suspect you'll get the same failure with or without my patch (though you'll probably need the change to defines.h to get this far...) SYSTEM_ZLIB is currently Mac only so I wouldn't expect to have got very far without your changes. Yes, the defines.h change I refer to is the one that removes the Mac clause. It fails earlier without that change (I've been hitting it on most builds now I have SYSTEM_ZLIB set), so the patch is an improvement, even if it doesn't work in all cases.
I'd guess you have STATIC_CXX set to true (the default apparently): I didn't specify any other build options so it's using the default.
An odd default, though I can understand why, given OpenJDK's history.
BTW: I should mention that I don't have any issues with the patch proposed as it works as it does now because SYSTEM_ZLIB is false. It may be that there is follow-up to allow SYSTEM_ZLIB=true and static linking to work together.
Can I take that as it's good to push? :-D
-Alan.
Thanks,
-- Andrew :) Free Java Software Engineer Red Hat, Inc. (http://www.redhat.com) PGP Key: 248BDC07 (https://keys.indymedia.org/) Fingerprint = EC5A 1F5E C0AD 1D15 8F1F 8F91 3B96 A578 248B DC07
On 14/08/2012 14:13, Andrew Hughes wrote:
Can I take that as it's good to push? :-D No objection from me although if the default is changed to SYSTEM_ZLIB=true for Linux (and maybe Solaris) then it will require making sure that it builds everywhere without needing other options.
-Alan.
----- Original Message -----
On 14/08/2012 14:13, Andrew Hughes wrote:
Can I take that as it's good to push? :-D No objection from me although if the default is changed to SYSTEM_ZLIB=true for Linux (and maybe Solaris) then it will require making sure that it builds everywhere without needing other options.
I have no plans to do this, and certainly not in this patch. I wouldn't feel comfortable enabling it unless the default was changed to build with shared libraries, as that's what we've tested extensively.
From my side, I think the shared solution is what the average user would expect (on GNU/Linux anyway), as that's what pretty much every other codebase does, and that enabling the static options should be done in the Oracle builds. But that's a whole other debate. The current defaults are a result of the codebase being used (and still being used) to produce proprietary binaries for over ten years and so it's no surprise that there are conflicts with attempting to adapt it to a completely different paradigm.
-Alan.
-- Andrew :) Free Java Software Engineer Red Hat, Inc. (http://www.redhat.com) PGP Key: 248BDC07 (https://keys.indymedia.org/) Fingerprint = EC5A 1F5E C0AD 1D15 8F1F 8F91 3B96 A578 248B DC07
participants (4)
-
Alan Bateman
-
Andrew Hughes
-
Kumar Srinivasan
-
Xueming Shen