726656 - optimizejars.py broken by the rename to omni.ja

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

We don't optimize jars anymore, and builds ignore the problem, letting it regress.

Comment 1

[(dormant account)]

(In reply to Marco Bonardo [:mak] from comment #0)
> We don't optimize jars anymore, and builds ignore the problem, letting it
> regress.

Do we know what caused the regression?

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

well, to begin with, at least a couple code points do jarfile.endswith(".jar")
not sure if there's other stuff involved at build config level

Comment 3

[Mozilla RelEng Bot]

Try run for db2b291b5b8f is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=db2b291b5b8f
Results (out of 1 total builds):
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-db2b291b5b8f

Comment 4

[Mozilla RelEng Bot]

Try run for 8fcc594671a1 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=8fcc594671a1
Results (out of 1 total builds):
    success: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-8fcc594671a1

Comment 5

[(dormant account)]

I'm not working on this atm.

Comment 6

[Nathan Froyd [:froydnj]]

I have some cycles to poke at this.

Comment 7

[Nathan Froyd [:froydnj]]

Attachment: patch

Here's the easy patch which makes optimizejars.py check for .ja extensions.  It would, of course, be nice to make this check more robust (check for zip file fingerprints?), but I do not know how fragile that would be.

Debugging output verified that we now look at omni.ja.

Comment 8

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

Comment on attachment 597794 [details] [diff] [review]
patch

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

r=me assuming it works.

Comment 9

[Nathan Froyd [:froydnj]]

How does one tell that it works, given that it took us a while to notice it was broken in the first place?

Comment 10

[Nathan Froyd [:froydnj]]

Actually, going to assume that khuey didn't see my comment about debugging output.

Comment 11

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

(In reply to Nathan Froyd (:froydnj) from comment #10)
> Actually, going to assume that khuey didn't see my comment about debugging
> output.

That would be a correct assumption.

Comment 12

[Mozilla RelEng Bot]

Autoland Patchset:
	Patches: 597794
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=49fd256eb14f
Try run started, revision 49fd256eb14f. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=49fd256eb14f

Comment 13

[(dormant account)]

(In reply to Nathan Froyd (:froydnj) from comment #9)
> How does one tell that it works, given that it took us a while to notice it
> was broken in the first place?

you unzip onmi.ja and check that prefs.js is near the top(and unzip bitches about file being corrupt). Please don't land this until that's verified.

Comment 14

[Nathan Froyd [:froydnj]]

(In reply to Taras Glek (:taras) from comment #13)
> (In reply to Nathan Froyd (:froydnj) from comment #9)
> > How does one tell that it works, given that it took us a while to notice it
> > was broken in the first place?
> 
> you unzip onmi.ja and check that prefs.js is near the top(and unzip bitches
> about file being corrupt). Please don't land this until that's verified.

With unzip -l, I see:

Archive:  dist/firefox/omni.ja
warning [dist/firefox/omni.ja]:  6747593 extra bytes at beginning or within zipfile
  (attempting to process anyway)
error [dist/firefox/omni.ja]:  reported length of central directory is
  -6747593 bytes too long (Atari STZip zipfile?  J.H.Holm ZIPSPLIT 1.1
  zipfile?).  Compensating...

which I assume is sufficient proof of corruption.

prefs.js, however, is nowhere near the top of the file.

Comment 15

[Nathan Froyd [:froydnj]]

Attachment: unzip -l omni.ja output

See for yourself what things look like.

Comment 16

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

That doesn't look like it's been reordered at all.

Comment 17

[Mike Hommey [:glandium]]

My guess is that he doesn't have an ordering list. IIRC, it's generated during the PGO profile run.

Comment 18

[Nathan Froyd [:froydnj]]

Attachment: unzip -l omni.ja output, optimized

Ah, I am enlightened.  So, after:

- starting dist/firefox/firefox with MOZ_JAR_LOG_DIR set;
- arranging for omni.ja.log to be deposited in jarlog/en-US/;
- running make package;

I get the attached output, which looks substantially different.

Comment 19

[Alex Keybl [:akeybl]]

This does not affect the built product, so no need to track for a specific release. Once resolved, interested parties can grab the latest optimizejars.py.

Comment 20

[Alex Keybl [:akeybl]]

That evaluation is based upon https://bugzilla.mozilla.org/show_bug.cgi?id=726656#c0, which states that "We don't optimize jars anymore".

Comment 21

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

(In reply to Alex Keybl [:akeybl] from comment #20)
> That evaluation is based upon
> https://bugzilla.mozilla.org/show_bug.cgi?id=726656#c0, which states that
> "We don't optimize jars anymore".

That's the bug that this patch fixes.

Comment 22

[Alex Keybl [:akeybl]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #21)
> (In reply to Alex Keybl [:akeybl] from comment #20)
> > That evaluation is based upon
> > https://bugzilla.mozilla.org/show_bug.cgi?id=726656#c0, which states that
> > "We don't optimize jars anymore".
> 
> That's the bug that this patch fixes.

Yep - RyanVM clarified in #developers as well. I took "We don't optimize jars anymore" to mean this tool isn't necessary for builds.

Comment 23

[Mozilla RelEng Bot]

Try run for 49fd256eb14f is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=49fd256eb14f
Results (out of 210 total builds):
    exception: 1
    success: 178
    warnings: 18
    failure: 13
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-49fd256eb14f

Comment 24

[Justin Wood (:Callek)]

(In reply to Mozilla RelEng Bot from comment #23)
> Results (out of 210 total builds):
>     exception: 1
^^^^^^^This is Jetpack (and hidden)

>     warnings: 18
^^^^^^^11 on OSX 10.7 that are hidden
^^^^^^^3 Android Native Opt that are hidden
^^^^^^^2 Intermittent
^^^^^^^1 Android Retriggered (Since log doesn't tell me much)
^^^^^^^1 Win Debug M(oth) that was leaking and I can't find a bug on, retriggered.

>     failure: 13
^^^^^^^These are all jetpack (and hidden)

Comment 25

[Justin Wood (:Callek)]

(In reply to Alex Keybl [:akeybl] from comment #22)
> (In reply to Kyle Huey [:khuey] ([e-mail]) from comment #21)
> > (In reply to Alex Keybl [:akeybl] from comment #20)
> > > That evaluation is based upon
> > > https://bugzilla.mozilla.org/show_bug.cgi?id=726656#c0, which states that
> > > "We don't optimize jars anymore".
> > 
> > That's the bug that this patch fixes.
> 
> Yep - RyanVM clarified in #developers as well. I took "We don't optimize
> jars anymore" to mean this tool isn't necessary for builds.

I'm guessing the tracking-esr10:13+ was a mistake and it should be 11+ given your choice to track this for 11 as well?

Comment 26

[Ryan VanderMeulen [:RyanVM]]

There won't be an ESR release based on 11.

Comment 27

[Alex Keybl [:akeybl]]

(In reply to Justin Wood (:Callek) from comment #25)
> I'm guessing the tracking-esr10:13+ was a mistake and it should be 11+ given
> your choice to track this for 11 as well?

My expectation is that this will land on Mozilla 13 first. We'll change the tracking flag if approved for m-a or m-b.

(In reply to Ryan VanderMeulen from comment #26)
> There won't be an ESR release based on 11.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for info on how we track ESR changes.

Comment 28

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

I have filed bug 728176 to investigate ways to fail the build when omni.ja optimization fails. So we don't mix up the two problems. Any idea for that should go there.

Comment 29

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/mozilla-central/rev/f88a05e00f47

Comment 30

[Alex Keybl [:akeybl]]

Please nominate for approval Aurora/Beta approval if you believe this fix to be low risk (sounds like it).

Comment 31

[Nathan Froyd [:froydnj]]

Comment on attachment 597794 [details] [diff] [review]
patch

[Approval Request Comment]
Regression caused by (bug #): 701875
User impact if declined: Slower startup.
Testing completed (on m-c, etc.): On m-c.
Risk to taking this patch (and alternatives if risky): Epsilon.
String changes made by this patch: None

Comment 32

[Alex Keybl [:akeybl]]

Comment on attachment 597794 [details] [diff] [review]
patch

[Triage Comment]
Approved for Aurora 12 and Beta 11.

Comment 33

[Nathan Froyd [:froydnj]]

Comment on attachment 597794 [details] [diff] [review]
patch

Need this checked in to aurora and beta; if somebody would be so kind as to tack a=akeybl on the commit line, that would be appreciated.  Thanks!

Comment 34

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

I can do that.

Comment 35

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/fd7744b11e51
https://hg.mozilla.org/releases/mozilla-beta/rev/db6414deacea

Comment 36

[Alex Keybl [:akeybl]]

We didn't change the ESR tracking flag from 13+ to 11+ when this landed a month ago. Feel free to land an appropriate patch on the ESR branch whenever possible.

Comment 37

[Nathan Froyd [:froydnj]]

http://hg.mozilla.org/releases/mozilla-esr10/rev/08ac106b5072