887115 - Moving omni.ja out of root directory breaks Android Nightly l10n repacks

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Nick Alexander :nalexander [he/him]]

I finally figured out the cause of the l10n bustage occasioned by Bug 873569.  The issue is that python/mozbuild/mozpack/packager/unpack.py unpacks assets/omni.ja into assets/, which is not where the l10n-central sub-directories expect it.

The relevant log entries look like https://tbpl.mozilla.org/php/getParsedLog.php?id=24563166&tree=Mozilla-Central&full=1#error1

Comment 1

[Nick Alexander :nalexander [he/him]]

Attachment: Make unpack treat omnijar contents as being in root. r=glandium

Comment 2

[Nick Alexander :nalexander [he/him]]

Comment on attachment 767578 [details] [diff] [review]
Make unpack treat omnijar contents as being in root. r=glandium

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

Hi Mike, thanks for all your help with the original ticket and the (never-ending, it seems) fallout.  The attached patch fixes the problem for me locally, but I'd like your feedback.  Is this safe?  Could we have two omnijars?  (I think not, but the code seems contradictory -- only one is handled, but any others are ignored.)  I wonder if explicitly checking for an omnijar named OMNIJAR_NAME is better.

Comment 3

[Nick Alexander :nalexander [he/him]]

I tested this as follows:

* regular Android build into objdir-droid
* l10n build into objdir-l10n
* hg clone l10n-central/ar

make -C objdir-l10n/config
cp ./objdir-droid/dist/fennec-24.0a1.en-US.android-arm.apk ./objdir-l10n/dist
make -C objdir-l10n/mobile/android/locales unpack
compare-locales -m objdir-l10n/merged mobile/android/locales/l10n.ini ../l10n-central ar
LOCALE_MERGEDIR=objdir-l10n/merged make -C objdir-l10n/mobile/android/locales installers-ar

aki, I'd like to help prepare documentation so the next poor sucker has an easier time.

Comment 4

[Mike Hommey [:glandium]]

Comment on attachment 767578 [details] [diff] [review]
Make unpack treat omnijar contents as being in root. r=glandium

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

::: python/mozbuild/mozpack/packager/unpack.py
@@ +60,5 @@
>                  jar = self._open_jar(p, f)
>                  if 'chrome.manifest' in jar:
>                      self.kind = 'omni'
> +                    self.omnijar = p
> +                    self._fill_with_omnijar('', jar)

I'm pretty sure this will break unpacking desktop Firefox.

The best thing to do would be to have everything in assets already in dist/bin, but that's likely to be hard, so the simplest is probably to just add a special rule that moves the contents of assets/ before running unpack.py, in INNER_UNMAKE_PACKAGE. Which, as it happens, is already the case, except only the libraries are moved. Just move assets/*.

Comment 5

[Aki Sasaki (not active)]

