903620 - Strip jsshell prior to packaging.

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Dan Minor [:dminor]]

To support Bug 858621 to run jit-tests from the test package we need a copy of the js shell available to run the tests. This is already being packaged, but it is not stripped, which is not ideal for running tests on android (i.e. as part of Bug 858622).

This will have the additional benefit of saving storage on the ftp server, etc.

I spoke with Ted in IRC and he was not aware of any reason not to strip it.

Comment 1

[(not currently active) Ted Mielczarek]

I looked at the original bugs and I couldn't find any evidence that not stripping the binaries was an intentional decision, so I think stripping them should be fine.

Comment 2

[Marty Rosenberg [:mjrosenb]]

I'm not sure if it will actually matter in practice, but for personal use, I try to always split the binary into a stripped binary and a symbols-only file.  When debugging a remote program, gdb knows how to pull the symbols from a local symbols-only file, which is absolutely fantastic in my experience.

Comment 3

[(not currently active) Ted Mielczarek]

Note that as a side effect of building symbols for Breakpad we add gnu-debuglink to all our binaries (including the js shell) and upload the debug bits to the symbol server for nightlies. (We don't do that for normal tinderbox builds for space reasons.)

Comment 4

[(not currently active) Ted Mielczarek]

Attachment: rework JS shell packaging as a Python script

Rather than hack more shell crap into a Makefile, I redid the JS shell packaging using the existing packager code. This is a first pass, I'd like some guidance on whether I did things right. I had to do a two-pass copy-then-jar because ExecutableFiles don't like going into a Jarrer, and the whole point here is to get them stripped.

Comment 5

[Mike Hommey [:glandium]]

Comment on attachment 8368617 [details] [diff] [review]
rework JS shell packaging as a Python script

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

::: toolkit/mozapps/installer/packagejsshell.py
@@ +10,5 @@
> +    Jarrer,
> +)
> +from mozpack.errors import errors
> +import mozfile
> +import mozpack.path

as mozpath

@@ +29,5 @@
> +
> +    with errors.accumulate():
> +        finder = FileFinder(distbin)
> +        for b in bins:
> +            p = os.path.relpath(b, distbin)

mozpath.relpath

@@ +30,5 @@
> +    with errors.accumulate():
> +        finder = FileFinder(distbin)
> +        for b in bins:
> +            p = os.path.relpath(b, distbin)
> +            print "p: ", p

I guess you'll remove those debugging messages afterwards?

@@ +37,5 @@
> +                copier.add(_, f)
> +    print "found all files"
> +    with mozfile.TemporaryDirectory() as d:
> +        copier.copy(d)
> +        # double-copy because Jarrer can't deal with ExecutableFiles.

I think it would be better to just make the Jarrer happy with ExecutableFiles.
Something like this:

diff --git a/python/mozbuild/mozpack/files.py b/python/mozbuild/mozpack/files.py
--- a/python/mozbuild/mozpack/files.py
+++ b/python/mozbuild/mozpack/files.py
@@ -22,16 +22,17 @@ from mozpack.chrome.manifest import Mani
 from io import BytesIO
 from mozpack.errors import (
     ErrorMessage,
     errors,
 )
 from mozpack.mozjar import JarReader
 import mozpack.path
 from collections import OrderedDict
