772841 - optimizejars.py doesn't do anything

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Mike Hommey [:glandium]]

optimizejars.py was broken by the change of rule used to prepare firefox for the profile run from package to stage-package in bug 481815. Stage-package doesn't do omnijar packing, so when the profile runs, there's no omnijar to log accesses to.

Comment 1

[Brian R. Bondy [:bbondy]]

Which change specifically caused the regression?

Comment 2

[Mike Hommey [:glandium]]

(In reply to Brian R. Bondy [:bbondy] from comment #1)
> Which change specifically caused the regression?

This:
-       $(MAKE) -C $(PGO_OBJDIR) package
+       $(MAKE) -C $(PGO_OBJDIR) stage-package

Comment 3

[Mike Hommey [:glandium]]

Attachment: Pack omnijar when doing stage-package

Comment 4

[Brian R. Bondy [:bbondy]]

I don't think 481815 made any such change.

Comment 5

[Mike Hommey [:glandium]]

(In reply to Brian R. Bondy [:bbondy] from comment #4)
> I don't think 481815 made any such change.

https://hg.mozilla.org/mozilla-central/rev/6b60ebe2cae4
bug 481815 comment 458

Comment 6

[Brian R. Bondy [:bbondy]]

ah ok, I thought you meant it was part of the core maintenance service code.  That patch really should have landed in its own bug number.

Comment 7

[Mike Hommey [:glandium]]

Comment on attachment 641051 [details] [diff] [review]
Pack omnijar when doing stage-package

Doesn't work :(
https://tbpl.mozilla.org/php/getParsedLog.php?id=13429714&tree=Try

Comment 8

[Mike Hommey [:glandium]]

Attachment: Pack omnijar when doing stage-package

This should work better, on top of avoiding zipping the omnijar twice like we currently do.

I'd push to try if try was working:
pushing to ssh://hg.mozilla.org/try
remote: Traceback (most recent call last):
remote:   File "/usr/local/bin/pash.py", line 4, in <module>
remote:     import hg_helper, ldap_helper
remote:   File "/usr/local/bin/hg_helper.py", line 4, in <module>
remote:     from ldap_helper import ldap_connect, get_ldap_attribute
remote:   File "/usr/local/bin/ldap_helper.py", line 50
remote:     if access_time < yesterday:
remote:                               ^
remote: IndentationError: unindent does not match any outer indentation level

Comment 9

[Mike Hommey [:glandium]]

Attachment: Pack omnijar when doing stage-package

Previous one was breaking b2g and mobile.

Comment 10

[Mike Hommey [:glandium]]

Comment on attachment 641143 [details] [diff] [review]
Pack omnijar when doing stage-package

Dammit, it breaks osx.

Comment 11

[Mike Hommey [:glandium]]

Attachment: Revert client.mk part of changeset 6b60ebe2cae4

It's getting too late in the 14 release process to try to do the right thing. It takes too long to get it not to break mac builds.

Comment 12

[Mike Hommey [:glandium]]

Attachment: Use make package instead of make stage-package before profile run, but avoid package signing.

Comment 13

[(not currently active) Ted Mielczarek]

Comment on attachment 641472 [details] [diff] [review]
Use make package instead of make stage-package before profile run, but avoid package signing.

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

Would be nice if we could avoid the overhead of actually generating the final package file as well, but not critical.

Comment 14

[Mike Hommey [:glandium]]

(In reply to Ted Mielczarek [:ted] from comment #13)
> Would be nice if we could avoid the overhead of actually generating the
> final package file as well, but not critical.

That's what i was trying to do, but ran out of time.

Comment 15

[Mike Hommey [:glandium]]

http://hg.mozilla.org/mozilla-central/rev/87ff6a0cc45e

Comment 16

[Lukas Blakk [:lsblakk] use ?needinfo]

please nominate for uplift with risk assessment so we can evaluate whether we can still take this on beta.

Comment 17

[Mike Hommey [:glandium]]

Comment on attachment 641472 [details] [diff] [review]
Use make package instead of make stage-package before profile run, but avoid package signing.

[Approval Request Comment]
Regression caused by a part of bug 481815
User impact if declined: Keeping cold startup performance regression we got in either FF12 or FF13 (not entirely clear which one)
Testing completed (on m-c, etc.): Tested on try with pgo ; try logs checked by catlee and myself for correctness. 
Risk to taking this patch (and alternatives if risky): Quite low. It's not introducing new code/functionality, it's restoring the use of code that was mistakenly disabled by a small change from bug 481815.
String or UUID changes made by this patch: None

Comment 18

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 641472 [details] [diff] [review]
Use make package instead of make stage-package before profile run, but avoid package signing.

restoring use of broken code, go ahead and land to branches.

Comment 19

[Mike Hommey [:glandium]]

http://hg.mozilla.org/releases/mozilla-aurora/rev/156d908eccd4
http://hg.mozilla.org/releases/mozilla-beta/rev/20b57236b6ec