Closed Bug 951865 Opened 11 years ago Closed 10 years ago

geckoview_example cannot build - missing res/ files

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

(firefox30 verified, fennec29+)

Status:
RESOLVED FIXED
Milestone:
Firefox 30
Tracking Flags:
Tracking Status
firefox30 --- verified
fennec 29+ ---

People

(Reporter: mcomella, Assigned: nalexander)

References

Details

Attachments

(3 files, 6 obsolete files)

Build output
5.05 KB, text/plain
Details
Part 2: Package GeckoView resources when building mobile/android/base.
6.26 KB, patch
nalexander
: review+
Details | Diff | Splinter Review
Part 1: Replace ABI_DIR with ANDROID_CPU_ARCH.
5.56 KB, patch
glandium
: review+
Details | Diff | Splinter Review
Attached file Build output — 
Built with `make package -C <objdir>/embedding/android/geckoview_example/`.

After debugging with nalexander on IRC, it seems that non-preprocessed resources have been removed from the objdir and can only be found in the source tree (bug 934646). However, it seems the GeckoView example still looks in the objdir for these files and so the build fails.

I tried working around this by symlinking attrs.xml into the geckoview_example/res/values/ dir, which built successfully, but then a runtime error occurred from missing additionally resource files.
this is a regression. changeset 96c4b3ea0540 is fine. I'll bisect
bisection points to bug 934646
Assignee: nobody → nalexander
Blocks: 934646
Mike - Can you check with Nick about a suitable solution to this? I think we just slurp all the resources nowadays. Maybe we could add a similar slurp for the geckoview_example makefile?
Assignee: nalexander → michael.l.comella
Flags: needinfo?(nalexander)
So, I think the issue is that the library is no longer included the correct resources: see

http://mxr.mozilla.org/mozilla-central/source/mobile/android/geckoview_library/Makefile.in#53

