Closed Bug 873569 Opened 11 years ago Closed 11 years ago

Move Gecko libraries and omni.ja into assets/ directory

Categories

(Firefox for Android Graveyard :: General, defect)

Product:
Firefox for Android Graveyard
Component:
General
Platform:
All
Android
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Firefox 24

People

(Reporter: nalexander, Assigned: nalexander)

References

Details

Attachments

(4 files, 4 obsolete files)

Part 0: Add a sanity test for JNI.jsm. r=wesj
3.83 KB, patch
wesj
: review+
Details | Diff | Splinter Review
Move omni.ja into assets directory.
2.78 KB, patch
glandium
: review+
Details | Diff | Splinter Review
Move .so libraries into assets/.
4.66 KB, patch
glandium
: review+
Details | Diff | Splinter Review
Follow-up: Teach packager to put Android libraries in assets/. r=glandium
21.49 KB, patch
glandium
: review-
Details | Diff | Splinter Review 
I think we should move the Gecko libraries and omni.ja out of the APK root and into assets/.  It's quite easy and normalizes Fennec's layout with the Android convention, which is "no package files in the APK root".

If we want to ship an embeddable GeckoView, third party developers will expect a JAR, some libraries to put in lib/, some resources, and possibly some assets.  We can leverage the Android APIs for finding the correct asset directory, removing a potential headache.  Embedders will certainly not be happy jumping through the hoops needed to insert files into the APK root.

This also makes it easier to build Fennec in IDEs.  For example, Eclipse and maven-android-plugin support package files in assets/ only.  Presumably, the newly announced Android Studio is the same, but I haven't checked.

I see no reason to treat omni.ja differently.
> I see no reason to treat omni.ja differently.

I mean that I see no reason to keep omni.ja in the APK root: we should move it into assets/.
blassey: I've looked at the mozglue/android/APKOpen* code, and this looks straightforward.  Any reason not to do this?

