Last Comment Bug 763651 - link JS to zlib
: link JS to zlib
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: mozilla16
Assigned To: Mike Hommey [:glandium]
:
: Gregory Szorc [:gps]
Mentors:
Depends on: 773551 773967 776305 776704 780976 825915
Blocks: savesource
  Show dependency treegraph
 
Reported: 2012-06-11 13:15 PDT by :Benjamin Peterson
Modified: 2013-01-02 08:10 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
first try (1.88 KB, patch)
2012-06-14 11:08 PDT, :Benjamin Peterson
no flags Details | Diff | Splinter Review
second try (18.72 KB, patch)
2012-06-14 23:31 PDT, :Benjamin Peterson
no flags Details | Diff | Splinter Review
link into mozglue when possible (20.40 KB, patch)
2012-06-16 20:51 PDT, :Benjamin Peterson
mh+mozilla: review-
Details | Diff | Splinter Review
address review comments (10.55 KB, patch)
2012-06-21 00:12 PDT, :Benjamin Peterson
no flags Details | Diff | Splinter Review
PoC (11.91 KB, patch)
2012-06-21 06:59 PDT, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
PoC (15.97 KB, patch)
2012-06-21 07:24 PDT, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
PoC (19.16 KB, patch)
2012-07-02 10:53 PDT, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
Link the javascript engine against zlib (20.71 KB, patch)
2012-07-10 06:38 PDT, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
make standalone builds work (498 bytes, patch)
2012-07-10 10:03 PDT, :Benjamin Peterson
no flags Details | Diff | Splinter Review
Link the javascript engine against zlib (20.83 KB, patch)
2012-07-10 13:34 PDT, Mike Hommey [:glandium]
khuey: review+
Details | Diff | Splinter Review

Description :Benjamin Peterson 2012-06-11 13:15:27 PDT
As part of bug 761723, I'm compressing JS source from the parser. I'm using zlib for this, so I need the appropriate zlib (system or built-in) to the built and linked to the JS library.
Comment 1 Ted Mielczarek [:ted.mielczarek] 2012-06-11 13:19:34 PDT
This is going to suck a little bit on Windows because mozjs is a separate DLL, so we'll have to link zlib into both mozjs and xul.
Comment 2 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-06-11 13:33:30 PDT
Can't we just stick zlib in mozglue?
Comment 3 :Benjamin Peterson 2012-06-14 11:08:22 PDT
Created attachment 633194 [details] [diff] [review]
first try

