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

VERIFIED FIXED in Firefox 24

Status

()

defect
VERIFIED FIXED
6 years ago
3 years ago

People

(Reporter: nalexander, Assigned: nalexander)

Tracking

Trunk
Firefox 25
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox24 verified, firefox25 verified)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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 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.
Attachment #767578 - Flags: feedback?(mh+mozilla)
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.
Blocks: 873569
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/*.
Attachment #767578 - Flags: feedback?(mh+mozilla) → feedback-
(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 ?
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
Attachment #768435 - Flags: review?(mh+mozilla)
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))
Attachment #768435 - Flags: review?(mh+mozilla) → review-
(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.
(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.
Review comments addressed.
Attachment #767578 - Attachment is obsolete: true
Attachment #768435 - Attachment is obsolete: true
Attachment #769251 - Flags: review?(mh+mozilla)
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."
Attachment #769251 - Flags: review?(mh+mozilla) → review+
https://hg.mozilla.org/services/services-central/rev/14694cb699d2
Assignee: nobody → nalexander
Status: NEW → ASSIGNED
Whiteboard: [fixed in services]
Target Milestone: --- → Firefox 25
https://hg.mozilla.org/mozilla-central/rev/14694cb699d2
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed in services]
N2 is failing due to Bug 889845 and Bug 889837.
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.
Attachment #769251 - Flags: approval-mozilla-aurora?
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.
Attachment #769251 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Have not seen any issues. I have verified through unzipping the APK that omni.ja has moved.
You need to log in before you can comment on or make changes to this bug.