Note to implementer: we need to handle plugin files like libomx*.so as well.
Flags: needinfo?(blassey.bugs)
(In reply to Nick Alexander :nalexander from comment #2)
> Note to implementer: we need to handle plugin files like libomx*.so as well.

You actually don't. As long as the files are in the same location as libxul.so, they will be found without modifying the existing code.
(In reply to Mike Hommey [:glandium] from comment #4)
> Moving omni.ja requires changing this:
> http://hg.mozilla.org/mozilla-central/file/37b8fa6c1c92/xpcom/build/Omnijar.
> cpp#l82

I found it easier to modify:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoAppShell.java#273
(In reply to Nick Alexander :nalexander from comment #5)
> I found it easier to modify:
> 
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/
> GeckoAppShell.java#273

That doesn't work, it gives the path of the apk, not the path of the omni.ja within the apk. Ideally, it should, and I'd encourage changing Omnijar.cpp such that it does.
(In reply to Mike Hommey [:glandium] from comment #6)
> (In reply to Nick Alexander :nalexander from comment #5)
> > I found it easier to modify:
> > 
> > http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/
> > GeckoAppShell.java#273
> 
> That doesn't work, it gives the path of the apk, not the path of the omni.ja
> within the apk. Ideally, it should, and I'd encourage changing Omnijar.cpp
> such that it does.

You're right -- I used this to load omni.ja from /data/local.  Glad you support making Omnijar.cpp a little more flexible.
(In reply to Nick Alexander :nalexander from comment #2)
> blassey: I've looked at the mozglue/android/APKOpen* code, and this looks
> straightforward.  Any reason not to do this?
> 
> Note to implementer: we need to handle plugin files like libomx*.so as well.

nope, no reason
Flags: needinfo?(blassey.bugs)
Comment on attachment 756133 [details] [diff] [review]
Part 1: Move Gecko .so libraries into assets/ directory of Android APK. r=glandium

Mike, can I get your comment on this patch?  It almost works, but I am getting reproducible failures similar to

3939 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_bug326337.html | uncaught exception - NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIDOMJSWindow.open] at http://mochi.test:8888/tests/content/base/test/test_bug326337.html:23

in a couple of try runs.  Any thoughts?
Attachment #756133 - Flags: feedback?(mh+mozilla)
CCing some ateam folks to see if they have any ideas.

Locally, with a test file that does "window.open('about:blank')", I see:

First time:
- Pop-up blocker opens, works on accept
Second time:
- Pop-up blocker doesn't pop up, but doesn't work

adb logcat of the two runs below:

E GeckoConsole(6652)          [JavaScript Error: "The character encoding of the HTML document was not declared. The document will render with garbled text in some browser configurations if the document contains characters from outside the US-ASCII range. The character encoding of the page must be declared in the document or in the transfer protocol." {file: "http://www.ncalexander.net/t/" line: 0}]
D dalvikvm(6652)              GC_CONCURRENT freed 635K, 7% free 11268K/12039K, paused 2ms+9ms
D dalvikvm(1988)              GC_EXPLICIT freed 532K, 22% free 15830K/20103K, paused 3ms+9ms
I GeckoToolbar(6652)          zerdatime 4497134 - Throbber stop
E Profiler(6652)              BPUnw: [6 total] thread_register_for_profiling(me=0x38b220, stacktop=0x60cd2dff)
E Profiler(6652)              BPUnw: [7 total] thread_register_for_profiling(me=0x34eee8, stacktop=0x60e26dc7)
E Profiler(6652)              BPUnw: [8 total] thread_register_for_profiling(me=0x357428, stacktop=0x60f93df7)
V WindowOrientationListener(1988) nearestRotation : 0   Angle: 357   tilt: 52
E Profiler(6652)              BPUnw: [7 total] thread_unregister_for_profiling(me=0x357428) 
E GeckoConsole(6652)          Allowing popups for: [xpconnect wrapped nsIURI]
D KeyguardUpdateMonitor(1988) received broadcast android.intent.action.BATTERY_CHANGED
D KeyguardUpdateMonitor(1988) handleBatteryUpdate
V WindowOrientationListener(1988) nearestRotation : 0   Angle: 351   tilt: 49
D KeyguardUpdateMonitor(1988) received broadcast android.intent.action.TIME_TICK
D KeyguardUpdateMonitor(1988) handleTimeUpdate
W InputManagerService(1988)   Window already focused, ignoring focus gain of: com.android.internal.view.IInputMethodClient$Stub$Proxy@41a66868
I GeckoToolbar(6652)          zerdatime 4517268 - Throbber start
I GeckoToolbar(6652)          zerdatime 4517637 - Throbber start
D GeckoFavicons(6652)         Requesting cancelation of favicon load (4)
E GeckoConsole(6652)          zerdatime 1370285763809 - browser chrome startup finished.
E GeckoConsole(6652)          [JavaScript Error: "Error: Only one top-level window could used with AccessFu" {file: "resource://gre/modules/accessibility/Utils.jsm" line: 30}]
E GeckoConsole(6652)          [JavaScript Error: "NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIDOMJSWindow.open]" {file: "http://www.ncalexander.net/t/" line: 18}]
E GeckoConsole(6652)          [JavaScript Error: "The character encoding of the HTML document was not declared. The document will render with garbled text in some browser configurations if the document contains characters from outside the US-ASCII range. The character encoding of the page must be declared in the document or in the transfer protocol." {file: "http://www.ncalexander.net/t/" line: 0}]
I'm afraid I have no idea what's going on here. :(
This looks to be clean.  Here's an updated try build:

https://tbpl.mozilla.org/?tree=Try&rev=b0e940034bfd

The rc oranges are due to a trivial error (fixed!) in testJNI.js that passes locally and wesj can verify.  I will get QA to verify Flash and other media still work.
Assignee: nobody → nalexander
Attachment #756133 - Attachment is obsolete: true
Attachment #756133 - Flags: feedback?(mh+mozilla)
Attachment #758358 - Flags: review?(wjohnston)
Attachment #758359 - Flags: review?(mh+mozilla)
Attachment #758360 - Flags: review?(mh+mozilla)
kats, glandium is going to be high latency for a month.  Is this something you can review?  If not, can you think of somebody else?
Flags: needinfo?(bugmail.mozilla)
Comment on attachment 758359 [details] [diff] [review]
Part 1: Move Gecko .so libraries into assets/ directory of Android APK. r=glandium

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

::: mobile/android/modules/JNI.jsm
@@ +29,3 @@
>      delete this.lib;
> +    return this.lib = ctypes.open(apkName != null ?
> +                                  apkName + "!/assets/libxul.so" : "assets/libxul.so");

That shouldn't be needed: opening without a path should find the already loaded one. If that doesn't work, there's something else wrong.

::: mozglue/linker/ElfLoader.cpp
@@ +171,5 @@
> +  if (lastBang) {
> +    const char *firstSlash = strchr(lastBang, '/');
> +    if (firstSlash)
> +      return firstSlash + 1;
> +    return lastBang + 1;

Ah, this is what's breaking ctypes.open. Don't do this. The leaf name is always the file name. Not the part after !/.

::: toolkit/mozapps/installer/packager.mk
@@ +362,5 @@
>    ( cd $(STAGEPATH)$(MOZ_PKG_DIR)$(_BINPATH) && \
>      mkdir -p lib/$(ABI_DIR) && \
>      mv libmozglue.so $(MOZ_CHILD_PROCESS_NAME) lib/$(ABI_DIR) && \
> +    mkdir -p assets && \
> +    cp $(SO_LIBRARIES) assets && \

mv
Attachment #758359 - Flags: review?(mh+mozilla) → review-
Comment on attachment 758360 [details] [diff] [review]
Part 2: Move omni.ja into assets/ directory of Android APK. r=glandium

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

::: xpcom/build/Omnijar.cpp
@@ +78,5 @@
>          return;
>      }
>  
>      nsRefPtr<nsZipHandle> handle;
> +    if (NS_SUCCEEDED(nsZipHandle::Init(zipReader, "assets/" NS_STRINGIFY(OMNIJAR_NAME), getter_AddRefs(handle)))) {

I'd be more in favor of something that makes you specify the complete url to Omnijar::Init, but meh, can do that in a followup (so probably, never ;) ).
Attachment #758360 - Flags: review?(mh+mozilla) → review+
Flags: needinfo?(bugmail.mozilla)
Comment on attachment 758358 [details] [diff] [review]
Part 0: Add a sanity test for JNI.jsm. r=wesj

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

Neat!
Attachment #758358 - Flags: review?(wjohnston) → review+
Blocks: 880118
Okay, let's try this again.  First try run, I was doing this for all Android OSes, which busted b2g, but everything else looks good:

https://tbpl.mozilla.org/?tree=Try&rev=7133f5a44c8e

Second try run, appears to have not run the b2g tests, but I'm happy with the Android green and believe I have avoided any b2g change:

https://tbpl.mozilla.org/?tree=Try&rev=2bbcd180702a
This ended up being simpler than I thought.  I tried this first and I believe it failed because I wasn't properly storing omni.ja in the APK.  Seems to work well now.
Attachment #758360 - Attachment is obsolete: true
Attachment #760740 - Flags: review?(mh+mozilla)
This ended up being /much/ simpler than I thought -- existing code works as written!  I believe this failed because I wasn't properly storing the .so files in the APK.

One irritation is that each |make package| invocation stages, szips, and mvs the .so files into assets/.  It would be nice to actually manage the dependency so that we don't incur the expensive szip each time through.  At the moment, with the copying and szip-in-place, that's not easy to arrange.  Any thoughts on how best to do that, Mike?
Attachment #758359 - Attachment is obsolete: true
Attachment #760741 - Flags: review?(mh+mozilla)
Comment on attachment 760740 [details] [diff] [review]
Move omni.ja into assets directory.

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

::: configure.in
@@ +7821,5 @@
>  fi
>  
> +if test "$MOZ_WIDGET_TOOLKIT" = "android"; then
> +    dnl On Android, static resources live in the assets/ folder of the APK.
> +    OMNIJAR_NAME=assets/omni.ja

clever trick.
Attachment #760740 - Flags: review?(mh+mozilla) → review+
Attachment #760741 - Flags: review?(mh+mozilla) → review+
(In reply to Nick Alexander :nalexander from comment #25)
> One irritation is that each |make package| invocation stages, szips, and mvs
> the .so files into assets/.  It would be nice to actually manage the
> dependency so that we don't incur the expensive szip each time through.  At
> the moment, with the copying and szip-in-place, that's not easy to arrange. 
> Any thoughts on how best to do that, Mike?

I think the best would be not to have to move the files at packaging time, by either making the files go under assets/ directly when building them (by setting FINAL_TARGET in the corresponding makefiles), or allowing to add a destination subdirectory in the package manifest for the packager itself to install files directly under assets/. I'd probably favor the latter approach (on the syntax level, it would make sense to add such destination information either on each line, as an optional second item, but that may conflict with possible whitespaces in file names, or as an extension to the [component] syntax, which could become [foo "dest/subdir"]). Feel free to do that in a followup.
So, this is strange.  What's happening is that the l10n repack is running the updated |make unpack| target from the landed changesets, but it's downloading an APK built without the landed changesets.  Since the changesets moved some files, there's a mismatch and a failure.  In particular, the log for the failing Nightly build at

https://tbpl.mozilla.org/php/getParsedLog.php?id=24101202&tree=Mozilla-Central&full=1

says that it's building with revision 68760713a30f and does in fact download an APK from

http://stage.mozilla.org/pub/mozilla.org/mobile/nightly/latest-mozilla-central-android/en-US/fennec-24.0a1.en-US.android-arm.apk

The corresponding fennec-24.0a1.en-US.android-arm.txt says

20130613031237
http://hg.mozilla.org/mozilla-central/rev/68760713a30f

so that checks out.  The downloaded APK has the files in their original locations, which makes sense, since my changesets aren't included in that build:

archo:mozilla-central ncalexan$ hg contains -c 44d88ac4b3e3 68760713a30f
false

I conclude that the l10n code itself is not reporting its own revision correctly.  My guess is that this will sort itself out tomorrow night, when the downloaded APK will have the correct layout, and I'd like to let the patches ride until then.  aki, do you think this is reasonable?  (If you're the wrong person, please re-assign.)
Flags: needinfo?(aki)
a) this makes sense.
I'm not sure how to fix this: the current l10n workflow, iirc, goes:

* pull gecko
* configure
* use the configured objdir to grab the latest nightly (make wget-en-US)
* extract nightly, figure out revision nightly was built from
* update sourcedir to that revision
* re-configure, start repack

b) we can kick off nightlies now, rather than waiting for tonight, if you'd like.
Flags: needinfo?(aki)
(In reply to Aki Sasaki [:aki] from comment #32)
> a) this makes sense.
> I'm not sure how to fix this: the current l10n workflow, iirc, goes:
> 
> * pull gecko
> * configure
> * use the configured objdir to grab the latest nightly (make wget-en-US)
> * extract nightly, figure out revision nightly was built from
> * update sourcedir to that revision

Based on my reading of the logs, it looks like this doesn't actually happen.  The relevant lines look like:

    720:04:29:51     INFO - Running command: ['hg', '--config', 'ui.merge=internal:merge', 'update', '-C', '-r', 'default'] in /builds/hg-shared/mozilla-central
    721:04:29:51     INFO - Copy/paste: hg --config ui.merge=internal:merge update -C -r default
    736:04:30:16     INFO - Updating /builds/slave/m-cen-and-l10n_1-0000000000000/build/mozilla-central revision default.
    737:04:30:16     INFO - Running command: ['hg', '--config', 'ui.merge=internal:merge', 'update', '-C', '-r', 'default'] in /builds/slave/m-cen-and-l10n_1-0000000000000/build/mozilla-central
    738:04:30:16     INFO - Copy/paste: hg --config ui.merge=internal:merge update -C -r default

Is default changed during that build script?  That would be tricksy.  If not, we should probably specify the actual build revision to |hg update|.

> * re-configure, start repack
> 
> b) we can kick off nightlies now, rather than waiting for tonight, if you'd
> like.

Let's do this.  Can you kick off Nightlies based on, say, https://hg.mozilla.org/mozilla-central/rev/b197bed90a98?  Then I have a chance to back out before EOD PST if there is still an issue.
It does happen.  For example, from yesterday's nightly:

http://ftp.mozilla.org/pub/mozilla.org/mobile/tinderbox-builds/mozilla-central-l10n/mozilla-central-android-l10n_1-unknown-bm61-build1-build11.txt.gz

04:47:30     INFO -  gecko_revision 0414d6d0f60d
04:47:30     INFO - fennec_revision 0414d6d0f60d
04:47:30     INFO - buildid 20130612031138
04:47:30     INFO - Running command: ['mock_mozilla', '-r', 'mozilla-centos6-i386', '-q', '--cwd', '/builds/slave/m-cen-and-l10n_1-0000000000000/build/mozilla-central', '--unpriv', '--shell', '/usr/bin/env "LESSOPEN=|/usr/bin/lesspipe.sh %s" LOGNAME=cltbld USER=cltbld MOZ_OBJDIR=obj-l10n SYMBOL_SERVER_USER=ffxbld DISPLAY=:2 CCACHE_UMASK=002 LANG=en_US.UTF-8 CCACHE_HASHDIR= TERM=linux SHELL=/bin/bash MOZ_SIGNING_SERVERS=signing1.build.scl1.mozilla.com:9100,signing2.build.scl1.mozilla.com:9100,signing3.srv.releng.scl3.mozilla.com:9100 SHLVL=1 G_BROKEN_FILENAMES=1 HISTSIZE=1000 SYMBOL_SERVER_PATH=/mnt/netapp/breakpad/symbols_mob/ JAVA_HOME=/tools/jdk6 HG_SHARE_BASE_DIR=/builds/hg-shared SYMBOL_SERVER_HOST=symbolpush.mozilla.org EN_US_BINARY_URL=http://stage.mozilla.org/pub/mozilla.org/mobile/nightly/latest-mozilla-central-android/en-US CCACHE_DIR=/builds/ccache SHIP_LICENSED_FONTS=1 PATH=/tools/jdk6/bin:/tools/jdk6/bin:/opt/local/bin:/tools/python/bin:/tools/buildbot/bin:/usr/kerberos/bin:/usr/local/bin:/bin:/usr/bin:/home/ TINDERBOX_OUTPUT=1 "MOZ_SIGN_CMD=python /builds/slave/m-cen-and-l10n_1-0000000000000/build/tools/release/signing/signtool.py --cachedir /builds/slave/m-cen-and-l10n_1-0000000000000/build/signing_cache -t /builds/slave/m-cen-and-l10n_1-0000000000000/token -n /builds/slave/m-cen-and-l10n_1-0000000000000/nonce -c /builds/slave/m-cen-and-l10n_1-0000000000000/build/tools/release/signing/host.cert -f jar -H signing1.build.scl1.mozilla.com:9100 -H signing2.build.scl1.mozilla.com:9100 -H signing3.srv.releng.scl3.mozilla.com:9100" LC_ALL=C _=/tools/buildbot/bin/python LOCALE_MERGEDIR=/builds/slave/m-cen-and-l10n_1-0000000000000/build/mozilla-central/obj-l10n/merged/ MAIL=/var/spool/mail/cltbld MOZ_UPDATE_CHANNEL=nightly HOSTNAME=bld-linux64-ec2-302.build.aws-us-east-1.mozilla.com SYMBOL_SERVER_SSH_KEY=/home/mock_mozilla/.ssh/ffxbld_dsa HISTCONTROL=ignoredups POST_SYMBOL_UPLOAD_CMD=/usr/local/bin/post-symbol-upload.py PWD=/builds/slave/m-cen-and-l10n_1-0000000000000 PROPERTIES_FILE=/builds/slave/m-cen-and-l10n_1-0000000000000/buildprops.json MOZ_CRASHREPORTER_NO_REPORT=1 CCACHE_COMPRESS=1 hg update -r 0414d6d0f60d'] in /builds/slave/m-cen-and-l10n_1-0000000000000/build/mozilla-central


From last night's nightly, https://tbpl.mozilla.org/php/getParsedLog.php?id=24101202&tree=Mozilla-Central&full=1 , it died in |make unpack| so didn't get a chance to see what revision to update to.

I'll kick an Android nightly.
(In reply to Aki Sasaki [:aki] from comment #34)
> It does happen.  For example, from yesterday's nightly:
> 
> http://ftp.mozilla.org/pub/mozilla.org/mobile/tinderbox-builds/mozilla-
> central-l10n/mozilla-central-android-l10n_1-unknown-bm61-build1-build11.txt.
> gz

I agree this build is pulling the correct revision.

> From last night's nightly,
> https://tbpl.mozilla.org/php/getParsedLog.php?id=24101202&tree=Mozilla-
> Central&full=1 , it died in |make unpack| so didn't get a chance to see what
> revision to update to.

I think this is a bug.  From your earlier list:

> * pull gecko
> * configure
> * use the configured objdir to grab the latest nightly (make wget-en-US)
> * extract nightly, figure out revision nightly was built from
> * update sourcedir to that revision

Can't we determine the revision Nightly was built from by pulling the fennec*txt file and parsing the rev id?  Then we wouldn't run make targets from differing revisions.

> I'll kick an Android nightly.

Thanks!  And thanks for your quick responses.  This process is confusing :(
(In reply to Nick Alexander :nalexander from comment #35)
> > From last night's nightly,
> > https://tbpl.mozilla.org/php/getParsedLog.php?id=24101202&tree=Mozilla-
> > Central&full=1 , it died in |make unpack| so didn't get a chance to see what
> > revision to update to.
> 
> I think this is a bug.  From your earlier list:
> 
> > * pull gecko
> > * configure
> > * use the configured objdir to grab the latest nightly (make wget-en-US)
> > * extract nightly, figure out revision nightly was built from
> > * update sourcedir to that revision
> 
> Can't we determine the revision Nightly was built from by pulling the
> fennec*txt file and parsing the rev id?  Then we wouldn't run make targets
> from differing revisions.

I think that's accurate.
Reading the revision from http://ftp.mozilla.org/pub/mozilla.org/mobile/nightly/latest-mozilla-central-android/en-US/fennec-24.0a1.en-US.android-arm.txt would be better.
I don't think that file existed when I first wrote this script.

However, we don't know the name of this file until we run configure.

We can solve this in a couple ways:

a) upload that text file twice, once with a predictable name (e.g., without versions) and follow a workflow like:

* grab text file
* pull gecko & update to revision
* configure (once!)
* make wget-en-US, extract, proceed with repack

or

b) follow a workflow like:

* pull gecko
* configure
* use the configured objdir to grab the latest revision text file (new makefile target)
* update sourcedir to that revision
* make wget-en-US
* extract, proceed with the repack

Either way, we'd have to add that additional makefile target / additional text file upload to all repos where we want to run single locale repacks.  Essentially, central, aurora, beta, release, IIRC.

> > I'll kick an Android nightly.
> 
> Thanks!  And thanks for your quick responses.  This process is confusing :(

Np.  Visible here: https://tbpl.mozilla.org/?rev=b197bed90a98&jobname=nightly
(In reply to Aki Sasaki [:aki] from comment #36)
> b) follow a workflow like:
> 
> * pull gecko
> * configure
> * use the configured objdir to grab the latest revision text file (new
> makefile target)
> * update sourcedir to that revision
* configure 2nd time
> * make wget-en-US
> * extract, proceed with the repack
Okay, got farther this time:

https://tbpl.mozilla.org/?rev=b197bed90a98&jobname=nightly&showall=1

Issue is now that l10n-repack.py can't find omni.ja, and that's because OMNIJAR_NAME=assets/omni.ja and I'm deleting assets/ entirely:

http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/packager.mk#386

We had some follow up work about not moving things into assets directly in packager.mk that I'm going to look into while addressing this.
This adds a Component type to mozbuild.mozpack and teaches the
packager to accept components of the form [name destdir=dir].  Then we
update the Android package manifest and simplify the packager code.

I would have liked to make the packager put mozglue.so and
MOZ_CHILD_PROCESS_NAME in lib/$(ABI_DIR) directly, but this turned out
to be awkward.  Since MOZ_CHILD_PROCESS_NAME needs to have lib/ in its
name to load successfully on Android, we would have to add notation in
package manifests to install bin/lib/*plugin-container* to
lib/$(ABI_DIR)/*plugin-container*.

I tried to make the option parsing future-useful, since it seems
likely we'll want to grow manifest options, but I cut corners by
breaking on whitespace.
Attachment #762942 - Flags: review?(mh+mozilla)
(In reply to Nick Alexander :nalexander from comment #39)
> Created attachment 762942 [details] [diff] [review]
> Follow-up: Teach packager to put Android libraries in assets/. r=glandium
> 
> This adds a Component type to mozbuild.mozpack and teaches the
> packager to accept components of the form [name destdir=dir].  Then we
> update the Android package manifest and simplify the packager code.
> 
> I would have liked to make the packager put mozglue.so and
> MOZ_CHILD_PROCESS_NAME in lib/$(ABI_DIR) directly, but this turned out
> to be awkward.  Since MOZ_CHILD_PROCESS_NAME needs to have lib/ in its
> name to load successfully on Android, we would have to add notation in
> package manifests to install bin/lib/*plugin-container* to
> lib/$(ABI_DIR)/*plugin-container*.
> 
> I tried to make the option parsing future-useful, since it seems
> likely we'll want to grow manifest options, but I cut corners by
> breaking on whitespace.

Hi ted, can you review this patch?  It should fix language repacks for Android Nightlies, which I broke last Friday.  I could land a one-line band-aid that would fix language repacks, but this solution also saves local developers re-szipping every single |make package|.  For context, see https://bugzilla.mozilla.org/show_bug.cgi?id=873569#c27.
Flags: needinfo?(ted)
Comment on attachment 762942 [details] [diff] [review]
Follow-up: Teach packager to put Android libraries in assets/. r=glandium

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

::: python/mozbuild/mozpack/packager/__init__.py
@@ +38,5 @@
> +
> +    @staticmethod
> +    def _split_component_and_options(string):
> +        '''
> +        Split 'name key1=value1 key2=value2' into

i'd prefer name key1="value1"...

@@ +109,2 @@
>                  errors.fatal('Malformed manifest')
> +            self._component = Component.from_string(str[1:-1])

please keep the else. errors.fatal doesn't raise when errors are being accumulated.

::: python/mozbuild/mozpack/test/test_packager.py
@@ +58,5 @@
>                  self.log = []
>  
> +            def _repr(self, component):
> +                if component.destdir:
> +                    return "%s destdir=%s" % (component.name, component.destdir)

you might as well implement Component.__repr__

@@ +235,5 @@
>               'add_manifest', ManifestResource('foo', 'foo', 'foo/')),
>              (None, 'add', 'foo/bar', foobar),
>              (None, 'add', 'foo/baz', foobaz),
>              (None, 'add', 'foo/qux', fooqux),
> +            (None, 'add', 'destdir/foo/zot', foozot),

I'm wondering if it wouldn't make sense to make the target destdir/zot, which would allow to move files. Although then you get something rather unexpected with directories... :-/
Attachment #762942 - Flags: review?(mh+mozilla) → feedback+
> @@ +235,5 @@
> >               'add_manifest', ManifestResource('foo', 'foo', 'foo/')),
> >              (None, 'add', 'foo/bar', foobar),
> >              (None, 'add', 'foo/baz', foobaz),
> >              (None, 'add', 'foo/qux', fooqux),
> > +            (None, 'add', 'destdir/foo/zot', foozot),
> 
> I'm wondering if it wouldn't make sense to make the target destdir/zot,
> which would allow to move files. Although then you get something rather
> unexpected with directories... :-/

My thought for this is to implement srcdir as well.  This gives you the flexibility to split foo/bar/baz however you'd like:

[comp srcdir=foo/bar destdir=zot]
baz

moves foo/bar/baz -> zot/baz

This will let me move lib/plugin-container to lib/armeabi/plugin-container for example, and the same with mozglue.
With review comments addressed (and final "wondering" left for a different ticket), this is looking pretty good on try:

https://tbpl.mozilla.org/?tree=Try&rev=1844e440cadf
This adds a Component type to the mozbuild.mozpack package manifest
parser, and teaches the packager to accept components of the form
[name destdir="dir"].  Then we update the Android package manifest and
simplify the packager code.

I would have liked to make the packager put mozglue.so and
MOZ_CHILD_PROCESS_NAME in lib/$(ABI_DIR) directly, but this turned out
to be awkward.  Since MOZ_CHILD_PROCESS_NAME needs to have lib/ in its
name to load successfully on Android, we would have to add notation in
package manifests to install bin/lib/*plugin-container* to
lib/$(ABI_DIR)/*plugin-container*.
Attachment #762942 - Attachment is obsolete: true
Attachment #765612 - Flags: review?(mh+mozilla)
Flags: needinfo?(ted)
I am concerned that this bug will not be fixed by the time it is uplifted to Aurora.  Here's a try build with a trivial band-aid that should fix Nightly re-packs but does not address the better behaviour I was hoping to have reviewed by then end of the cycle.

https://tbpl.mozilla.org/?tree=Try&rev=a53ba49925c5

aki, if the build above looks green tomorrow, can you spin an N and Nk set for me?  You did this for me once before, which I appreciated.  I don't know how to manage that myself and testing locally isn't sufficient.
Flags: needinfo?(aki)
Pretty sure I got my comment syntax wrong.  Try:

https://tbpl.mozilla.org/?tree=Try&rev=d0cf23b880c2
(In reply to Nick Alexander :nalexander from comment #46)
> I am concerned that this bug will not be fixed by the time it is uplifted to
> Aurora.  Here's a try build with a trivial band-aid that should fix Nightly
> re-packs but does not address the better behaviour I was hoping to have
> reviewed by then end of the cycle.
> 
> https://tbpl.mozilla.org/?tree=Try&rev=a53ba49925c5
> 
> aki, if the build above looks green tomorrow, can you spin an N and Nk set
> for me?  You did this for me once before, which I appreciated.  I don't know
> how to manage that myself and testing locally isn't sufficient.

There's no easy/fast way to spin a nightly on Try, as nightlies aren't enabled on Try.
I could potentially manually kick off a nightly, but this will be a) manually time consuming, b) aiui lower priority than my b2g work, and c) not guaranteed to produce green results.

Do you have a strategy for disabling this on aurora?
Flags: needinfo?(aki)
> Do you have a strategy for disabling this on aurora?

As I said to aki in #releng, I'm just going to land this fix on m-c directly, since it won't make anything worse and it's not feasible to test language repacks.

As for disabling this on Aurora, I don't have a good strategy.  I expect the band-aid to fix repacks, but if it doesn't I may have to backout all earlier patches.  That would be really unfortunate :(
Comment on attachment 765612 [details] [diff] [review]
Follow-up: Teach packager to put Android libraries in assets/. r=glandium

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

::: python/mozbuild/mozpack/packager/__init__.py
@@ +56,5 @@
> +        # don't match the regular expression, they were not
> +        # well-formed.
> +        keyVal = re.compile(r'''
> +        \s*                 # optional whitespace.
> +        (                   # key-value group.

You actually don't need to capture the key-value group. split is always going to give you non-matching, first group, second group, third group, non-matching, first group, second group, third group, non-matching, etc. You can just check that the non-matching parts are always ''.

@@ +59,5 @@
> +        \s*                 # optional whitespace.
> +        (                   # key-value group.
> +        ([a-zA-Z0-9_]+)     # key.
> +        \s*=\s*             # optional space around =.
> +        "([^"]*?)"          # value without quotes.

You don't need an explicit non-greedy match here.

@@ +61,5 @@
> +        ([a-zA-Z0-9_]+)     # key.
> +        \s*=\s*             # optional space around =.
> +        "([^"]*?)"          # value without quotes.
> +        )
> +        \s*

(?:\s+|$) would be better, i think.

@@ +67,5 @@
> +
> +        options = {}
> +        splits = keyVal.split(string)
> +        i = 0
> +        while i < len(splits):

A more pythonic way to iterate the splits is to do:
for no_match, key, val in zip(*[iter(splits)] * 3):
  if no_match:
    errors.fatal('...')
    return None
  options[key] = val

See http://stackoverflow.com/questions/2233204/how-does-zipitersn-work-in-python for how this works. The zip() call might grant being in a separate function with a nicer name and comment for the code here to be more readable.

@@ +105,5 @@
> +        matches = componentRemainder.match(string)
> +        if not matches:
> +            errors.fatal('Malformed manifest: no component found')
> +            return
> +        component, remainder = matches.groups()

The above can be replaced with:
splits = string.split(None, 1)
component = splits[0]
remainder = splits[1] if len(splits) > 1 else ''

or

def unpack2(x, *y):
  return x, y[0] if y else ''

component, remainder = unpack2(*string.split(None, 1))

@@ +108,5 @@
> +            return
> +        component, remainder = matches.groups()
> +        options = Component._split_options(remainder)
> +        if options is None:
> +            errors.fatal('Malformed manifest: invalid options found')

_split_options is already going to report the error.

@@ +119,5 @@
> +        Create a component from a string.
> +        '''
> +        nameOptions = Component._split_component_and_options(string)
> +        if nameOptions is None:
> +            errors.fatal('Malformed manifest: invalid name or options found')

_split_options, called by _split_component_and_options is already going to report the error.
Attachment #765612 - Flags: review?(mh+mozilla) → review-
(In reply to Nick Alexander :nalexander from comment #49)
> > Do you have a strategy for disabling this on aurora?
> 
> As I said to aki in #releng, I'm just going to land this fix on m-c
> directly, since it won't make anything worse and it's not feasible to test
> language repacks.
> 
> As for disabling this on Aurora, I don't have a good strategy.  I expect the
> band-aid to fix repacks, but if it doesn't I may have to backout all earlier
> patches.  That would be really unfortunate :(

I saw you trying to run this manually -- hopefully that's working for you.
If not, I can help more, or you can request a project branch with android nightlies+l10n that you can use to verify fixes.
Depends on: 887121, 887115
Depends on: 895442
Depends on: 912377
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: