Closed Bug 969725 Opened 7 years ago Closed 7 years ago

geckoview_example uses incorrect main.xml layout and AndroidManifest

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 30

People

(Reporter: mcomella, Assigned: blassey)

References

Details

Attachments

(3 files, 1 obsolete file)

`<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 ;).
Turns out `AndroidManifest.xml` never gets copied either. Perhaps this is some quirky behavior of NSINSTALL that prevents it from ovewritting files?
Summary: geckoview_example uses incorrect main.xml layout → geckoview_example uses incorrect main.xml layout and AndroidManifest
Note that AndroidManifest.xml is also auto-generated by `android create project`.
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.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WORKSFORME
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.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
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.
Assignee: nobody → michael.l.comella
Status: REOPENED → ASSIGNED
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).
Attachment #8377940 - Flags: review?(blassey.bugs)
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.
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
(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/
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
Attachment #8377940 - Flags: review?(blassey.bugs) → review-
Attached patch WIP toward better dependencies. (obsolete) — Splinter Review
This is the output of some collaborative hacking with mcomella.
Attachment #8379228 - Attachment is obsolete: true
Assignee: michael.l.comella → blassey.bugs
Attachment #8383135 - Flags: review?(nalexander)
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.
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.
Attachment #8383135 - Flags: review?(nalexander) → review?(mh+mozilla)
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.
Attachment #8383135 - Flags: feedback-
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.
Attachment #8383135 - Flags: review?(mh+mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/aa9ca1a3dbdf
Status: ASSIGNED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
You need to log in before you can comment on or make changes to this bug.