Closed Bug 1040945 Opened 5 years ago Closed 5 years ago

Make AndroidEclipse backend make target install native libraries more aggressively

Categories

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

All
Android
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla35

People

(Reporter: nalexander, Assigned: nalexander)

References

(Depends on 1 open bug)

Details

Attachments

(2 files, 3 obsolete files)

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 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.
Attachment #8458925 - Flags: feedback?(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]:
-----------------------------------------------------------------

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?
Attachment #8458925 - Flags: review?(rnewman) → review+
Why not just change the package manifest with a destdir ?
(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.
(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 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.
Attachment #8458925 - Flags: feedback?(gps) → feedback+
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.
Attachment #8482981 - Flags: review?(mh+mozilla)
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.
Attachment #8482984 - Flags: review+
A nice simple one for you, margaret.  Feel free to redirect (to
mcomella?) if your queue is too long.
Attachment #8483794 - Flags: review?(margaret.leibovic)
Comment on attachment 8483794 [details] [diff] [review]
Part 6: Display device type icon. r=margaret

Sigh, wrong bug number.
Attachment #8483794 - Attachment is obsolete: true
Attachment #8483794 - Flags: review?(margaret.leibovic)
This is part of the try push at https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a5e7a9b48ef9, which is green.
Assignee: nobody → nalexander
Status: NEW → ASSIGNED
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."
Attachment #8482981 - Flags: review?(mh+mozilla) → review+
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.
Attachment #8482985 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/3063f757bc97
https://hg.mozilla.org/mozilla-central/rev/42aa92ad76c8
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
Product: Firefox for Android → Firefox Build System
Target Milestone: Firefox 35 → mozilla35
You need to log in before you can comment on or make changes to this bug.