(In reply to Nick Alexander :nalexander from comment #3)
> aki, I'd like to help prepare documentation so the next poor sucker has an
> easier time.

Awesome.
Where do you think, somewhere near https://wiki.mozilla.org/Mobile/Fennec/Android#Multilocale_builds or MDN ?

Comment 6

[Nick Alexander :nalexander [he/him]]

Attachment: Make Android packager expect omnijar in root directory. r=glandium

This is intended to follow Bug 887121.  You suggested moving assets/*
into /, but I want the packager to expect the .so files in assets/ so
we don't re-szip every |make package|, so I only fiddle with omni.ja.

There's a try build cooking at
https://tbpl.mozilla.org/?tree=Try&rev=d172e53562f5

Comment 7

[Mike Hommey [:glandium]]

Comment on attachment 768435 [details] [diff] [review]
Make Android packager expect omnijar in root directory. r=glandium

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

::: toolkit/mozapps/installer/packager.mk
@@ +374,5 @@
>      rm $(_ABS_DIST)/gecko.ap_ && \
>      $(ZIP) -0 $(_ABS_DIST)/gecko.ap_ $(ASSET_SO_LIBRARIES) && \
>      $(ZIP) -r9D $(_ABS_DIST)/gecko.ap_ $(DIST_FILES) -x $(NON_DIST_FILES) $(SZIP_LIBRARIES) && \
> +    mkdir -p $(dir $(OMNIJAR_NAME)) && \
> +    mv $(notdir $(OMNIJAR_NAME)) $(OMNIJAR_NAME) && \

Would be safer to ensure $(dir $(OMNIJAR_NAME)) is not empty. Or tolerate that it is by handling that case properly.

::: toolkit/mozapps/installer/packager.py
@@ +277,2 @@
>          formatter = OmniJarFormatter(copier,
> +                                     omnijar,

I think it would be best to just override the OMNIJAR_NAME variable when invoking packager.py. Adding the following in packager.mk at the beginning of the packager.py command should work:

OMNIJAR_NAME=$(notdir $(OMNIJAR_NAME))

Comment 8

[Mike Hommey [:glandium]]

(In reply to Nick Alexander :nalexander from comment #6)
> Created attachment 768435 [details] [diff] [review]
> Make Android packager expect omnijar in root directory. r=glandium
> 
> This is intended to follow Bug 887121.  You suggested moving assets/*
> into /, but I want the packager to expect the .so files in assets/ so
> we don't re-szip every |make package|, so I only fiddle with omni.ja.

szip ignores files that are already in the szip format. And moving the files won't change that fact. I'd still prefer you to move everything.

Comment 9

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #8)
> szip ignores files that are already in the szip format. And moving the files
> won't change that fact. I'd still prefer you to move everything.

Ignore this comment, I hadn't looked at bug 887121.

Comment 10

[Nick Alexander :nalexander [he/him]]

Attachment: Make Android packager expect omnijar in root directory. r=glandium

Review comments addressed.

Comment 11

[Nick Alexander :nalexander [he/him]]

http://tbpl.mozilla.org/?tree=Try&rev=adc16e5bd868

Comment 12

[Mike Hommey [:glandium]]

Comment on attachment 769251 [details] [diff] [review]
Make Android packager expect omnijar in root directory. r=glandium

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

::: toolkit/mozapps/installer/packager.mk
@@ +361,5 @@
> +# / in order to be consistent with the layout expected by language
> +# repacks. Therefore, we move it to the correct path here, in
> +# INNER_MAKE_PACKAGE.  See comment about OMNIJAR_NAME in configure.in.
> +
> +OMNIJAR_WITHOUTDIR := $(notdir $(OMNIJAR_NAME))

might as well do OMNIJAR_NAME := $(notdir $(OMNIJAR_NAME))
(as long as you define OMNIJAR_DIR above that)

@@ +612,3 @@
>  stage-package: $(MOZ_PKG_MANIFEST)
>  	@rm -rf $(DIST)/$(PKG_PATH)$(PKG_BASENAME).tar $(DIST)/$(PKG_PATH)$(PKG_BASENAME).dmg $@ $(EXCLUDE_LIST)
> +	OMNIJAR_NAME=$(notdir $(OMNIJAR_NAME)) \

with the suggested change, that becomes OMNIJAR_NAME=$(OMNIJAR_NAME) in which case the comment above gets confusing, so it can be replaced by something like "Override the value of OMNIJAR_NAME from config.status with the value set in this file."

Comment 13

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/services/services-central/rev/14694cb699d2

Comment 14

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/mozilla-central/rev/14694cb699d2

Comment 15

[Nick Alexander :nalexander [he/him]]

Looking pretty healthy:

https://tbpl.mozilla.org/php/getParsedLog.php?id=24880990&tree=Mozilla-Central
https://tbpl.mozilla.org/php/getParsedLog.php?id=24880776&tree=Mozilla-Central
https://tbpl.mozilla.org/php/getParsedLog.php?id=24880978&tree=Mozilla-Central
https://tbpl.mozilla.org/php/getParsedLog.php?id=24880983&tree=Mozilla-Central

Comment 16

[Nick Alexander :nalexander [he/him]]

N2 is failing due to Bug 889845 and Bug 889837.

Comment 17

[Nick Alexander :nalexander [he/him]]

Comment on attachment 769251 [details] [diff] [review]
Make Android packager expect omnijar in root directory. r=glandium

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Regression from Bug 873569, tracked by Bug 895442.  This ticket is the real fix for 895442, but it really needs to be uplifted on top of Bug 887121, since it builds on it.

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 18

[bhavana bajaj [:bajaj]]

Comment on attachment 769251 [details] [diff] [review]
Make Android packager expect omnijar in root directory. r=glandium

Approving due to broken repacks.Please add qawanted here if there is any QA needed here to make sure L10n Aurora Nightly builds are as expected.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6afaaba02827

Comment 20

[Aaron Train [:aaronmt]]

Have not seen any issues. I have verified through unzipping the APK that omni.ja has moved.