What I suggest is rather than this tom-foolery across directories, we extract the resources from the built APK file.  That is, we do a "pseudo-unpack" of the APK and grab whatever res/ files and lib/ files are needed from there (oh, and omni.ja).  mcomella, does this make sense?
Flags: needinfo?(nalexander) → needinfo?(michael.l.comella)
(In reply to Nick Alexander :nalexander from comment #4)
> So, I think the issue is that the library is no longer included the correct
> resources: see
> 
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/
> geckoview_library/Makefile.in#53
> 
> What I suggest is rather than this tom-foolery across directories, we
> extract the resources from the built APK file.

We could also interpret ANDROID_RES_DIRS from mobile/android/base/moz.build, but this is rather heavier weight: in this case, I would suggest writing the "geckoview library packaging steps" as a Python build step (a Python script integrated with the build system) that has first class access to the moz.build sandbox.
Or we can build geckoview_libraries/assets in the main build and have fennec depend on those directly
(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #6)
> Or we can build geckoview_libraries/assets in the main build and have fennec
> depend on those directly

Much harder problem: we'd need to separate out the GeckoView code from the Fennec code better (at the moment, GeckoView ships all of Fennec); we'd need to package GeckoView "properly" (as an AAR?); and we'd need to re-write the build system pieces to assemble the two.  Long term, yes.  I have zero cycles to work on this until February but am happy to provide guidance and review.
(In reply to Nick Alexander :nalexander from comment #4)
> So, I think the issue is that the library is no longer included the correct
> resources: see
> 
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/
> geckoview_library/Makefile.in#53
> 
> What I suggest is rather than this tom-foolery across directories, we
> extract the resources from the built APK file.  That is, we do a
> "pseudo-unpack" of the APK and grab whatever res/ files and lib/ files are
> needed from there (oh, and omni.ja).  mcomella, does this make sense?

This makes less sense, actually -- I'm not sure the res/ directories in the APK are text.  So interpreting ANDROID_RES_DIRS is the best idea, which means writing some build system stuff.  Actually, it might be best to move some of this gecko library packaging into mobile/android/base to avoid these shenanigans.  mcomella, can you try to move this building mobile/android/base, using the jar and res dirs lists there?
I'm not too familiar with the build system but I'll be taking a look.
Flags: needinfo?(michael.l.comella)
Status: NEW → ASSIGNED
tracking-fennec: --- → ?
This is a temporary measure.  We have multiple resource directories
that all need to be included into the GeckoView library.  This patch
stuffs the resources into a zip file which can be included in the
GeckoView packaging step.

These resources used to be copied into res/, which led to
synchronization issues.  This new zip file will now suffer the same
synchronization problems.  Android Archive (.aar) files make this
synchronization step explicit, and tools such as gradle do the work to
keep things in sync.  We'll move to use a tool like this eventually,
but for now...
Assignee: michael.l.comella → nalexander
Comment on attachment 8355017 [details] [diff] [review]
Package GeckoView resources when building mobile/android/base.

For the sins of this patch, I apologize.  Testing and feedback appreciated.  Try push:

http://tbpl.mozilla.org/?tree=Try&rev=5910afea47bb
Attachment #8355017 - Flags: review?(mh+mozilla)
Attachment #8355017 - Flags: feedback?(michael.l.comella)
Attachment #8355017 - Flags: feedback?(blassey.bugs)
(In reply to Nick Alexander :nalexander from comment #8)
> (In reply to Nick Alexander :nalexander from comment #4)
> > So, I think the issue is that the library is no longer included the correct
> > resources: see
> > 
> > http://mxr.mozilla.org/mozilla-central/source/mobile/android/
> > geckoview_library/Makefile.in#53
> > 
> > What I suggest is rather than this tom-foolery across directories, we
> > extract the resources from the built APK file.  That is, we do a
> > "pseudo-unpack" of the APK and grab whatever res/ files and lib/ files are
> > needed from there (oh, and omni.ja).  mcomella, does this make sense?
> 
> This makes less sense, actually -- I'm not sure the res/ directories in the
> APK are text.

Just confirming that the XML resources in the APK are indeed packed binary representations, so they can't just be extracted directly.
> http://tbpl.mozilla.org/?tree=Try&rev=5910afea47bb

This fails for a trivial reason.  Building locally, then will push a new try build.
tracking-fennec: ? → 29+
Comment on attachment 8355017 [details] [diff] [review]
Package GeckoView resources when building mobile/android/base.

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

::: mobile/android/base/Makefile.in
@@ +170,5 @@
>  # resource files one subdirectory below the parent resource directory.
> +android_res_files := $(filter-out $(not_android_res_files),$(wildcard $(addsuffix /*,$(wildcard $(addsuffix /*,$(ANDROID_RES_DIRS))))))
> +
> +geckoview_resources.zip: $(android_res_files)
> +	$(foreach dir,$(ANDROID_RES_DIRS),cd $(dir) && zip $(CURDIR)/$@ -r * -x $(not_android_res_files);)

can this be "zip $@ $<"?

::: mobile/android/geckoview_library/Makefile.in
@@ +50,5 @@
>  	# Copy the SOs
>  	cp $(DIST)/bin/libmozglue.so $(DIST)/bin/lib/libplugin-container.so libs/$(ABI_DIR)/
>  
>  	# Copy the resources
> +	$(MAKE) -C $(DEPTH)/mobile/android/base geckoview_resources.zip && \

I think I'd rather export this zip from mobile/android/base
Attachment #8355017 - Flags: feedback?(blassey.bugs) → feedback+
(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #15)
> Comment on attachment 8355017 [details] [diff] [review]
> Package GeckoView resources when building mobile/android/base.
> 
> Review of attachment 8355017 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/Makefile.in
> @@ +170,5 @@
> >  # resource files one subdirectory below the parent resource directory.
> > +android_res_files := $(filter-out $(not_android_res_files),$(wildcard $(addsuffix /*,$(wildcard $(addsuffix /*,$(ANDROID_RES_DIRS))))))
> > +
> > +geckoview_resources.zip: $(android_res_files)
> > +	$(foreach dir,$(ANDROID_RES_DIRS),cd $(dir) && zip $(CURDIR)/$@ -r * -x $(not_android_res_files);)
> 
> can this be "zip $@ $<"?

Unfortunately, no.  These shenanigans create the res/*/*.xml directory structure correctly.  Without them, you get inappropriate paths.

> ::: mobile/android/geckoview_library/Makefile.in
> @@ +50,5 @@
> >  	# Copy the SOs
> >  	cp $(DIST)/bin/libmozglue.so $(DIST)/bin/lib/libplugin-container.so libs/$(ABI_DIR)/
> >  
> >  	# Copy the resources
> > +	$(MAKE) -C $(DEPTH)/mobile/android/base geckoview_resources.zip && \
> 
> I think I'd rather export this zip from mobile/android/base

WFM.  glandium?
(In reply to Nick Alexander :nalexander from comment #16)
> (In reply to Brad Lassey [:blassey] (use needinfo?) from comment #15)

> > can this be "zip $@ $<"?
> 
> Unfortunately, no.  These shenanigans create the res/*/*.xml directory
> structure correctly.  Without them, you get inappropriate paths.

Can you explain a little more? What structure would "zip $@ $<" produce? and conversely what is it that we need?
(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #17)
> (In reply to Nick Alexander :nalexander from comment #16)
> > (In reply to Brad Lassey [:blassey] (use needinfo?) from comment #15)
> 
> > > can this be "zip $@ $<"?
> > 
> > Unfortunately, no.  These shenanigans create the res/*/*.xml directory
> > structure correctly.  Without them, you get inappropriate paths.
> 
> Can you explain a little more? What structure would "zip $@ $<" produce? and
> conversely what is it that we need?

We want to unzip this zip and create a valid res/ directory.  That is, it should unzip to res/drawable/*, res/anim/*, etc.  Or (like I do here) to drawable/*, anim/*, etc.

$< includes entries that are
* absolute paths in the srcdir;
* include mobile/android/base/resources;
* relative paths to $(CURDIR).

zip maintains paths.  So "zip $@ $<" gives an assortment of path names, almost none of which are correct.  The script above takes care to build a zip with useable paths.

Note for the cognoscenti: this is what gradle and other build tools do when using multiple resource directories.
Comment on attachment 8355017 [details] [diff] [review]
Package GeckoView resources when building mobile/android/base.

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

I honestly don't know enough about make to be useful here - re-needinfo me if you want me to take the time to learn anyway. :)
Attachment #8355017 - Flags: feedback?(michael.l.comella)
Comment on attachment 8355017 [details] [diff] [review]
Package GeckoView resources when building mobile/android/base.

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

::: mobile/android/base/Makefile.in
@@ +170,5 @@
>  # resource files one subdirectory below the parent resource directory.
> +android_res_files := $(filter-out $(not_android_res_files),$(wildcard $(addsuffix /*,$(wildcard $(addsuffix /*,$(ANDROID_RES_DIRS))))))
> +
> +geckoview_resources.zip: $(android_res_files)
> +	$(foreach dir,$(ANDROID_RES_DIRS),cd $(dir) && zip $(CURDIR)/$@ -r * -x $(not_android_res_files);)

define foo
$(1)

endef

$(foreach dir,$(ANDROID_RES_DIRS),$(call foo,cd $(dir) && zip $(CURDIR)/$@ -r * -x $(not_android_res_files)))

With a sensible name for "foo" would make logs be more useful in case of errors.

::: mobile/android/geckoview_library/Makefile.in
@@ +30,5 @@
>    $(NULL)
>  
>  include $(topsrcdir)/config/rules.mk
>  
> +$(if $(ABI_DIR),,$(error Missing ABI_DIR))

ifndef ABI_DIR
$(error)
endif

@@ +50,5 @@
>  	# Copy the SOs
>  	cp $(DIST)/bin/libmozglue.so $(DIST)/bin/lib/libplugin-container.so libs/$(ABI_DIR)/
>  
>  	# Copy the resources
> +	$(MAKE) -C $(DEPTH)/mobile/android/base geckoview_resources.zip && \

Those && are useless and make logs less useful in case of errors.

@@ +59,4 @@
>  
>  	# Zip the directory
> +	cd $(DEPTH)/mobile/android && \
> +	$(ZIP) -r ../../dist/geckoview_library/geckoview_library.zip geckoview_library -x geckoview_library/backend.mk geckoview_library/Makefile geckoview_library/*.deps* *.mkdir.done*

Presumably, at this point there shouldn't be any .mkdir.done or .deps in there, since you've excluded them when creating the first zip.
Attachment #8355017 - Flags: review?(mh+mozilla) → feedback+
I compiled GeckoView with this, and all resources all copied, but I get another ResourceNotFoundException

 android.content.res.Resources$NotFoundException: Resource ID #0x7f0a002a
 	at android.content.res.Resources.getValue(Resources.java:1052)
 	at android.content.res.Resources.getDimension(Resources.java:522)
 	at org.mozilla.gecko.ThumbnailHelper.<init>(ThumbnailHelper.java:56)
	at org.mozilla.gecko.ThumbnailHelper.getInstance(ThumbnailHelper.java:41)
	at org.mozilla.gecko.Tab$5.run(Tab.java:671)


It was "R.dimen.tab_thumbnail_width" but I have "tab_thumbnail_width in values/dimens.xml"

How can I fix this?
> It was "R.dimen.tab_thumbnail_width" but I have "tab_thumbnail_width in values/dimens.xml"

Was this in your objdir? i.e. <path-to-m-c>/<objdir>/mobile/android/geckoview_library/res/values/dimens.xml. As it currently stands, the files need to be copied there in order for the application to compile and run correctly.

> How can I fix this?

There is a possible workaround posted in bug 952400 comment 3 if you don't need to change the GeckoView source directly.
(In reply to Michael Comella (:mcomella) from comment #23)
> > It was "R.dimen.tab_thumbnail_width" but I have "tab_thumbnail_width in values/dimens.xml"
> 
> Was this in your objdir? i.e.
> <path-to-m-c>/<objdir>/mobile/android/geckoview_library/res/values/dimens.
> xml. As it currently stands, the files need to be copied there in order for
> the application to compile and run correctly.
> 
> > How can I fix this?
> 
> There is a possible workaround posted in bug 952400 comment 3 if you don't
> need to change the GeckoView source directly.



It was copied correctly. But as values in R.txt (objdir/R.txt) and R.java (GeckoView/gen/R.java) do not match, I think I need a way to set value for gen/R.java
Blocks: 949495
This is a temporary measure.  We have multiple resource directories
that all need to be included into the GeckoView library.  This patch
stuffs the resources into a zip file which can be included in the
GeckoView packaging step.

These resources used to be copied into res/, which led to
synchronization issues.  This new zip file will now suffer the same
synchronization problems.  Android Archive (.aar) files make this
synchronization step explicit, and tools such as gradle do the work to
keep things in sync.  We'll move to use a tool like this eventually,
but for now...
Attachment #8355017 - Attachment is obsolete: true
Comment on attachment 8360693 [details] [diff] [review]
Part 1: Expose MOZ_ANDROID_ABI_DIR to mobile/android.

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

Not sure when defs.mk is slated for removal, but it's as easy to move this to configure then as now.  And defs.mk makes it clear that this is mobile/android only.
Attachment #8360693 - Flags: review?(mh+mozilla)
Comment on attachment 8360691 [details] [diff] [review]
Part 2: Package GeckoView resources when building mobile/android/base.

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

The def for the zip command doesn't do what you'd hope: the foreach just builds one long command that gets executed by the shell.  I did it anyway since the code is clearer that way.
Attachment #8360691 - Flags: review?(mh+mozilla)
This is a temporary measure.  We have multiple resource directories
that all need to be included into the GeckoView library.  This patch
stuffs the resources into a zip file which can be included in the
GeckoView packaging step.

These resources used to be copied into res/, which led to
synchronization issues.  This new zip file will now suffer the same
synchronization problems.  Android Archive (.aar) files make this
synchronization step explicit, and tools such as gradle do the work to
keep things in sync.  We'll move to use a tool like this eventually,
but for now...
Attachment #8360691 - Attachment is obsolete: true
Attachment #8360691 - Flags: review?(mh+mozilla)
Comment on attachment 8360725 [details] [diff] [review]
Part 2: Package GeckoView resources when building mobile/android/base.

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

Ah... the newline at the end matters :)  Thanks!
Attachment #8360725 - Flags: review?(mh+mozilla)
This is a temporary measure.  We have multiple resource directories
that all need to be included into the GeckoView library.  This patch
stuffs the resources into a zip file which can be included in the
GeckoView packaging step.

These resources used to be copied into res/, which led to
synchronization issues.  This new zip file will now suffer the same
synchronization problems.  Android Archive (.aar) files make this
synchronization step explicit, and tools such as gradle do the work to
keep things in sync.  We'll move to use a tool like this eventually,
but for now...
Attachment #8361173 - Flags: review?(mh+mozilla)
Attachment #8360725 - Attachment is obsolete: true
Attachment #8360725 - Flags: review?(mh+mozilla)
glandium: the trailing && \ matter because otherwise each command executes independently and doesn't have the correct working directory.  Forgot to test that part.  This works locally.
Comment on attachment 8360693 [details] [diff] [review]
Part 1: Expose MOZ_ANDROID_ABI_DIR to mobile/android.

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

::: mobile/android/defs.mk
@@ +26,5 @@
> +# packager as well (via mobile/android/installer/Makefile.in.
> +ifeq ($(CPU_ARCH),x86)
> +MOZ_ANDROID_ABI_DIR = x86
> +else
> +ifdef MOZ_THUMB2

This is the wrong test, but it doesn't matter anyway because we already have ANDROID_CPU_ARCH with the right values.

@@ +31,5 @@
> +MOZ_ANDROID_ABI_DIR = armeabi-v7a
> +else
> +MOZ_ANDROID_ABI_DIR = armeabi
> +endif
> +endif

We already have ANDROID_CPU_ARCH with those values.

::: toolkit/mozapps/installer/packager.mk
@@ -355,5 @@
> -ABI_DIR = armeabi-v7a
> -else
> -ABI_DIR = armeabi
> -endif
> -endif

And yes, this means this was wrong.
Attachment #8360693 - Flags: review?(mh+mozilla) → review-
Comment on attachment 8361173 [details] [diff] [review]
Part 2: Package GeckoView resources when building mobile/android/base.

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

::: mobile/android/geckoview_library/Makefile.in
@@ +36,3 @@
>  package: $(properties_deps)
>  	# Make directory for the zips
> +	$(MKDIR) -p $(_ABS_DIST)/geckoview_library

why do you need an absolute path to DIST in this file?
Attachment #8361173 - Flags: review?(mh+mozilla) → review+
(In reply to Nick Alexander :nalexander from comment #32)
> glandium: the trailing && \ matter because otherwise each command executes
> independently and doesn't have the correct working directory.

Note I wasn't talking about the ones you left, but about those you removed :)
(In reply to Mike Hommey [:glandium] from comment #35)
> Comment on attachment 8361173 [details] [diff] [review]
> Part 2: Package GeckoView resources when building mobile/android/base.
> 
> Review of attachment 8361173 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/geckoview_library/Makefile.in
> @@ +36,3 @@
> >  package: $(properties_deps)
> >  	# Make directory for the zips
> > +	$(MKDIR) -p $(_ABS_DIST)/geckoview_library
> 
> why do you need an absolute path to DIST in this file?

Technically, no, but it's nice to be consistent.
This is a temporary measure.  We have multiple resource directories
that all need to be included into the GeckoView library.  This patch
stuffs the resources into a zip file which can be included in the
GeckoView packaging step.

These resources used to be copied into res/, which led to
synchronization issues.  This new zip file will now suffer the same
synchronization problems.  Android Archive (.aar) files make this
synchronization step explicit, and tools such as gradle do the work to
keep things in sync.  We'll move to use a tool like this eventually,
but for now...
Attachment #8361173 - Attachment is obsolete: true
Comment on attachment 8368897 [details] [diff] [review]
Part 2: Package GeckoView resources when building mobile/android/base.

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

This is just a rebase; carrying forward glandium's r+.
Attachment #8368897 - Flags: review+
Attachment #8360693 - Attachment is obsolete: true
Attachment #8368898 - Attachment is obsolete: true
Comment on attachment 8368903 [details] [diff] [review]
Part 1: Replace ABI_DIR with ANDROID_CPU_ARCH.

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

Sorry for the delay, glandium.  Here's a version that uses ANDROID_CPU_ARCH.
Attachment #8368903 - Flags: review?(mh+mozilla)
Attachment #8368903 - Flags: review?(mh+mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/7cb8cbe2343f
https://hg.mozilla.org/mozilla-central/rev/b39330088023
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
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: