930062 - GeckoView library should support building as an ant project

Status: RESOLVED FIXED

(Firefox for Android :: General, defect)

Description

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

Attachment: geckoview_ant_support.patch

Comment 1

[Nick Alexander :nalexander [he/him]]

Comment on attachment 821074 [details] [diff] [review]
geckoview_ant_support.patch

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

This doesn't seem ready for review.

::: mobile/android/geckoview_library/Makefile.in
@@ +9,4 @@
>    .project \
>    AndroidManifest.xml \
>    project.properties \
> +  build.xml \

This file doesn't exist in the tree or the patch.

@@ +13,3 @@
>    $(NULL)
>  
> +GECKOVIEW_LIBRARY_PP_FILES = local.properties

ditto.

@@ +30,3 @@
>  include $(topsrcdir)/config/rules.mk
>  
> +$(GECKOVIEW_LIBRARY_PP_FILES): % : %.in

PP_TARGETS, please.

Comment 2

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

Attachment: geckoview_ant_support.patch

Comment 3

[Nick Alexander :nalexander [he/him]]

Comment on attachment 821351 [details] [diff] [review]
geckoview_ant_support.patch

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

r+ after comments are addressed.

::: mobile/android/geckoview_library/AndroidManifest.xml
@@ +4,5 @@
>      android:versionName="1.0" >
>  
> +    <uses-permission android:name="android.permission.ACCESS_NETWORK_STATE"/>
> +    <uses-permission android:name="android.permission.INTERNET"/>
> +<!--    <uses-permission android:name="android.permission.GET_ACCOUNTS"/>

Either delete these lines, or explain why they are commented.

::: mobile/android/geckoview_library/Makefile.in
@@ +15,5 @@
> +
> +PP_TARGETS = properties
> +
> +properties = local.properties.in
> +proerties_PATH = .

nit: typo.

@@ +18,5 @@
> +properties = local.properties.in
> +proerties_PATH = .
> +
> +
> +GARBAGE = $(GECKOVIEW_LIBRARY_FILES) $(PP_TARGETS)

$(PP_TARGETS) doesn't do what you want.  Unfortunately this is lame.  Try:

properties_deps := $(patsubst %.in,%,$(properties))

and depend on $(properties_deps).

@@ +21,5 @@
> +
> +GARBAGE = $(GECKOVIEW_LIBRARY_FILES) $(PP_TARGETS)
> +
> +GARBAGE_DIRS = \
> +         bin   \

nit: two spaces leading space, no spaces before \.

@@ +35,3 @@
>  include $(topsrcdir)/config/rules.mk
>  
> +package: local.properties

$(properties_deps)

::: mobile/android/geckoview_library/local.properties.in
@@ +2,5 @@
> +# This file is automatically generated by Android Tools.
> +# Do not modify this file -- YOUR CHANGES WILL BE ERASED!
> +#
> +# This file must *NOT* be checked into Version Control Systems,
> +# as it contains information specific to your local configuration.

Given the warning, this seems wrong.  But I don't know ANT.

Comment 4

[Gregory Szorc [:gps]]

Comment on attachment 821351 [details] [diff] [review]
geckoview_ant_support.patch

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

I'm deferring review to nalexander. He's an Android build peer and has more context than I do regarding the future of Android's build system and how this interacts with it.

::: mobile/android/geckoview_library/build.xml
@@ +1,1 @@
> +<?xml version="1.0" encoding="UTF-8"?>

Do we need a license header?

Comment 5

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

https://hg.mozilla.org/mozilla-central/rev/9bb77ef52ef8