589368 - Locale repacking support for jar reordering

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[(dormant account)]

Attachment: magic patch

Added ability to reverse jar optimization. --deoptimize mode takes a list of optimized jars and produces a bunch of boring jars + startup logs.
Locale logic then does s/en-US/relevant-locale/ and reoptimizes jars.

Kyle, asking for your review first for build stuff(perhaps there is a better way to do my perl oneliner?).

Comment 1

[Axel Hecht [:Pike]]

Comment on attachment 467984 [details] [diff] [review]
magic patch

Some python style nits:

if inlog <> None:

is commonly wrote as

if inlog is not None:
and the opposite as
if inlog is None:

On the makefile, use sed or python? No pearls for perl.

I didn't do a thorough review, but didn't see anything obvious.

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to comment #1)
> On the makefile, use sed or python? No pearls for perl.

I haven't looked at this closely, but yes, please use sed.

Comment 3

[Benjamin Smedberg]

I don't understand the need to deoptimize the JAR. Can't we create a startup log (if we need to) from optimized JARs, instead of "boring" JARs? The actual code to read the JARs isn't going to change, is it?

Comment 4

[(dormant account)]

(In reply to comment #3)
> I don't understand the need to deoptimize the JAR. Can't we create a startup
> log (if we need to) from optimized JARs, instead of "boring" JARs? The actual
> code to read the JARs isn't going to change, is it?

I do create a startup log while deoptimizing the jar. The main reason for deoptimizing is that unzip has braindamaged error checks. It unzips the jar successfully, but prints out bogus warnings about zip structure and returns an error code. I figured the easiest way to work around that was to in addition to extracting the log to deoptimize the jar too.

Comment 5

[(dormant account)]

Attachment: l10n repacking support for jar ordering

s/perl/sed/ + misc absolute path fix

Comment 6

[(dormant account)]

Attachment: l10n repacking support for jar ordering

Now with Pike's "is not None" suggestion

Comment 7

[(dormant account)]

Attachment: l10n repacking support for jar ordering

Missed the is not None changes in previous patch.
I wish there was some way to abort uploading a patch if I got outstanding changes that weren't qrefed.

Comment 8

[Axel Hecht [:Pike]]

Comment on attachment 468729 [details] [diff] [review]
l10n repacking support for jar ordering

I don't think that I should offer more than a feedback+ on patches of this scope.

A few more hints on what to watch out for:

There are a bunch of files created in the working dir, if I read this right. The l10n repacks do src-dir builds, still, so watch out for that?

Also, I haven't run across prefixing a command with -, what does that do to the sed line?

Comment 9

[(dormant account)]

(In reply to comment #8)
> 
> There are a bunch of files created in the working dir, if I read this right.
> The l10n repacks do src-dir builds, still, so watch out for that?

The only files created should be jar log files, those seem ok in my testing.
> 
> Also, I haven't run across prefixing a command with -, what does that do to the
> sed line?

Makes it ignore errors from sed, so if there are no log files, repacking still works.

Comment 10

[(dormant account)]

Comment on attachment 468729 [details] [diff] [review]
l10n repacking support for jar ordering

Going to wait for a way to specify the path/to/omnijar as a absolute path. Too fragile otherwise.

Comment 11

[Mike Beltzner [:beltzner, not reading bugmail]]

Is this needed before we do l10n repacks for beta5? If so, can we get a quick review, khuey?

Comment 12

[Benjamin Smedberg]

No, it's not. The l10n builds might be slower than the en-US builds, but it shouldn't affect functionality.

Comment 13

[(dormant account)]

Attachment: l10n repacking support for jar ordering

Comment 14

[(dormant account)]

Comment on attachment 470592 [details] [diff] [review]
l10n repacking support for jar ordering

Ted, perhaps you can review this sooner than Kyle who might be busy with school.

Comment 15

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Yeah, I'm a little swamped with school and my own blockers right now, but if ted can't get to it I'll try to handle it.

Comment 16

[(not currently active) Ted Mielczarek]

Comment on attachment 470592 [details] [diff] [review]
l10n repacking support for jar ordering

># HG changeset patch
># User Taras Glek <[e-mail]>
># Parent 64f6f59520c7b67b53260449347fe0b308edd25d
>Bug 589368 - Locale repacking support for jar reordering
>
>diff --git a/config/optimizejars.py b/config/optimizejars.py
>--- a/config/optimizejars.py
>+++ b/config/optimizejars.py
>@@ -183,14 +183,24 @@ class BinaryBlob:
>         assert_true(out_data == data, "Serialization fail %d !=%d"% (len(out_data), len(data)))
>         return retstruct
> 
>-def optimizejar(log, jar, outjar):
>-    entries = open(log).read().rstrip().split("\n")
>+def optimizejar(jar, outjar, inlog = None):
>+    if inlog is not None:
>+        inlog = open(inlog).read().rstrip()
>+        # in the case of an empty log still move the index forward
>+        if len(inlog) == 0:
>+            inlog = []
>+        else:
>+            inlog = inlog.split("\n")

inlog = open(inlog).readlines()

>+def optimize(JAR_LOG_DIR, IN_JAR_DIR, OUT_JAR_DIR):
>+    if not os.path.exists(JAR_LOG_DIR):
>+        print("No jar logs found in %s. No jars to optimize." % JAR_LOG_DIR)
>+        exit(0)
> 
>-if not os.path.exists(JAR_LOG_DIR):
>-    print "No jar logs found. No jars to optimize."
>-    exit(0)
>+    ls = os.listdir(JAR_LOG_DIR)
>+    for logfile in ls:
>+        if logfile[-8:] != ".jar.log":

if not logfile.endswith(".jar.log"):

>+def deoptimize(JAR_LOG_DIR, IN_JAR_DIR, OUT_JAR_DIR):
>+    if not os.path.exists(JAR_LOG_DIR):
>+        os.makedirs(JAR_LOG_DIR)
>+
>+    ls = os.listdir(IN_JAR_DIR)
>+    for jarfile in ls:
>+        if jarfile[-4:] != ".jar":

if not jarfile.endswith(".jar"):

>+MODE = sys.argv[1]
>+JAR_LOG_DIR = sys.argv[2]
>+IN_JAR_DIR = sys.argv[3]
>+OUT_JAR_DIR = sys.argv[4]
>+if MODE == "--optimize":
>+    optimize(JAR_LOG_DIR, IN_JAR_DIR, OUT_JAR_DIR)
>+elif MODE == "--deoptimize":
>+    deoptimize(JAR_LOG_DIR, IN_JAR_DIR, OUT_JAR_DIR)
>+else:
>+    print("Unknown mode %s" % MODE)
>+    exit(1)

Could you fix this slightly to do:

if __name__ == '__main__':
  main()

and stick all the current top-level stuff in:
def main():
  ...

Looks fine otherwise. r=me with those changes.

Comment 17

[Axel Hecht [:Pike]]

inlog = inlog.split("\n")
and
inlog = open(inlog).readlines()
are not equiv, sadly, you'll need to strip the trailing newlines if you use readlines().

Comment 18

[(not currently active) Ted Mielczarek]

True. [line.rstrip() for line in open(inlog).readlines()] would work.

Comment 19

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 470592 [details] [diff] [review]
l10n repacking support for jar ordering

Clearing review since ted has this covered.

Comment 20

[(dormant account)]

Attachment: l10n repacking support for jar ordering

Addressed review comments(except for .readlines(), I like the existing code better).

Comment 21

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 473234 [details] [diff] [review]
l10n repacking support for jar ordering

a=beltzner

Comment 22

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/4cffee2c859b