Closed Bug 589368 Opened 9 years ago Closed 9 years ago

Locale repacking support for jar reordering

Categories

(Firefox Build System :: General, defect)

x86
Linux
defect
Not set

Tracking

(blocking2.0 betaN+)

RESOLVED FIXED
mozilla2.0b7
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: taras.mozilla, Assigned: taras.mozilla)

References

Details

Attachments

(1 file, 5 obsolete files)

Attached patch magic patch (obsolete) — Splinter Review
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?).
Attachment #467984 - Flags: review?(khuey)
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.
(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.
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?
(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.
s/perl/sed/ + misc absolute path fix
Assignee: nobody → tglek
Attachment #467984 - Attachment is obsolete: true
Attachment #468490 - Flags: review?
Attachment #467984 - Flags: review?(khuey)
Now with Pike's "is not None" suggestion
Attachment #468490 - Attachment is obsolete: true
Attachment #468493 - Flags: review?(l10n)
Attachment #468490 - Flags: review?
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.
Attachment #468493 - Attachment is obsolete: true
Attachment #468729 - Flags: review?(l10n)
Attachment #468493 - Flags: review?(l10n)
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?
Attachment #468729 - Flags: review?(l10n) → feedback+
(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.
Attachment #468729 - Flags: review?(khuey)
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.
Attachment #468729 - Flags: review?(khuey) → review?
Is this needed before we do l10n repacks for beta5? If so, can we get a quick review, khuey?
No, it's not. The l10n builds might be slower than the en-US builds, but it shouldn't affect functionality.
blocking2.0: beta5+ → betaN+
Attachment #468729 - Attachment is obsolete: true
Attachment #470592 - Flags: review?(khuey)
Attachment #468729 - Flags: review?
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.
Attachment #470592 - Flags: review?(ted.mielczarek)
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 on attachment 470592 [details] [diff] [review]
l10n repacking support for jar ordering

># HG changeset patch
># User Taras Glek <tglek@mozilla.com>
># 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.
Attachment #470592 - Flags: review?(ted.mielczarek) → review+
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().
True. [line.rstrip() for line in open(inlog).readlines()] would work.
Comment on attachment 470592 [details] [diff] [review]
l10n repacking support for jar ordering

Clearing review since ted has this covered.
Attachment #470592 - Flags: review?(khuey)
Addressed review comments(except for .readlines(), I like the existing code better).
Attachment #470592 - Attachment is obsolete: true
Attachment #473234 - Flags: review+
Attachment #473234 - Flags: approval2.0?
Comment on attachment 473234 [details] [diff] [review]
l10n repacking support for jar ordering

a=beltzner
Attachment #473234 - Flags: approval2.0? → approval2.0+
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/4cffee2c859b
Status: NEW → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b6
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.