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)
Firefox Build System
Android Studio and Gradle Integration
All
Android
Tracking
(firefox46 fixed)
RESOLVED
FIXED
mozilla46
Tracking | Status | |
---|---|---|
firefox46 | --- | fixed |
People
(Reporter: jchen, Unassigned)
References
Details
Attachments
(2 files, 1 obsolete file)
1.61 KB,
patch
|
nalexander
:
review+
|
Details | Diff | Splinter Review |
9.70 KB,
patch
|
nalexander
:
review+
mfinkle
:
review+
|
Details | Diff | Splinter Review |
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)
Comment 1•9 years ago
|
||
(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)
Reporter | ||
Comment 2•9 years ago
|
||
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)
Reporter | ||
Comment 3•9 years ago
|
||
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 4•9 years ago
|
||
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 5•9 years ago
|
||
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+
Reporter | ||
Comment 6•9 years ago
|
||
(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.
Comment 7•9 years ago
|
||
(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.
Reporter | ||
Comment 8•8 years ago
|
||
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)
Reporter | ||
Updated•8 years ago
|
Attachment #8686391 -
Attachment is obsolete: true
Comment 9•8 years ago
|
||
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 10•8 years ago
|
||
Comment on attachment 8707143 [details] [diff] [review] Remove geckoview_example (v1) Sounds OK to me.
Attachment #8707143 -
Flags: review?(mark.finkle) → review+
Comment 11•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f8b80d42f00 https://hg.mozilla.org/integration/mozilla-inbound/rev/0c8cd67f497e
Comment 12•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3f8b80d42f00 https://hg.mozilla.org/mozilla-central/rev/0c8cd67f497e
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox46:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
Reporter | ||
Comment 13•8 years ago
|
||
(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.
Updated•5 years ago
|
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.
Description
•