Closed Bug 930062 Opened 7 years ago Closed 7 years ago

GeckoView library should support building as an ant project

Categories

(Firefox for Android :: General, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 28

People

(Reporter: blassey, Assigned: blassey)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch geckoview_ant_support.patch (obsolete) — Splinter Review
No description provided.
Attachment #821074 - Flags: review?(nalexander)
Attachment #821074 - Flags: review?(khuey)
Blocks: 930072
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.
Attachment #821074 - Flags: review?(nalexander) → review-
Assignee: nobody → blassey.bugs
Attachment #821074 - Attachment is obsolete: true
Attachment #821074 - Flags: review?(khuey)
Attachment #821351 - Flags: review?(nalexander)
Attachment #821351 - Flags: review?(khuey)
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.
Attachment #821351 - Flags: review?(nalexander) → review+
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?
Attachment #821351 - Flags: review?(gps)
https://hg.mozilla.org/mozilla-central/rev/9bb77ef52ef8
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 28
You need to log in before you can comment on or make changes to this bug.