1040945 - Make AndroidEclipse backend make target install native libraries more aggressively

Status: RESOLVED FIXED

(Firefox Build System :: Android Studio and Gradle Integration, defect)

Description

[Nick Alexander :nalexander [he/him]]

At the moment, native libraries (libmozglue and libplugin-container) are installed as part of a ** glob.  That means if they're missing, that's okay.  But it's not okay -- an Eclipse-built package missing those libraries crashes immediately.

Let's prevent the foot-gun by specifying them explicitly.

Comment 1

[Nick Alexander :nalexander [he/him]]

Attachment: Copy native libraries by name in AndroidEclipse backend make target. r=rnewman

Comment 2

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8458925 [details] [diff] [review]
Copy native libraries by name in AndroidEclipse backend make target. r=rnewman

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

gps: you might take this one as well, since Bug 1040938 is in the same area.  This is the only use of project.libs in the tree.

Comment 3

[Richard Newman [:rnewman]]

Comment on attachment 8458925 [details] [diff] [review]
Copy native libraries by name in AndroidEclipse backend make target. r=rnewman

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

I'm fine with this if Greg is.

::: mobile/android/base/Makefile.in
@@ +407,5 @@
> +	$(MKDIR) $(DIST)/fennec/lib/$(ANDROID_CPU_ARCH)
> +	rsync --update $(DIST)/fennec/libmozglue.so $(DIST)/fennec/lib/$(ANDROID_CPU_ARCH)/libmozglue.so
> +	rsync --update $(DIST)/fennec/lib/libplugin-container.so $(DIST)/fennec/lib/$(ANDROID_CPU_ARCH)/libplugin-container.so
> +	$(RM) $(DIST)/fennec/libmozglue.so
> +	$(RM) $(DIST)/fennec/lib/libplugin-container.so

Is there a reason to do this stuff rather than moving the two files?

Comment 4

[Mike Hommey [:glandium]]

Why not just change the package manifest with a destdir ?

Comment 5

[Nick Alexander :nalexander [he/him]]

(In reply to Mike Hommey [:glandium] from comment #4)
> Why not just change the package manifest with a destdir ?

At one time, I wanted to do this, and I can't recall if there was a reason I didn't or if I just didn't get to it.  I'll look into doing this in the mobile/android package-manifest.in.

Comment 6

[Nick Alexander :nalexander [he/him]]

(In reply to Richard Newman [:rnewman] from comment #3)
> Comment on attachment 8458925 [details] [diff] [review]
> Copy native libraries by name in AndroidEclipse backend make target.
> r=rnewman
> 
> Review of attachment 8458925 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm fine with this if Greg is.
> 
> ::: mobile/android/base/Makefile.in
> @@ +407,5 @@
> > +	$(MKDIR) $(DIST)/fennec/lib/$(ANDROID_CPU_ARCH)
> > +	rsync --update $(DIST)/fennec/libmozglue.so $(DIST)/fennec/lib/$(ANDROID_CPU_ARCH)/libmozglue.so
> > +	rsync --update $(DIST)/fennec/lib/libplugin-container.so $(DIST)/fennec/lib/$(ANDROID_CPU_ARCH)/libplugin-container.so
> > +	$(RM) $(DIST)/fennec/libmozglue.so
> > +	$(RM) $(DIST)/fennec/lib/libplugin-container.so
> 
> Is there a reason to do this stuff rather than moving the two files?

This preserves timestamps if nothing's changed.  I'm not confident it's necessary, but the more operations we make idempotent, the better off we are.

Comment 7

[Gregory Szorc [:gps]]

Comment on attachment 8458925 [details] [diff] [review]
Copy native libraries by name in AndroidEclipse backend make target. r=rnewman

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

Hacks, hacks, everywhere. Par for the course on the build system.

I defer mostly to your domain expertise here.

::: mobile/android/base/Makefile.in
@@ +407,5 @@
> +	$(MKDIR) $(DIST)/fennec/lib/$(ANDROID_CPU_ARCH)
> +	rsync --update $(DIST)/fennec/libmozglue.so $(DIST)/fennec/lib/$(ANDROID_CPU_ARCH)/libmozglue.so
> +	rsync --update $(DIST)/fennec/lib/libplugin-container.so $(DIST)/fennec/lib/$(ANDROID_CPU_ARCH)/libplugin-container.so
> +	$(RM) $(DIST)/fennec/libmozglue.so
> +	$(RM) $(DIST)/fennec/lib/libplugin-container.so

A comment here would be nice.

Comment 8

[Nick Alexander :nalexander [he/him]]

Attachment: Part 1: Install Android native libraries directly into lib/ANDROID_CPU_ARCH. r=glandium

This is a little tricky only because MOZ_CHILD_PROCESS_NAME is
"lib/libplugin-container.so" on Android, which meant special handling at
multiple. This patch reduces the number of such places.

glandium, this is as you suggested.  Unpackaging does the right thing locally.

Comment 9

[Nick Alexander :nalexander [he/him]]

Attachment: Part 2: Copy native libraries by name in AndroidEclipse backend make target. r=rnewman

Comment 10

[Nick Alexander :nalexander [he/him]]

Attachment: Part 3: Update to Java 1.7 and bump SDK version in AndroidEclipse. r=me

Comment 11

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8482984 [details] [diff] [review]
Part 2: Copy native libraries by name in AndroidEclipse backend make target. r=rnewman

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

Carrying forward rnewman's r+ 'cuz the confusing part goes away with glandium's change.

Comment 12

[Nick Alexander :nalexander [he/him]]

Attachment: Part 6: Display device type icon. r=margaret

A nice simple one for you, margaret.  Feel free to redirect (to
mcomella?) if your queue is too long.

Comment 13

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8483794 [details] [diff] [review]
Part 6: Display device type icon. r=margaret

Sigh, wrong bug number.

Comment 14

[Nick Alexander :nalexander [he/him]]

This is part of the try push at https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a5e7a9b48ef9, which is green.

Comment 15

[Mike Hommey [:glandium]]

Comment on attachment 8482981 [details] [diff] [review]
Part 1: Install Android native libraries directly into lib/ANDROID_CPU_ARCH. r=glandium

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

There seems to be missing words in your commit message:
"which meant special handling at multiple."

Comment 16

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8482985 [details] [diff] [review]
Part 3: Update to Java 1.7 and bump SDK version in AndroidEclipse. r=me

This landed as Bug 1062566.

Comment 17

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/integration/fx-team/rev/3063f757bc97
https://hg.mozilla.org/integration/fx-team/rev/42aa92ad76c8

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3063f757bc97
https://hg.mozilla.org/mozilla-central/rev/42aa92ad76c8