+from tempfile import mkstemp
 
 
 class Dest(object):
     '''
     Helper interface for BaseFile.copy. The interface works as follows:
     - read() and write() can be used to sequentially read/write from the
       underlying file.
     - a call to read() after a write() will re-open the underlying file and
@@ -174,31 +175,43 @@ class File(BaseFile):
 
 
 class ExecutableFile(File):
     '''
     File class for executable and library files on OS/2, OS/X and ELF systems.
     (see mozpack.executables.is_executable documentation).
     '''
     def copy(self, dest, skip_if_older=True):
+        real_dest = dest
+        if not isinstance(dest, basestring):
+            fd, dest = mkstemp()
+            os.close(fd)
+            os.remove(dest)
+
         assert isinstance(dest, basestring)
         # If File.copy didn't actually copy because dest is newer, check the
         # file sizes. If dest is smaller, it means it is already stripped and
         # elfhacked, so we can skip.
         if not File.copy(self, dest, skip_if_older) and \
                 os.path.getsize(self.path) > os.path.getsize(dest):
             return False
         try:
             if may_strip(dest):
                 strip(dest)
             if may_elfhack(dest):
                 elfhack(dest)
         except ErrorMessage:
             os.remove(dest)
             raise
+
+        if real_dest != dest:
+            f = File(dest)
+            ret = f.copy(real_dest, skip_if_older)
+            os.remove(dest)
+            return ret
         return True
 
 
 class AbsoluteSymlinkFile(File):
     '''File class that is copied by symlinking (if available).
 
     This class only works if the target path is absolute.
     '''

::: toolkit/mozapps/installer/packager.mk
@@ +132,5 @@
>    $(NULL)
>  endif # Darwin
>  endif # WINNT
>  endif # MOZ_STATIC_JS
> +MAKE_JSSHELL  = $(PYTHON) $(topsrcdir)/toolkit/mozapps/installer/packagejsshell.py $(PKG_JSSHELL) $(foreach b,$(JSSHELL_BINS),$(abspath $(b)))

$(abspath $(JSSHELL_BINS))

Note you might as well make the script generic in its name, since it doesn't really do anything jsshell specific.

Comment 6

[(not currently active) Ted Mielczarek]

Thanks for the feedback. You're right, the script is actually currently just "make a zip file". My thought was that maybe later we'd store the list of JSSHELL_BINS in moz.build instead and we could make this slightly smarter.

Comment 7

[(not currently active) Ted Mielczarek]

Attachment: Make ExecutableFile support being put directly into a jar

This is glandium's patch from the comment above.

Comment 8

[(not currently active) Ted Mielczarek]

Attachment: rework JS shell packaging as a Python script

This addresses all your comments.

Comment 9

[Mike Hommey [:glandium]]

Comment on attachment 8370252 [details] [diff] [review]
rework JS shell packaging as a Python script

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

r+ if you choose to remove the ArgumentParser, f+ if you choose to use an ArgumentParser (that is, if you do, i want to do a quick check)

::: toolkit/mozapps/installer/dozip.py
@@ +5,5 @@
> +# This script creates a zip file, but will also strip any binaries
> +# it finds before adding them to the zip.
> +
> +from mozpack.files import (
> +    FileFinder,

Might as well put that on one line

@@ +8,5 @@
> +from mozpack.files import (
> +    FileFinder,
> +)
> +from mozpack.copier import (
> +    FileCopier,

You're not using the FileCopier anymore

@@ +12,5 @@
> +    FileCopier,
> +    Jarrer,
> +)
> +from mozpack.errors import errors
> +import argparse

see below

@@ +24,5 @@
> +def main(args):
> +    parser = argparse.ArgumentParser()
> +    parser.add_argument("--base-dir", help="Store paths relative to this directory")
> +    parser.add_argument("url", nargs='?', default="http://localhost:8314",
> +                        help="URL of job queue server")

This ArgumentParser looks like it doesn't belong here. It's not used, and it doesn't match the command line at all. That being said, you could use an ArgumentParser for those arguments you get from the command line.

Comment 10

[(not currently active) Ted Mielczarek]

*sigh* I started adding the ArgumentParser and apparently lost track of things. I'll fix that. I wanted to add --base-dir instead of hardcoding dist/bin there.

Comment 11

[(not currently active) Ted Mielczarek]

Attachment: rework JS shell packaging as a Python script

Okay, sorry, this patch has the ArgumentParser actually hooked up like I intended.

Comment 12

[Mike Hommey [:glandium]]

Comment on attachment 8370745 [details] [diff] [review]
rework JS shell packaging as a Python script

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

::: toolkit/mozapps/installer/dozip.py
@@ +23,5 @@
> +                                             "dist", "bin"),
> +                        help="Store paths relative to this directory")
> +    parser.add_argument("zip", help="Path to zip file to write")
> +    parser.add_argument("input", nargs="+",
> +                        help="Path to file to add to zip")

files

::: toolkit/mozapps/installer/packager.mk
@@ +132,5 @@
>    $(NULL)
>  endif # Darwin
>  endif # WINNT
>  endif # MOZ_STATIC_JS
> +MAKE_JSSHELL  = $(PYTHON) $(topsrcdir)/toolkit/mozapps/installer/dozip.py $(PKG_JSSHELL) $(abspath $(JSSHELL_BINS))

Note, I wonder if it wouldn't be simpler to not use $(abspath) and to give file names relative to DIST/bin directly on the command line. We're not using JSSHELL_BINS for anything else anyways.

Comment 13

[Gregory Szorc [:gps]]

Comment on attachment 8370251 [details] [diff] [review]
Make ExecutableFile support being put directly into a jar

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

That's pretty hacky. I'd prefer to do this all in-memory. Are there things touching files requiring the temp file dance?

meh. r+

Comment 14

[Mike Hommey [:glandium]]

(In reply to Gregory Szorc [:gps] from comment #13)
> That's pretty hacky. I'd prefer to do this all in-memory. Are there things
> touching files requiring the temp file dance?

elfhack and strip. I'm not going to reimplement them in python.

Comment 15

[(not currently active) Ted Mielczarek]

Yeah, I looked for a bit to see if there was a way to coerce strip to output to stdout, but it doesn't seem like a supported mode of operation.

Comment 16

[(not currently active) Ted Mielczarek]

(In reply to Mike Hommey [:glandium] from comment #12)
> Note, I wonder if it wouldn't be simpler to not use $(abspath) and to give
> file names relative to DIST/bin directly on the command line. We're not
> using JSSHELL_BINS for anything else anyways.

This is probably sane, but I'd like to put it off to a followup. I'm hoping we can move more of this gunk out of the Makefile into moz.build and we can sort it then.

Comment 17

[(not currently active) Ted Mielczarek]

This was green on Try and I verified that the JS shell packages all contained what they were supposed to:
https://tbpl.mozilla.org/?tree=Try&rev=f01d10da0dd8

(they're all quite a bit smaller though!)

Comment 18

[(not currently active) Ted Mielczarek]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d37dcd149e8c
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/480f92acc087

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d37dcd149e8c
https://hg.mozilla.org/mozilla-central/rev/480f92acc087

Comment 20

[Dan Minor [:dminor]]

It looks like they are being packaged without the executable bit set for Linux and OS X, causing the jit-tests to fail from the test package on Cedar.