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

RESOLVED FIXED in Firefox 42

Status

()

P1
normal
RESOLVED FIXED
4 years ago
a year ago

People

(Reporter: snorp, Assigned: amoghbl1)

Tracking

(Blocks: 2 bugs)

Trunk
Firefox 42
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 fixed)

Details

(Whiteboard: [tor-mobile])

Attachments

(2 attachments, 4 obsolete attachments)

Created attachment 8612527 [details]
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.
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.
(Assignee)

Comment 2

4 years ago
Moving to ch.boye entirely ? I'll take this up.
Assignee: nobody → amoghbl1
Status: NEW → ASSIGNED
Summary: Build fails with Android M SDK 23 → Switch Fennec to use ch.boye instead of org.apache.http to allow for building with Android M SDK 23
Version: Firefox 34 → Trunk
(Assignee)

Comment 3

4 years ago
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();
Flags: needinfo?(snorp)
(Assignee)

Comment 4

4 years ago
Created attachment 8623308 [details] [diff] [review]
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.
Attachment #8623308 - Flags: review?(mark.finkle)
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
Attachment #8623308 - Flags: review?(mark.finkle) → review-
(Assignee)

Comment 6

3 years ago
Created attachment 8624445 [details] [diff] [review]
bug_1169421.patch

User Agent header added to request.
Attachment #8623308 - Attachment is obsolete: true
Attachment #8624445 - Flags: review?(mark.finkle)
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 :)
Attachment #8624445 - Flags: review?(mark.finkle) → review-
(Assignee)

Comment 8

3 years ago
Created attachment 8625033 [details] [diff] [review]
fix_1169421.patch
Attachment #8624445 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8625033 - Flags: review?(mark.finkle)
Comment on attachment 8625033 [details] [diff] [review]
fix_1169421.patch

Looks OK
Attachment #8625033 - Flags: review?(mark.finkle) → review+
Is anything stopping us from landing this? I'll push the patch to try.
Flags: needinfo?(amoghbl1)
(In reply to :Sebastian Kaspari from comment #11)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=470de7bc28a1

The build(s) failed :(
(Assignee)

Comment 13

3 years ago
Oops, sorry about that, I've made a mistake in the import.
Will fix now!
Flags: needinfo?(amoghbl1)
(Assignee)

Comment 14

3 years ago
Created attachment 8632874 [details] [diff] [review]
fix_1169421.patch
Attachment #8625033 - Attachment is obsolete: true
Thank you for the updated patch! I just pushed it to try. :)
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.
Flags: needinfo?(amoghbl1)
(Assignee)

Comment 18

3 years ago
nalexander: Shall do it for the next patch! I still need to read up on how to use try.
Flags: needinfo?(amoghbl1)
(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.
Yep! All HttpClient related build errors are gone. Thank you! :)
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. :)
Flags: needinfo?(amoghbl1)
(Assignee)

Comment 22

3 years ago
(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.
Flags: needinfo?(amoghbl1)
(Assignee)

Comment 23

3 years ago
Created attachment 8636955 [details] [diff] [review]
fix_1169421.patch

Updated the commit message.
Attachment #8632874 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7b48142f4120
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Flags: needinfo?(snorp)
See Also: → bug 1208587
See Also: → bug 765064

Updated

2 years ago
Whiteboard: [tor-mobile]

Updated

2 years ago
Blocks: 1357994

Updated

a year ago
Priority: -- → P1
You need to log in before you can comment on or make changes to this bug.