Here is a patch which links libmozz into mozglue. It works fine when mozglue is a static library on Linux. When it's shared, the zlib symbols are not kept (or exported?), so building fails. I'd appreciate any advice on how to deal with this.
Comment 4 Mike Hommey [:glandium] 2012-06-14 11:33:30 PDT
(In reply to Benjamin Peterson from comment #3)
> Created attachment 633194 [details] [diff] [review]
> first try
> 
> Here is a patch which links libmozz into mozglue. It works fine when mozglue
> is a static library on Linux.

Are you sure?

> When it's shared, the zlib symbols are not
> kept (or exported?), so building fails. I'd appreciate any advice on how to
> deal with this.

You probably just need to set VISIBILITY_FLAGS to nothing in zlib's makefile.
Comment 5 :Benjamin Peterson 2012-06-14 13:56:47 PDT
(In reply to Mike Hommey [:glandium] from comment #4)
> (In reply to Benjamin Peterson from comment #3)
> > Created attachment 633194 [details] [diff] [review]
> > first try
> > 
> > Here is a patch which links libmozz into mozglue. It works fine when mozglue
> > is a static library on Linux.
> 
> Are you sure?

Yes. For example https://tbpl.mozilla.org/php/getParsedLog.php?id=12650568&tree=Try&full=1

> 
> > When it's shared, the zlib symbols are not
> > kept (or exported?), so building fails. I'd appreciate any advice on how to
> > deal with this.
> 
> You probably just need to set VISIBILITY_FLAGS to nothing in zlib's makefile.

I wonder if the problem is the linker is not retaining the zlib symbols in the shared library since no one uses them.
Comment 6 Mike Hommey [:glandium] 2012-06-14 22:56:30 PDT
(In reply to Benjamin Peterson from comment #5)
> (In reply to Mike Hommey [:glandium] from comment #4)
> > (In reply to Benjamin Peterson from comment #3)
> > > Created attachment 633194 [details] [diff] [review]
> > > first try
> > > 
> > > Here is a patch which links libmozz into mozglue. It works fine when mozglue
> > > is a static library on Linux.
> > 
> > Are you sure?
> 
> Yes. For example
> https://tbpl.mozilla.org/php/getParsedLog.php?id=12650568&tree=Try&full=1

It's not because it builds that it does the right thing. I'll check.

> > > When it's shared, the zlib symbols are not
> > > kept (or exported?), so building fails. I'd appreciate any advice on how to
> > > deal with this.
> > 
> > You probably just need to set VISIBILITY_FLAGS to nothing in zlib's makefile.
> 
> I wonder if the problem is the linker is not retaining the zlib symbols in
> the shared library since no one uses them.

They are probably not exported (thus resetting VISIBILITY_FLAGS).
Comment 7 :Benjamin Peterson 2012-06-14 23:31:10 PDT
Created attachment 633410 [details] [diff] [review]
second try

Here's a second iteration. It works if WRAP_SYSTEM_INCLUDES is on (thanks khuey), but fails when -fvisibility=hidden is being used everywhere. I'm not sure what needs to be flipped now.
Comment 8 Mike Hommey [:glandium] 2012-06-14 23:41:58 PDT
Comment on attachment 633410 [details] [diff] [review]
second try

Review of attachment 633410 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/configure.in
@@ +3494,5 @@
> +     [ZLIB_CFLAGS=$withval
> +      MOZ_NATIVE_ZLIB=])
> +
> +if test -n "$MOZ_NATIVE_ZLIB"; then
> +   PKG_CHECK_MODULES(ZLIB, zlib)

The main configure doesn't do this check with pkg-config. It would be better if both did the same thing. Depending on how old the pkg-config file is for zlib, that could mean changing the main configure.in. However, if it's rather new, I'd go for using the same thing as the main configure.

::: mozglue/build/Makefile.in
@@ +18,5 @@
>  # If this is ever changed, update MOZ_SHARED_MOZGLUE in browser/installer/Makefile.in
>  ifneq (,$(filter WINNT Darwin Android,$(OS_TARGET)))
>  FORCE_SHARED_LIB = 1
>  else
> +FORCE_SHARED_LIB = 1

You can't do that.

@@ +30,1 @@
>  CPPSRCS = dummy.cpp

You can remove this and the comment. As well as the dummy.cpp file.
Comment 9 Mike Hommey [:glandium] 2012-06-14 23:49:01 PDT
(In reply to Mike Hommey [:glandium] from comment #6)
> (In reply to Benjamin Peterson from comment #5)
> > (In reply to Mike Hommey [:glandium] from comment #4)
> > > (In reply to Benjamin Peterson from comment #3)
> > > > Created attachment 633194 [details] [diff] [review]
> > > > first try
> > > > 
> > > > Here is a patch which links libmozz into mozglue. It works fine when mozglue
> > > > is a static library on Linux.
> > > 
> > > Are you sure?
> > 
> > Yes. For example
> > https://tbpl.mozilla.org/php/getParsedLog.php?id=12650568&tree=Try&full=1
> 
> It's not because it builds that it does the right thing. I'll check.

I see why this works: you statically link zlib in mozglue *and* libxul.

Here's how I see it: you only really need to make it part of mozglue when js is built as a shared library. Leave it linked to libxul when it's not.
Comment 10 :Benjamin Peterson 2012-06-15 17:04:36 PDT
(In reply to Mike Hommey [:glandium] from comment #9)
> Here's how I see it: you only really need to make it part of mozglue when js
> is built as a shared library. Leave it linked to libxul when it's not.

The problem is we still need to link the js shell.
Comment 11 Mike Hommey [:glandium] 2012-06-15 22:46:24 PDT
(In reply to Benjamin Peterson from comment #10)
> (In reply to Mike Hommey [:glandium] from comment #9)
> > Here's how I see it: you only really need to make it part of mozglue when js
> > is built as a shared library. Leave it linked to libxul when it's not.
> 
> The problem is we still need to link the js shell.

Add it to the shell, then. You already have to add something in case you use native zlib.
Comment 12 :Benjamin Peterson 2012-06-16 20:51:45 PDT
Created attachment 633863 [details] [diff] [review]
link into mozglue when possible
Comment 13 Mike Hommey [:glandium] 2012-06-16 23:20:46 PDT
Comment on attachment 633863 [details] [diff] [review]
link into mozglue when possible

Review of attachment 633863 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/configure.in
@@ +3491,5 @@
> +
> +MOZ_ARG_ENABLE_BOOL(moz-zlib,
> +[  --enable-moz-zlib=FLAGS   INTERNAL build with builtin Mozilla zlib],
> +     [ZLIB_LIBS='$(call EXPAND_LIBNAME_PATH,mozz,../../$(DEPTH)/modules/zlib/src)'
> +      ZLIB_DIR=])

I think it would be better to define these in the main configure.in, and export them from there, where they can be picked up without an option. The check below would then pick these values, provided they're not reset above.

@@ +3507,5 @@
> +fi
> +if test -z "$ZLIB_DIR" -o "$ZLIB_DIR" = no; then
> +    SYSTEM_ZLIB=
> +elif test "$ZLIB_DIR" != yes; then
> +    PKG_CHECK_MODULES(ZLIB, zlib)

If you're doing that, please do the same in the main configure.

@@ +3536,5 @@
> +fi
> +
> +ZLIB_PROG_LIBS="${ZLIB_LIBS}"
> +
> +if test "$MOZ_GLUE_PROGRAM_LDFLAGS" -a "$OS_ARCH" != "WINNT"; then

This should be test -z "$MOZ_GLUE_PROGRAM_LDFLAGS", shouldn't it? Or, on the contrary, just set ZLIB_PROG_LIBS when MOZ_GLUE_PROGRAM_LDFLAGS is set.
I also don't think you need to check OS_ARCH.

::: modules/zlib/src/Makefile.in
@@ -14,5 @@
>  MODULE		= zlib
>  LIBRARY_NAME	= mozz
>  GRE_MODULE	= 1
>  LIBXUL_LIBRARY = 1
> -DIST_INSTALL = 1

Is there a particular reason why you remove this?

::: tools/profiler/libunwind/src/Makefile.in
@@ +16,5 @@
>  @SET_MAKE@
>  
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.

You don't want to make these libunwind changes part of this patch :)
Comment 14 Mike Hommey [:glandium] 2012-06-20 00:36:44 PDT
Please note that I landed bug 763987 and bug 764046, which conflicts with some parts of your patch.
Comment 15 :Benjamin Peterson 2012-06-20 11:20:10 PDT
(In reply to Mike Hommey [:glandium] from comment #13)
> Comment on attachment 633863 [details] [diff] [review]
> link into mozglue when possible
> 
> Review of attachment 633863 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/configure.in
> @@ +3491,5 @@
> > +
> > +MOZ_ARG_ENABLE_BOOL(moz-zlib,
> > +[  --enable-moz-zlib=FLAGS   INTERNAL build with builtin Mozilla zlib],
> > +     [ZLIB_LIBS='$(call EXPAND_LIBNAME_PATH,mozz,../../$(DEPTH)/modules/zlib/src)'
> > +      ZLIB_DIR=])
> 
> I think it would be better to define these in the main configure.in, and
> export them from there, where they can be picked up without an option. The
> check below would then pick these values, provided they're not reset above.
> 
> @@ +3507,5 @@
> > +fi
> > +if test -z "$ZLIB_DIR" -o "$ZLIB_DIR" = no; then
> > +    SYSTEM_ZLIB=
> > +elif test "$ZLIB_DIR" != yes; then
> > +    PKG_CHECK_MODULES(ZLIB, zlib)
> 
> If you're doing that, please do the same in the main configure.

The reason I did this, is so is you build js standalone, you get system zlib by default. This doesn't seem to be what the main configure is interested in.

> 
> @@ +3536,5 @@
> > +fi
> > +
> > +ZLIB_PROG_LIBS="${ZLIB_LIBS}"
> > +
> > +if test "$MOZ_GLUE_PROGRAM_LDFLAGS" -a "$OS_ARCH" != "WINNT"; then
> 
> This should be test -z "$MOZ_GLUE_PROGRAM_LDFLAGS", shouldn't it? Or, on the
> contrary, just set ZLIB_PROG_LIBS when MOZ_GLUE_PROGRAM_LDFLAGS is set.
> I also don't think you need to check OS_ARCH.

We only want to link zlib in the js programs if mozglue is not being linked into them.

Checking OS_ARCH was the only way I could convince it to build on Windows.

> 
> ::: modules/zlib/src/Makefile.in
> @@ -14,5 @@
> >  MODULE		= zlib
> >  LIBRARY_NAME	= mozz
> >  GRE_MODULE	= 1
> >  LIBXUL_LIBRARY = 1
> > -DIST_INSTALL = 1
> 
> Is there a particular reason why you remove this?

It somehow allows the zlib symbols to be exported from the mozglue library.

> 
> ::: tools/profiler/libunwind/src/Makefile.in
> @@ +16,5 @@
> >  @SET_MAKE@
> >  
> > +# This Source Code Form is subject to the terms of the Mozilla Public
> > +# License, v. 2.0. If a copy of the MPL was not distributed with this
> > +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> 
> You don't want to make these libunwind changes part of this patch :)

Curiously, running autoreconf-2.13 causes this.
Comment 16 Mike Hommey [:glandium] 2012-06-20 11:37:24 PDT
(In reply to Benjamin Peterson from comment #15)
> (In reply to Mike Hommey [:glandium] from comment #13)
> > Comment on attachment 633863 [details] [diff] [review]
> > link into mozglue when possible
> > 
> > Review of attachment 633863 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: js/src/configure.in
> > @@ +3491,5 @@
> > > +
> > > +MOZ_ARG_ENABLE_BOOL(moz-zlib,
> > > +[  --enable-moz-zlib=FLAGS   INTERNAL build with builtin Mozilla zlib],
> > > +     [ZLIB_LIBS='$(call EXPAND_LIBNAME_PATH,mozz,../../$(DEPTH)/modules/zlib/src)'
> > > +      ZLIB_DIR=])
> > 
> > I think it would be better to define these in the main configure.in, and
> > export them from there, where they can be picked up without an option. The
> > check below would then pick these values, provided they're not reset above.
> > 
> > @@ +3507,5 @@
> > > +fi
> > > +if test -z "$ZLIB_DIR" -o "$ZLIB_DIR" = no; then
> > > +    SYSTEM_ZLIB=
> > > +elif test "$ZLIB_DIR" != yes; then
> > > +    PKG_CHECK_MODULES(ZLIB, zlib)
> > 
> > If you're doing that, please do the same in the main configure.
> 
> The reason I did this, is so is you build js standalone, you get system zlib
> by default. This doesn't seem to be what the main configure is interested in.

I meant the PKG_CHECK_MODULES part. The main configure doesn't use that.

> > @@ +3536,5 @@
> > > +fi
> > > +
> > > +ZLIB_PROG_LIBS="${ZLIB_LIBS}"
> > > +
> > > +if test "$MOZ_GLUE_PROGRAM_LDFLAGS" -a "$OS_ARCH" != "WINNT"; then
> > 
> > This should be test -z "$MOZ_GLUE_PROGRAM_LDFLAGS", shouldn't it? Or, on the
> > contrary, just set ZLIB_PROG_LIBS when MOZ_GLUE_PROGRAM_LDFLAGS is set.
> > I also don't think you need to check OS_ARCH.
> 
> We only want to link zlib in the js programs if mozglue is not being linked
> into them.

Actually, you do want to link zlib in the js programs if mozglue *is* being linked (statically), and only link zlib in mozglue if mozglue is a shared library.
 
> Checking OS_ARCH was the only way I could convince it to build on Windows.
> 
> > 
> > ::: modules/zlib/src/Makefile.in
> > @@ -14,5 @@
> > >  MODULE		= zlib
> > >  LIBRARY_NAME	= mozz
> > >  GRE_MODULE	= 1
> > >  LIBXUL_LIBRARY = 1
> > > -DIST_INSTALL = 1
> > 
> > Is there a particular reason why you remove this?
> 
> It somehow allows the zlib symbols to be exported from the mozglue library.

That sounds unexpected.

> > ::: tools/profiler/libunwind/src/Makefile.in
> > @@ +16,5 @@
> > >  @SET_MAKE@
> > >  
> > > +# This Source Code Form is subject to the terms of the Mozilla Public
> > > +# License, v. 2.0. If a copy of the MPL was not distributed with this
> > > +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> > 
> > You don't want to make these libunwind changes part of this patch :)
> 
> Curiously, running autoreconf-2.13 causes this.

You should just use make -f client.mk, it will update configure.
Comment 17 :Benjamin Peterson 2012-06-21 00:12:28 PDT
Created attachment 635205 [details] [diff] [review]
address review comments
Comment 18 :Benjamin Peterson 2012-06-21 00:14:58 PDT
(In reply to Mike Hommey [:glandium] from comment #16)
> (In reply to Benjamin Peterson from comment #15)
> > (In reply to Mike Hommey [:glandium] from comment #13)
> > > Comment on attachment 633863 [details] [diff] [review]
> > > link into mozglue when possible
> > > 
> > > Review of attachment 633863 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > ::: js/src/configure.in
> > > @@ +3491,5 @@
> > > > +
> > > > +MOZ_ARG_ENABLE_BOOL(moz-zlib,
> > > > +[  --enable-moz-zlib=FLAGS   INTERNAL build with builtin Mozilla zlib],
> > > > +     [ZLIB_LIBS='$(call EXPAND_LIBNAME_PATH,mozz,../../$(DEPTH)/modules/zlib/src)'
> > > > +      ZLIB_DIR=])
> > > 
> > > I think it would be better to define these in the main configure.in, and
> > > export them from there, where they can be picked up without an option. The
> > > check below would then pick these values, provided they're not reset above.
> > > 
> > > @@ +3507,5 @@
> > > > +fi
> > > > +if test -z "$ZLIB_DIR" -o "$ZLIB_DIR" = no; then
> > > > +    SYSTEM_ZLIB=
> > > > +elif test "$ZLIB_DIR" != yes; then
> > > > +    PKG_CHECK_MODULES(ZLIB, zlib)
> > > 
> > > If you're doing that, please do the same in the main configure.
> > 
> > The reason I did this, is so is you build js standalone, you get system zlib
> > by default. This doesn't seem to be what the main configure is interested in.
> 
> I meant the PKG_CHECK_MODULES part. The main configure doesn't use that.

Yes, I was also talking about that. I didn't think this was a useful behaviour for the main configure to have.

> 
> > > @@ +3536,5 @@
> > > > +fi
> > > > +
> > > > +ZLIB_PROG_LIBS="${ZLIB_LIBS}"
> > > > +
> > > > +if test "$MOZ_GLUE_PROGRAM_LDFLAGS" -a "$OS_ARCH" != "WINNT"; then
> > > 
> > > This should be test -z "$MOZ_GLUE_PROGRAM_LDFLAGS", shouldn't it? Or, on the
> > > contrary, just set ZLIB_PROG_LIBS when MOZ_GLUE_PROGRAM_LDFLAGS is set.
> > > I also don't think you need to check OS_ARCH.
> > 
> > We only want to link zlib in the js programs if mozglue is not being linked
> > into them.
> 
> Actually, you do want to link zlib in the js programs if mozglue *is* being
> linked (statically), and only link zlib in mozglue if mozglue is a shared
> library.

Why not if it's static?

>  
> > Checking OS_ARCH was the only way I could convince it to build on Windows.
> > 
> > > 
> > > ::: modules/zlib/src/Makefile.in
> > > @@ -14,5 @@
> > > >  MODULE		= zlib
> > > >  LIBRARY_NAME	= mozz
> > > >  GRE_MODULE	= 1
> > > >  LIBXUL_LIBRARY = 1
> > > > -DIST_INSTALL = 1
> > > 
> > > Is there a particular reason why you remove this?
> > 
> > It somehow allows the zlib symbols to be exported from the mozglue library.
> 
> That sounds unexpected.

It was on advice from khuey.
Comment 19 Mike Hommey [:glandium] 2012-06-21 00:30:45 PDT
(In reply to Benjamin Peterson from comment #18)
> > > > > +MOZ_ARG_ENABLE_BOOL(moz-zlib,
> > > > > +[  --enable-moz-zlib=FLAGS   INTERNAL build with builtin Mozilla zlib],
> > > > > +     [ZLIB_LIBS='$(call EXPAND_LIBNAME_PATH,mozz,../../$(DEPTH)/modules/zlib/src)'
> > > > > +      ZLIB_DIR=])
> > > > 
> > > > I think it would be better to define these in the main configure.in, and
> > > > export them from there, where they can be picked up without an option. The
> > > > check below would then pick these values, provided they're not reset above.

You didn't address this in your patch.

> > > > @@ +3507,5 @@
> > > > > +fi
> > > > > +if test -z "$ZLIB_DIR" -o "$ZLIB_DIR" = no; then
> > > > > +    SYSTEM_ZLIB=
> > > > > +elif test "$ZLIB_DIR" != yes; then
> > > > > +    PKG_CHECK_MODULES(ZLIB, zlib)
> > > > 
> > > > If you're doing that, please do the same in the main configure.
> > > 
> > > The reason I did this, is so is you build js standalone, you get system zlib
> > > by default. This doesn't seem to be what the main configure is interested in.
> > 
> > I meant the PKG_CHECK_MODULES part. The main configure doesn't use that.
> 
> Yes, I was also talking about that. I didn't think this was a useful
> behaviour for the main configure to have.

Having native zlib enabled by default surely isn't wanted in the main configure at this point (although i do think we could), but using pkg-config when it is enabled surely is.

> > Actually, you do want to link zlib in the js programs if mozglue *is* being
> > linked (statically), and only link zlib in mozglue if mozglue is a shared
> > library.
> 
> Why not if it's static?

See comment 9. Linking zlib in static mozglue and only there, on unix, opens a can of worms you don't want to open.

> > > Checking OS_ARCH was the only way I could convince it to build on Windows.
> > > 
> > > > 
> > > > ::: modules/zlib/src/Makefile.in
> > > > @@ -14,5 @@
> > > > >  MODULE		= zlib
> > > > >  LIBRARY_NAME	= mozz
> > > > >  GRE_MODULE	= 1
> > > > >  LIBXUL_LIBRARY = 1
> > > > > -DIST_INSTALL = 1
> > > > 
> > > > Is there a particular reason why you remove this?
> > > 
> > > It somehow allows the zlib symbols to be exported from the mozglue library.
> > 
> > That sounds unexpected.
> 
> It was on advice from khuey.

The advice was probably about VISIBILITY_FLAGS, not DIST_INSTALL. It shouldn't matter much that DIST_INSTALL is removed, anyways, but it would be better to avoid unrelated changes.
Comment 20 Mike Hommey [:glandium] 2012-06-21 06:59:46 PDT
Created attachment 635292 [details] [diff] [review]
PoC

This should do what you want. It would need some testing, though. (I pushed it to try, for starters)
Comment 21 Mike Hommey [:glandium] 2012-06-21 07:24:23 PDT
Created attachment 635300 [details] [diff] [review]
PoC

It's better with the new files.
Comment 22 :Benjamin Peterson 2012-06-21 10:11:06 PDT
The latest patch(In reply to Mike Hommey [:glandium] from comment #20)
> Created attachment 635292 [details] [diff] [review]
> PoC
> 
> This should do what you want. It would need some testing, though. (I pushed
> it to try, for starters)

Thank you very much. It's broken on Windows, though: https://tbpl.mozilla.org/?tree=Try&rev=c6c9b86590f3
Comment 23 :Benjamin Peterson 2012-06-21 14:55:14 PDT
It also fails for standalone js builds.
Comment 24 Mike Hommey [:glandium] 2012-06-21 23:16:27 PDT
(In reply to Benjamin Peterson from comment #23)
> It also fails for standalone js builds.

On what platform, with what error?
Comment 25 :Benjamin Peterson 2012-06-21 23:23:45 PDT
(In reply to Mike Hommey [:glandium] from comment #24)
> (In reply to Benjamin Peterson from comment #23)
> > It also fails for standalone js builds.
> 
> On what platform, with what error?

Linux 64:

$ mkdir _DBG.OBJ && cd _DBG.OBJ
$ ../configure --enable-debug --disable-optimize && make -j9
...
/usr/bin/python2.7 ../config/pythonpath.py -I./config ../config/expandlibs_exec.py --depend .deps/libmozjs.pp --target libmozjs.so --uselist --  c++  -pedantic -Wall -Wpointer-arith -Woverloaded-virtual -Werror=return-type -Wtype-limits -Wempty-body -Wno-ctor-dtor-privacy -Wno-overlength-strings -Wno-invalid-offsetof -Wno-variadic-macros -Wcast-align -Wno-long-long -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -pthread -pipe  -DDEBUG -D_DEBUG -DTRACING -g -fno-omit-frame-pointer  -DUSE_SYSTEM_MALLOC=1 -DENABLE_ASSEMBLER=1 -DENABLE_JIT=1 -fPIC -shared -Wl,-z,defs -Wl,--gc-sections -Wl,-h,libmozjs.so -o libmozjs.so  bignum-dtoa.o bignum.o cached-powers.o diy-fp.o double-conversion.o fast-dtoa.o fixed-dtoa.o strtod.o jsalloc.o jsanalyze.o jsapi.o jsarray.o jsatom.o jsbool.o jsclone.o jscntxt.o jscompartment.o jsdate.o jsdbgapi.o jsdhash.o jsdtoa.o jsexn.o jsfriendapi.o jsfun.o jsgc.o jscrashreport.o jshash.o jsinfer.o jsinterp.o jsiter.o jslog2.o jsmath.o jsnativestack.o jsnum.o jsobj.o json.o jsonparser.o jsopcode.o jsproxy.o jsprf.o jsprobes.o jspropertycache.o jspropertytree.o jsreflect.o jsscope.o jsscript.o jsstr.o jstypedarray.o jsutil.o jswatchpoint.o jsweakmap.o jswrapper.o jsxml.o prmjtime.o sharkctl.o ArgumentsObject.o ScopeObject.o Debugger.o GlobalObject.o MethodGuard.o ObjectImpl.o Stack.o String.o BytecodeCompiler.o BytecodeEmitter.o FoldConstants.o ParseMaps.o ParseNode.o Parser.o SemanticAnalysis.o TokenStream.o TreeContext.o TestingFunctions.o LifoAlloc.o MapObject.o MemoryMetrics.o RegExpObject.o RegExpStatics.o RegExp.o Marking.o Memory.o Statistics.o StringBuffer.o Unicode.o Xdr.o MethodJIT.o StubCalls.o Compiler.o FrameState.o FastArithmetic.o FastBuiltins.o FastOps.o LoopState.o StubCompiler.o MonoIC.o PolyIC.o ImmutableSync.o InvokeHelpers.o Retcon.o TrampolineCompiler.o ExecutableAllocator.o PageBlock.o YarrInterpreter.o YarrPattern.o YarrSyntaxChecker.o Logging.o ExecutableAllocatorPosix.o OSAllocatorPosix.o ARMAssembler.o MacroAssemblerARM.o MacroAssemblerX86Common.o YarrJIT.o jsperf.o pm_linux.o HashFunctions.o    -lpthread    -Wl,-rpath-link,./dist/bin -Wl,-rpath-link,/usr/local/lib     -ldl  -lm -ldl  
jsutil.o: In function `js::TryCompressString(unsigned char const*, unsigned long, unsigned char*, unsigned long*)':
/home/benjamin/dev/repos/mozilla/central/js/src/jsutil.cpp:59: undefined reference to `deflateInit_'
/home/benjamin/dev/repos/mozilla/central/js/src/jsutil.cpp:64: undefined reference to `deflate'
/home/benjamin/dev/repos/mozilla/central/js/src/jsutil.cpp:65: undefined reference to `deflateEnd'
jsutil.o: In function `js::DecompressString(unsigned char const*, unsigned long, unsigned char*, unsigned long)':
/home/benjamin/dev/repos/mozilla/central/js/src/jsutil.cpp:86: undefined reference to `inflateInit_'
/home/benjamin/dev/repos/mozilla/central/js/src/jsutil.cpp:96: undefined reference to `inflate'
/home/benjamin/dev/repos/mozilla/central/js/src/jsutil.cpp:98: undefined reference to `inflateEnd'
collect2: ld returned 1 exit status
make[1]: *** [libmozjs.so] Error 1
make[1]: Leaving directory `/home/benjamin/dev/repos/mozilla/central/js/src/_DBG.OBJ'
make: *** [default] Error 2


(Note this is with the patch for bug 761723, which actually uses zlib, applied.)
Comment 26 Mike Hommey [:glandium] 2012-07-02 10:53:28 PDT
Created attachment 638413 [details] [diff] [review]
PoC

This works on win32 (but fails on win64 because mozjs is not built as a separate lib there, and that makes the assumptions from this patch fail)
Comment 27 Mike Hommey [:glandium] 2012-07-10 06:38:36 PDT
Created attachment 640579 [details] [diff] [review]
Link the javascript engine against zlib
Comment 28 Mike Hommey [:glandium] 2012-07-10 06:40:29 PDT
(In reply to Mike Hommey [:glandium] from comment #27)
> Created attachment 640579 [details] [diff] [review]
> Link the javascript engine against zlib

This passed try: https://tbpl.mozilla.org/?tree=Try&rev=d78cdbe17c10
I also tested Benjamin's standalone case from comment 25.
Comment 29 :Benjamin Peterson 2012-07-10 10:03:59 PDT
Created attachment 640654 [details] [diff] [review]
make standalone builds work

This one-liner is needed to make JS standalone builds work.
Comment 30 Mike Hommey [:glandium] 2012-07-10 10:09:20 PDT
Ah damn, my test was useless because there's nothing in js actually using zlib, so i didn't get the failure. Unfortunately, your patch breaks windows.
Comment 31 Mike Hommey [:glandium] 2012-07-10 10:09:40 PDT
Comment on attachment 640579 [details] [diff] [review]
Link the javascript engine against zlib

Needs a fix.
Comment 32 Mike Hommey [:glandium] 2012-07-10 13:34:25 PDT
Created attachment 640759 [details] [diff] [review]
Link the javascript engine against zlib

https://tbpl.mozilla.org/?tree=Try&rev=4548f9c0f23d
Comment 34 Ed Morley [:emorley] 2012-07-12 09:36:11 PDT
https://hg.mozilla.org/mozilla-central/rev/4be97839a566
Comment 35 Octoploid 2012-07-23 04:25:48 PDT
Profiled build on Linux is still broken:

 % make -f client.mk profiledbuild
...
/usr/bin/python2.7 /var/tmp/mozilla-central/js/src/config/pythonpath.py -I../config /var/tmp/mozilla-central/js/src/config/expandlibs_exec.py --depend .deps/js.pp --target js --uselist --  c++ -o js  -pedantic -Wall -Wpointer-arith -Woverloaded-virtual -Werror=return-type -Wtype-limits -Wempty-body -Wno-ctor-dtor-privacy -Wno-overlength-strings -Wno-invalid-offsetof -Wno-variadic-macros -Wcast-align -Wno-long-long -march=native -Wno-delete-non-virtual-dtor -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -pthread -pipe  -DNDEBUG -DTRIMMED -fprofile-generate -O3 -fomit-frame-pointer js.i_o jsoptparse.i_o jsheaptools.i_o   -lpthread -fuse-linker-plugin -Wl,-O1,--hash-style=gnu,--no-keep-memory -Wl,--icf=safe  -fprofile-generate -Wl,-rpath-link,../../../dist/bin -Wl,-rpath-link,/var/tmp/moz-build-dir/dist/lib   -L../../../dist/bin -L../../../dist/lib -L/var/tmp/moz-build-dir/dist/lib -lplds4 -lplc4 -lnspr4 -lpthread -ldl ../editline/libeditline.a ../libjs_static.a -lz -Wl,--whole-archive ../../../dist/lib/libmozglue.a ../../../dist/lib/libmemory.a -Wl,--no-whole-archive -rdynamic -ldl    
/usr/lib/gcc/x86_64-pc-linux-gnu/4.8.0/../../../../x86_64-pc-linux-gnu/bin/ld: error: hidden symbol 'inflateInit_' is not defined locally
/usr/lib/gcc/x86_64-pc-linux-gnu/4.8.0/../../../../x86_64-pc-linux-gnu/bin/ld: error: hidden symbol 'inflate' is not defined locally
/usr/lib/gcc/x86_64-pc-linux-gnu/4.8.0/../../../../x86_64-pc-linux-gnu/bin/ld: error: hidden symbol 'inflateEnd' is not defined locally
/usr/lib/gcc/x86_64-pc-linux-gnu/4.8.0/../../../../x86_64-pc-linux-gnu/bin/ld: error: hidden symbol 'deflateInit_' is not defined locally
/usr/lib/gcc/x86_64-pc-linux-gnu/4.8.0/../../../../x86_64-pc-linux-gnu/bin/ld: error: hidden symbol 'deflate' is not defined locally
/usr/lib/gcc/x86_64-pc-linux-gnu/4.8.0/../../../../x86_64-pc-linux-gnu/bin/ld: error: hidden symbol 'deflateEnd' is not defined locally
collect2: error: ld returned 1 exit status
make[6]: *** [js] Error 1
Comment 36 Octoploid 2012-07-23 04:28:56 PDT
Please ignore. I see this is fixed in Bug 776305.

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