887121 - Make packager install and szip .so libraries in assets/ directly

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Nick Alexander :nalexander [he/him]]

This is a follow-up to Bug 873569, specifically https://bugzilla.mozilla.org/show_bug.cgi?id=873569#c27. 

That ticket makes each |make package| copy .so files into $(STAGE), szip them, and then move them into $(STAGE)/assets.  szip is very slow, so each iteration takes a long time.  We'd like to have |make package| copy the .so files into $(STAGE)/assets directly, so the szip is in-place and not repeated due to newer timestamps.

There's a good deal of work in progress on Bug 873569; this ticket is good Bugzilla hygiene.

Comment 1

[Nick Alexander :nalexander [he/him]]

Attachment: Make packager install and szip .so libraries in assets/ directly. r=glandium

Hi glandium, I broke this out into a separate ticket; your last review
is at https://bugzilla.mozilla.org/show_bug.cgi?id=873569#c51.

Thanks for pushing me towards more idiomatic Python; I think your
guidance leaves it much improved.  I addressed the multiple warnings
by throwing ValueError in the helper functions and catching once.

There's a try build with this and the companion Bug 887115 working
away at https://tbpl.mozilla.org/?tree=Try&rev=d172e53562f5.

Comment 2

[Mike Hommey [:glandium]]

Comment on attachment 768429 [details] [diff] [review]
Make packager install and szip .so libraries in assets/ directly. r=glandium

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

::: python/mozbuild/mozpack/packager/__init__.py
@@ +107,5 @@
> +        '''
> +        try:
> +            name, options = Component._split_component_and_options(string)
> +        except ValueError:
> +            errors.fatal('Malformed manifest: invalid name or options found')

You should use the string you gave in the exception.

@@ +150,5 @@
>          str = str.strip()
>          if not str or str.startswith(';'):
>              return
>          if str.startswith('[') and str.endswith(']'):
> +            if str == '[]' or re.search(r'[\[\]]', str[1:-1]):

You're explicitly excluding [ and ] in Component, so you can lift that re.search.

Comment 3

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dfc8d955b043

Comment 4

[Phil Ringnalda (:philor)]

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

Comment 5

[Nick Alexander :nalexander [he/him]]

Comment on attachment 768429 [details] [diff] [review]
Make packager install and szip .so libraries in assets/ directly. r=glandium

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Regression from Bug 873569, tracked by Bug 895442.

User impact if declined: continued broken Android nightly repacks on Aurora and above.

Testing completed (on m-c, etc.): This has been on m-c for weeks; I meant to request uplift after it demonstrated its value and forgot.

Risk to taking this patch (and alternatives if risky): This messes with Android packaging, so there's a slim possibility of discovering additional packaging/l10n repacking differences between Aurora and Nightly.  We could revert Bug 873569 in Aurora and above, but that seems riskier, since new uplifts would have landed on top of 873569.

String or IDL/UUID changes made by this patch: none.

Comment 6

[Nick Alexander :nalexander [he/him]]

(In reply to Nick Alexander :nalexander from comment #5)
> Comment on attachment 768429 [details] [diff] [review]
> Make packager install and szip .so libraries in assets/ directly. r=glandium
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): Regression from Bug 873569,
> tracked by Bug 895442.
> 
> User impact if declined: continued broken Android nightly repacks on Aurora
> and above.
> 
> Testing completed (on m-c, etc.): This has been on m-c for weeks; I meant to
> request uplift after it demonstrated its value and forgot.
> 
> Risk to taking this patch (and alternatives if risky): This messes with
> Android packaging, so there's a slim possibility of discovering additional
> packaging/l10n repacking differences between Aurora and Nightly.  We could
> revert Bug 873569 in Aurora and above, but that seems riskier, since new
> uplifts would have landed on top of 873569.
> 
> String or IDL/UUID changes made by this patch: none.

I forgot to say: the real value of this is uplifting Bug 887115; I wouldn't like to uplift 887115 without this in place.

Comment 7

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 768429 [details] [diff] [review]
Make packager install and szip .so libraries in assets/ directly. r=glandium

approving to go with bug 887115

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0d566d5133be