Opened 4 weeks ago

Closed 3 weeks ago

Last modified 2 weeks ago

#28468 closed defect (fixed)

Modify Android toolchain to support Orbot

Reported by: sisbell Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-rbm, tbb-mobile, TBA-a2, TorBrowserTeam201811R
Cc: sisbell Actual Points:
Parent ID: #26693 Points:
Reviewer: Sponsor: Sponsor8

Description


Child Tickets

Change History (11)

comment:1 Changed 4 weeks ago by sisbell

Status: newneeds_review

Changes (android-1115)

  • Changed NDK version used from 26 to 16 (this is only for building of rust)

comment:2 Changed 4 weeks ago by gk

Keywords: TorBrowserTeam201811R added; TorBrowserTeam201811 removed

comment:3 in reply to:  1 ; Changed 4 weeks ago by gk

Keywords: TorBrowserTeam201811 added; TorBrowserTeam201811R removed
Status: needs_reviewneeds_revision

Replying to sisbell:

Changes (android-1115)

  • Changed NDK version used from 26 to 16 (this is only for building of rust)

You mean this is for Orbot, right? Because that's what the commit message says and there is no Rust involvement into building Orbot. So, I think commit message needs to get updated at least.

What speaks against hardcoding "16" as you did before and just adding a comment in the build script explaining the situation and what we are doing?

You wrote "Changed NDK version used from 26 to 16" but that's confusing to me. We are using NDK *r15c* for our build so far (which is why we are downloading only that one in the first place) so far as that is what Mozilla's Fennec is using and SDK 26. So, we weren't previously using *NDK* 26. Do you mean "SDK" here? (Looking at the NDK download page r18b is the highest stable you get)

comment:4 Changed 4 weeks ago by gk

Okay, I chatted a bit with sysrqb and am still digging through all the different concepts (that seem to get used differently sometimes, too). So, could you say why exactly we did need to go back from --api 26 to --api 16.

And it seems the toolchain Mozilla uses is defining -D__ANDROID_API__=9 while the make_standalone_toolchain.py script defines -D__ANDROID_API__=$api-level with api-level being the one passed on with --api.

Interestingly enough, passing --api 9 to the python script results in an error: 9 is less than minimum platform for arm (14). I wonder whether that's a shortcoming of that script which means that we actually can't assemble a similar toolchain to the one Mozilla uses?

comment:5 in reply to:  4 Changed 4 weeks ago by sisbell

Replying to gk:

Okay, I chatted a bit with sysrqb and am still digging through all the different concepts (that seem to get used differently sometimes, too). So, could you say why exactly we did need to go back from --api 26 to --api 16.

My preference is to default to higher version unless something has been deprecated or something bad shows up in runtime testing. I changed back to 16 since from my understanding of the discussions that we should be more cautious and use what firefox is using. I see valid points for this as well. Honestly, I think we are ok with either version since we aren't using the more recent features in the NDK.

And it seems the toolchain Mozilla uses is defining -D__ANDROID_API__=9 while the make_standalone_toolchain.py script defines -D__ANDROID_API__=$api-level with api-level being the one passed on with --api.

Interestingly enough, passing --api 9 to the python script results in an error: 9 is less than minimum platform for arm (14). I wonder whether that's a shortcoming of that script which means that we actually can't assemble a similar toolchain to the one Mozilla uses?

We can build firefox code with api 9 but yes it is a red flag that the toolchain script refuses to do this (I assume google is not being arbitrary on this). I don't think Mozilla is following best practices in all of this (partly for historical and legacy reasons).

comment:6 in reply to:  3 Changed 4 weeks ago by sisbell

Replying to gk:

Replying to sisbell:

Changes (android-1115)

  • Changed NDK version used from 26 to 16 (this is only for building of rust)

You mean this is for Orbot, right? Because that's what the commit message says and there is no Rust involvement into building Orbot. So, I think commit message needs to get updated at least.

You are right the api version has nothing to do with Orbot, only the new export variables in setup are for Orbot. I'll break apart the version issue into a separate ticket.

What speaks against hardcoding "16" as you did before and just adding a comment in the build script explaining the situation and what we are doing?

I just added the var/standalone_ndk to make it more explicit. I'm ok either way, your call.

comment:7 Changed 4 weeks ago by gk

Just hardcoding it as we have it right now is okay.

comment:8 Changed 4 weeks ago by sisbell

Changes (android-1116)

  • hardcode api 16 (this is done in #28483)
  • Adds exports for Orbot

comment:9 Changed 3 weeks ago by sisbell

Status: needs_revisionneeds_review

comment:10 Changed 3 weeks ago by gk

Keywords: TorBrowserTeam201811R added; TorBrowserTeam201811 removed
Resolution: fixed
Status: needs_reviewclosed

Cherry-picked to master (commit 5e3b6403e384198f0f85c482777970340333bc5a), thanks.

comment:11 Changed 2 weeks ago by pili

Sponsor: Sponsor8
Note: See TracTickets for help on using tickets.