Closed Bug 1223209 Opened 9 years ago Closed 8 years ago

Continue building GeckoView AARs

Categories

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

All
Android
defect
Not set
normal

Tracking

(firefox46 fixed)

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: jchen, Unassigned)

References

Details

Attachments

(2 files, 1 obsolete file)

Bug 1212347 turned off builds for both GeckoView AAR and GeckoView example. I think we still want the AARs to be built, and only disable GeckoView example. Nick, is that correct?
Flags: needinfo?(nalexander)
(In reply to Jim Chen [:jchen] [:darchons] from comment #0)
> Bug 1212347 turned off builds for both GeckoView AAR and GeckoView example.
> I think we still want the AARs to be built, and only disable GeckoView
> example. Nick, is that correct?

I think we'd like them to be built, but we have zero consumers outside of the tree.  I just don't care to make it happen with moz.build; if we do this, let's do it with Gradle.
Flags: needinfo?(nalexander)
We should keep building GeckoView library due to ongoing work on supporting
multiple GeckoViews.

This patch lets us keep building GeckoView library/AARs. It keeps
GeckoView example disabled because that doesn't currently build.
Attachment #8686391 - Flags: review?(nalexander)
Right now, each incremental build produces a new set of GeckoView library
files, but the previous files don't get cleaned up, and you end up with a bunch
of old libraries in your objdir. This patch cleans up the old files before
producing new ones, but I'm not sure if this is the best approach.
Attachment #8686392 - Flags: review?(nalexander)
Comment on attachment 8686391 [details] [diff] [review]
Build GeckoView library but not GeckoView example (v1)

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

::: mobile/android/confvars.sh
@@ +115,5 @@
>  # usage of the framework.
>  MOZ_SWITCHBOARD=1
>  
> +# Disable GeckoView example by default.
> +# MOZ_ANDROID_GECKOVIEW_EXAMPLE=1

How can this work?  It's disabled.

::: toolkit/mozapps/installer/upload-files.mk
@@ +350,5 @@
>  ifdef NIGHTLY_BUILD
>  ifndef MOZ_DISABLE_GECKOVIEW
>  INNER_MAKE_GECKOVIEW_LIBRARY= \
>    $(MAKE) -C ../mobile/android/geckoview_library package
> +ifdef MOZ_ANDROID_GECKOVIEW_EXAMPLE

I don't think this is available from confvars.sh directly.  I think you need to expose it from configure.in.
Attachment #8686391 - Flags: review?(nalexander) → review-
Comment on attachment 8686392 [details] [diff] [review]
Clean up old GeckoView library files (v1)

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

nits.  Feel free to land this (after issues) before the other part.  Thanks for digging in!

::: python/mozbuild/mozbuild/action/package_geckolibs_aar.py
@@ +175,5 @@
>      paths_to_hash.append(gecklibs_aar)
>      geckoview_aar = os.path.join(args.dir, 'geckoview-{revision}.aar').format(revision=args.revision)
>      paths_to_hash.append(geckoview_aar)
>  
> +    for f in os.listdir(args.dir):

We have nice code for listing matching files using FileFinder; use that.

We'd want this in automation and local builds, but it's irritating to have it when just running the code.  So let's add a --purge-old flag protecting this, defaulting to off, and add that flag at 

https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/upload-files.mk#387
Attachment #8686392 - Flags: review?(nalexander) → review+
(In reply to Nick Alexander :nalexander from comment #4)
> Comment on attachment 8686391 [details] [diff] [review]
> Build GeckoView library but not GeckoView example (v1)
> 
> Review of attachment 8686391 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/confvars.sh
> @@ +115,5 @@
> >  # usage of the framework.
> >  MOZ_SWITCHBOARD=1
> >  
> > +# Disable GeckoView example by default.
> > +# MOZ_ANDROID_GECKOVIEW_EXAMPLE=1
> 
> How can this work?  It's disabled.

Right, we want to enable GeckoView library but disable GeckoView example.

The patch first gets rid of MOZ_DISABLE_GECKOVIEW=1, so now both the library and example are enabled.

To uncouple the example from the library, the patch introduces MOZ_ANDROID_GECKOVIEW_EXAMPLE, which conditionally enables GeckoView example.

Finally, by commenting out MOZ_ANDROID_GECKOVIEW_EXAMPLE=1, we now have GeckoView example disabled.
(In reply to Jim Chen [:jchen] [:darchons] from comment #6)
> (In reply to Nick Alexander :nalexander from comment #4)
> > Comment on attachment 8686391 [details] [diff] [review]
> > Build GeckoView library but not GeckoView example (v1)
> > 
> > Review of attachment 8686391 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: mobile/android/confvars.sh
> > @@ +115,5 @@
> > >  # usage of the framework.
> > >  MOZ_SWITCHBOARD=1
> > >  
> > > +# Disable GeckoView example by default.
> > > +# MOZ_ANDROID_GECKOVIEW_EXAMPLE=1
> > 
> > How can this work?  It's disabled.
> 
> Right, we want to enable GeckoView library but disable GeckoView example.
> 
> The patch first gets rid of MOZ_DISABLE_GECKOVIEW=1, so now both the library
> and example are enabled.
> 
> To uncouple the example from the library, the patch introduces
> MOZ_ANDROID_GECKOVIEW_EXAMPLE, which conditionally enables GeckoView example.
> 
> Finally, by commenting out MOZ_ANDROID_GECKOVIEW_EXAMPLE=1, we now have
> GeckoView example disabled.

OK, I'm fine with the approach.  But I really don't think this works as you want, and even if it does, it's opaque: setting a variable (behind a comment, no less!) in confvars.sh that only gets picked up in a packager Make file.  Instead, define this flag in configure.in, defaulting to off, and roll on.
I think a better approach is to remove geckoview_example entirely. It's broken
and obsolete, and we haven't maintained it for a long time. Removing it lets us
continue building and uploading the AAR libraries without geckoview_example
getting in the way.
Attachment #8707143 - Flags: review?(nalexander)
Attachment #8707143 - Flags: review?(mark.finkle)
Attachment #8686391 - Attachment is obsolete: true
Comment on attachment 8707143 [details] [diff] [review]
Remove geckoview_example (v1)

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

Huzzah!  If it builds in automation and locally, roll on.

I think we can remove the ant requirement from the builders (https://dxr.mozilla.org/mozilla-central/source/mobile/android/config/tooltool-manifests/android/releng.manifest#27) and from mozboot (https://dxr.mozilla.org/mozilla-central/source/python/mozboot/mozboot/osx.py#337, https://dxr.mozilla.org/mozilla-central/source/python/mozboot/mozboot/debian.py#77, https://dxr.mozilla.org/mozilla-central/source/python/mozboot/mozboot/fedora.py#46).  Feel free to punt to follow-up if you prefer.
Attachment #8707143 - Flags: review?(nalexander) → review+
Comment on attachment 8707143 [details] [diff] [review]
Remove geckoview_example (v1)

Sounds OK to me.
Attachment #8707143 - Flags: review?(mark.finkle) → review+
https://hg.mozilla.org/mozilla-central/rev/3f8b80d42f00
https://hg.mozilla.org/mozilla-central/rev/0c8cd67f497e
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
(In reply to Nick Alexander :nalexander from comment #9)
> Comment on attachment 8707143 [details] [diff] [review]
> Remove geckoview_example (v1)
> 
> I think we can remove the ant requirement from the builders
> (https://dxr.mozilla.org/mozilla-central/source/mobile/android/config/
> tooltool-manifests/android/releng.manifest#27) and from mozboot
> (https://dxr.mozilla.org/mozilla-central/source/python/mozboot/mozboot/osx.
> py#337,
> https://dxr.mozilla.org/mozilla-central/source/python/mozboot/mozboot/debian.
> py#77,
> https://dxr.mozilla.org/mozilla-central/source/python/mozboot/mozboot/fedora.
> py#46).  Feel free to punt to follow-up if you prefer.

I ran into some problems with build failing on try after removing the ant requirement, so I decided to go with a follow-up.
Product: Firefox for Android → Firefox Build System
Target Milestone: Firefox 46 → mozilla46
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: