Strip jsshell prior to packaging.

RESOLVED FIXED in mozilla30

Status

()

Core
Build Config
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: dminor, Assigned: ted)

Tracking

unspecified
mozilla30
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

4 years ago
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.
(Assignee)

Comment 1

4 years ago
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.
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.
(Assignee)

Comment 3

4 years ago
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.)
(Assignee)

Comment 4

4 years ago
Created attachment 8368617 [details] [diff] [review]
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.
Attachment #8368617 - Flags: review?(mh+mozilla)
(Assignee)

Updated

4 years ago
Assignee: nobody → ted
Status: NEW → ASSIGNED
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.
Attachment #8368617 - Flags: review?(mh+mozilla) → feedback+
(Assignee)

Comment 6

4 years ago
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.
(Assignee)

Comment 7

4 years ago
Created attachment 8370251 [details] [diff] [review]
Make ExecutableFile support being put directly into a jar

This is glandium's patch from the comment above.
Attachment #8370251 - Flags: review?(gps)
(Assignee)

Comment 8

4 years ago
Created attachment 8370252 [details] [diff] [review]
rework JS shell packaging as a Python script

This addresses all your comments.
Attachment #8370252 - Flags: review?(mh+mozilla)
(Assignee)

Updated

4 years ago
Attachment #8368617 - Attachment is obsolete: true
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.
Attachment #8370252 - Flags: review?(mh+mozilla) → review+
(Assignee)

Comment 10

4 years ago
*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.
(Assignee)

Comment 11

4 years ago
Created attachment 8370745 [details] [diff] [review]
rework JS shell packaging as a Python script

Okay, sorry, this patch has the ArgumentParser actually hooked up like I intended.
Attachment #8370745 - Flags: review?(mh+mozilla)
(Assignee)

Updated

4 years ago
Attachment #8370252 - Attachment is obsolete: true
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.
Attachment #8370745 - Flags: review?(mh+mozilla) → review+
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+
Attachment #8370251 - Flags: review?(gps) → review+
(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.
(Assignee)

Comment 15

4 years ago
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.
(Assignee)

Comment 16

4 years ago
(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.
(Assignee)

Comment 17

4 years ago
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!)
(Assignee)

Comment 18

4 years ago
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d37dcd149e8c
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/480f92acc087
https://hg.mozilla.org/mozilla-central/rev/d37dcd149e8c
https://hg.mozilla.org/mozilla-central/rev/480f92acc087
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
(Reporter)

Comment 20

4 years ago
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.
(Assignee)

Updated

4 years ago
Depends on: 971802
You need to log in before you can comment on or make changes to this bug.