969725 - geckoview_example uses incorrect main.xml layout and AndroidManifest

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

`<objdir>/embedding/android/geckoview_example/res/layout/main.xml` appears to be auto-generated by the ant build process rather than the one copied from the associated source directory (which notably uses GeckoView ;).

Comment 1

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Turns out `AndroidManifest.xml` never gets copied either. Perhaps this is some quirky behavior of NSINSTALL that prevents it from ovewritting files?

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Note that AndroidManifest.xml is also auto-generated by `android create project`.

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

With the build process:

`mach build && mach package && make package -C <objdir>/embedding/android/geckoview_example` 

This no longer seems to be an issue. According to blassey, the build process should be streamlined in bug 971101.

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Okay, nevermind - I `cat` the wrong file. This still seems to be an issue for me. blassey reports no issues - perhaps this is platform dependant? I'm compiling on Linux.

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Found by rnewman: Prerequisite `/Users/rnewman/moz/hg/fx-team/embedding/android/geckoview_example/AndroidManifest.xml' is older than target `AndroidManifest.xml'.

Explanation: `android create project` creates these default files, make sees that the files it depends on are newer than the source files, and does not run these targets so the source files are never copied.

Looking for a fix using phony targets.

Comment 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Remove default android project files in geckoview_example build.

I threw in an `rm` instead of using phony targets because they seemed to be
designed for a different purpose (i.e. files of the same name).

Also, make would fail with a circular dependency on AndroidManifest.xml if I
left it as AndroidManifest.xml because it would use the source dir instead of
the build dir in the target (and I have no idea why). Thus, I made the path
explicit. Seeing how I don't think anyone will be building this target by hand,
I think it will be alright (but yes, it's terribly hacky).

Comment 7

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 8377940 [details] [diff] [review]
Remove default android project files in geckoview_example build.

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

::: embedding/android/geckoview_example/Makefile.in
@@ +26,5 @@
>  
>  PACKAGE_DEPS = \
>  	build.xml \
>  	res/layout/main.xml \
> +	$(relativesrcdir)/AndroidManifest.xml \

why do you need $(relativesrcdir)? will $(srcdir) work?

@@ +40,4 @@
>  res/layout/main.xml: $(srcdir)/main.xml
>  	$(NSINSTALL) $(srcdir)/main.xml res/layout/
>  
> +$(relativesrcdir)/AndroidManifest.xml: $(srcdir)/AndroidManifest.xml

same question here. Shouldn't $(relativesrcdir) be the same directory as $(srcdir)? I don't see $(relativesrcdir) being used much outside of tests in other makefiles, so I'm wary of it.

Comment 8

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Disclaimer: I don't really know make all that well.

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #7)
> why do you need $(relativesrcdir)? will $(srcdir) work?

Sorry, I just realized that it was a brain fart: for some reason, I thought `relativesrcdir` referred to the objdir. The problem I was trying to solve was: 

make: Circular /home/mcomella/dev/moz/embedding/android/geckoview_example/AndroidManifest.xml <- /home/mcomella/dev/moz/embedding/android/geckoview_example/AndroidManifest.xml dependency dropped.

This occurs when calling the target "AndroidManifest.xml" and I have no idea why because it correctly (at least to my understanding of make) refers to the objdir without my `rm` changes.

In the output, notably:

Considering target file '/home/mcomella/dev/moz/embedding/android/geckoview_example/AndroidManifest.xml'.

As opposed to:

Considering target file 'src/org/mozilla/geckoviewexample/GeckoViewExample.java'.

main.xml uses the full path to the srcdir like AndroidManifest - I'm not sure why that works. My brain fart solved the problem because relativesrcdir, `embedding/android/geckoview_example/` does not exist in the objdir. Thus, when using srcdir, an absolute path, the circular dependency returns.

Unless you know why it's using the absolute srcdir path, I know how to fix it a few ways:
  * Phony target (so it will run unconditionally) that installs all of the files (should I include GeckoViewExample and assets too?)
  * Do the installation at the end of the build.xml target

Comment 9

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Michael Comella (:mcomella) from comment #8)
> My brain fart solved the problem because relativesrcdir,
> `embedding/android/geckoview_example/` does not exist in the objdir.

e.g. <objdir>/embedding/android/geckoview_example/embedding/android/geckoview_example/

Comment 10

[Brad Lassey [:blassey] (use needinfo?)]

This may be why it was working for me and not for you ("this" being that you have some kind of broken make). My understanding is that:

AndroidManifest.xml: $(srcdir)/AndroidManifest.xml

Should translate to $(objdir)/AndroidManifest.xml depending on $(srcdir)/AndroidManifest.xml

But, given that people build on all kinds of systems and this is obviously potentially confusing, may I suggest either

$(objdir)/AndroidManifest.xml: $(srcdir)/AndroidManifest.xml

   or

AndroidManifest.xml: AndroidManifest.xml.in

   and rename the AndroidManifest.xml that we're copying in to AndroidManifest.xml.in

Comment 11

[Nick Alexander :nalexander [he/him]]

Attachment: WIP toward better dependencies.

This is the output of some collaborative hacking with mcomella.

Comment 12

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Rework geckoview_example makefile for better deps.

Comment 13

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: GeckoView_deps.patch

Comment 14

[Brad Lassey [:blassey] (use needinfo?)]

note, I added FORCE on the package target for geckoview_library and it's unpacking target in geckoview_example such that we always pick up changes to GeckoView. This is certainly needed in geckoview_library. I don't see why it is needed in geckoview_example since it appears to appropriately depends on the zip file, but it seems it is needed.

Comment 15

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Cleaned up nalexander's patch in comment 12 but there are runtime errors (likely bug 977677). Notable changes from the first file:
  * Spaces in lists, tabs in commands
  * Used INSTALL_TARGETS to move the various files
  * Built out corresponding src dirs in the tree (for use with INSTALL_TARGETS)
  * libxul.so target had an error where the unpacked files would be older than the zip because timestamps were restored
  * Move rules.mk inclusion to the bottom of the file (since it should be processed after items like INSTALL_TARGETS are defined)

The patch still need some comments cleaned up and to figure out the `echo ... lib ...` issue (is that necessary, what does that do?). 

I don't really know make well enough to claim which patch between comment 12 and comment 14 (blassey's patch) is preferred but since nalexander and I worked together for the patch in comment 12, it's likely blassey's call.

Comment 16

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8383135 [details] [diff] [review]
GeckoView_deps.patch

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

I think that FORCE is not a substitute for understanding what the correct dependencies are, but this is already in |make package|, where deps go to die.

Comment 17

[Nick Alexander :nalexander [he/him]]

I should add that this is almost certainly going to make it look like GeckoView example is working, when in fact the packaging is busted in a way that "real" consumers of GeckoView (those with lots of Android resources) will see and a trivial geckoview_example will not.  I have explained why in https://bugzilla.mozilla.org/show_bug.cgi?id=977677#c5.

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/aa9ca1a3dbdf