1169421 - Switch Fennec to use ch.boye instead of org.apache.http to allow for building with Android M SDK 23

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: build-23.log

I get the attached build failure when trying to build against the Android M preview SDK (after fixing a couple new warnings). Looks like it can't find the apache stuff for some reason.

Comment 1

[James Willcox (:snorp) ([e-mail]) (he/him)]

Looks like the removed the apache http client in the SDK entirely. It's available as a separate jar, so we could do that. We apparently already include ch.boye, though, so we could also just change all the call sites to use that.

Comment 2

[amoghbl1]

Moving to ch.boye entirely ? I'll take this up.

Comment 3

[amoghbl1]

This switch would involve replacing the android.net.http.AndroidHttpClient with ch.boye.httpclientandroidlib.impl.client.DefaultHttpClient .
The current call for a httpClient is (from LoadFaviconTask.java)
> static AndroidHttpClient httpClient = AndroidHttpClient.newInstance(GeckoAppShell.getGeckoInterface().getDefaultUAString());

This would need to be replaced with (from DoCommand.java)
> HttpClient httpClient = new DefaultHttpClient();

Comment 4

[amoghbl1]

Attachment: bug_1169421.patch

Changes the http client library used for two files
1) mobile/android/base/distribution/Distribution.java
2) mobile/android/base/favicons/LoadFaviconTask.java
Also changes the AndroidHttpClient to DefaultHttpClient for the second class.

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8623308 [details] [diff] [review]
bug_1169421.patch

>diff --git a/mobile/android/base/favicons/LoadFaviconTask.java b/mobile/android/base/favicons/LoadFaviconTask.java

>-    static AndroidHttpClient httpClient = AndroidHttpClient.newInstance(GeckoAppShell.getGeckoInterface().getDefaultUAString());
>+    static DefaultHttpClient httpClient = new DefaultHttpClient();

What about the UA?

I think we'll need to add a line here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/favicons/LoadFaviconTask.java#131

Something like:
request.setHeader("User-Agent", GeckoAppShell.getGeckoInterface().getDefaultUAString());

Other global approaches are here:
http://stackoverflow.com/questions/5027309/how-to-set-custom-user-agent-with-apache-http-client-library-4-1

Comment 6

[amoghbl1]

Attachment: bug_1169421.patch

User Agent header added to request.

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8624445 [details] [diff] [review]
bug_1169421.patch

You have a bunch of file deletes in this patch which I don't think we want :)

Comment 8

[amoghbl1]

Attachment: fix_1169421.patch

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8625033 [details] [diff] [review]
fix_1169421.patch

Looks OK

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

Is anything stopping us from landing this? I'll push the patch to try.

Comment 11

[Sebastian Kaspari (:sebastian; :pocmo)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=470de7bc28a1

Comment 12

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to :Sebastian Kaspari from comment #11)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=470de7bc28a1

The build(s) failed :(

Comment 13

[amoghbl1]

Oops, sorry about that, I've made a mistake in the import.
Will fix now!

Comment 14

[amoghbl1]

Attachment: fix_1169421.patch

Comment 15

[Sebastian Kaspari (:sebastian; :pocmo)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc76725b63ec

Comment 16

[Sebastian Kaspari (:sebastian; :pocmo)]

Thank you for the updated patch! I just pushed it to try. :)

Comment 17

[Nick Alexander :nalexander [he/him]]

amoghbl1: is there a reason you're not pushing to try yourself?  (Other than too busy :)  I see I vouched for you in Bug 1039210.

Comment 18

[amoghbl1]

nalexander: Shall do it for the next patch! I still need to read up on how to use try.

Comment 19

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to amoghbl1 from comment #18)
> nalexander: Shall do it for the next patch! I still need to read up on how
> to use try.

I'm not nalexander but yeah, whenever you think your patch is ready, push it to try to see if there are any regressions or other things you missed.

You can find more information about the try server here: https://wiki.mozilla.org/ReleaseEngineering/TryServer Note that this page is very "patch queue" (mq) heavy - so this can be confusing if it's not your primary work flow. This TryChooser syntax builder can be helpful when creating a commit to push to try: http://trychooser.pub.build.mozilla.org/ (For most Android stuff: Select "both" build types, all three Android platforms and check all robocop tests)

Seems like the last try run was sucessful. I've setup a build system using the M preview SDK locally and will try to build with your patch applied to see if we have missed anything.

Comment 20

[Sebastian Kaspari (:sebastian; :pocmo)]

Yep! All HttpClient related build errors are gone. Thank you! :)

Comment 21

[Sebastian Kaspari (:sebastian; :pocmo)]

amoghbl1: I guess this patch is ready to land. Just update your commit message to include the reviewer "r=mfinkle" and remove the "[mq]: " prefix. Then we should be ready to add the "checkin-needed" keyword. :)

Comment 22

[amoghbl1]

(In reply to :Sebastian Kaspari from comment #21)
> amoghbl1: I guess this patch is ready to land. Just update your commit
> message to include the reviewer "r=mfinkle" and remove the "[mq]: " prefix.
> Then we should be ready to add the "checkin-needed" keyword. :)

Wow, I forgot about this one! Updating now.

Comment 23

[amoghbl1]

Attachment: fix_1169421.patch

Updated the commit message.

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/7b48142f4120

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7b48142f4120