Opened 13 months ago

Last modified 5 months ago

#31882 accepted defect

move Android build config into core tor

Reported by: eighthave Owned by: nickm
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Normal Keywords: Android, tbb-mobile, 043-deferred, 044-deferred
Cc: gk, sysrqb, sisbell Actual Points:
Parent ID: Points:
Reviewer: nickm Sponsor:

Description

Guardian Project has maintained a wrapper build system for building tor for Android since the beginning. Since many more people are now building tor for Android, I'm working to upstream as much of that as possible. The goal is that the requirements for building on Android can be fulfilled by tor's own configure.ac.

The first change is to remove the unnecessary ./configure --enable-android option.

  • Android NDK compilers define __ANDROID__ so that can be used for Android-only code without adding anything in configure.ac, (following the standard pattern like __APPLE__, __linux__, __FreeBSD__, _WIN32, etc.
  • android/log.h and __android_log_write() are always present if building for Android, so this would be like testing for printf() on UNIX.

Then move the Android config into configure.ac reusing the ./configure --enable-android flag. A working sketch for that is here:
https://trac.torproject.org/projects/tor/ticket/28766#comment:6

The last piece is including a Java JNI wrapper in tor, enabled by ./configure --enable-jni. This lets us run tor as an Android Service, which is the Android equivalent of a UNIX daemon (UNIX daemons are not really supported on Android and using them is quite problematic). This JNI API should be generic enough to be useable in Java in general, though that's not a priority for us.

I'll add patches for merging once we agree on these details. I have this all working already on my machine.

Child Tickets

Change History (35)

comment:1 Changed 13 months ago by nickm

Milestone: Tor: 0.4.3.x-final

This seems like a plausible line of work to me. The 0.4.2.x series is in feature-freeze, so this would have to go in 0.4.3.

On the JNI wrapper: is it possible to implement this using (and mirroring) the API in tor-api.h? I'd like to keep the number of in-process interfaces that we expose as low as possible.

comment:2 Changed 13 months ago by nickm

Also, for long-term stability, it would be valuable to have a CI process that tests our android builds. I wonder how hard it would be for us to get one.

comment:3 Changed 13 months ago by eighthave

Looks like the JNI wrapper can be entirely based on what's in tor_api.h. You can get an idea now of what this needs, it uses only the pieces in the current src/feature/api/tor_api.[ch]. This is based on @sysrqb's work in #26653, e.g:
https://gitweb.torproject.org/user/sysrqb/tor.git/commit/?h=testing_26653&id=731b7e2b176f7cf6439c1286886492998e58cd7f

My big question is now to add a new src/feature/jni section and still have its symbols exported. When I add my files to src/feature/jni, it builds fine, but then its missing the exported JNI functions. The symbols make it into the libtor-app.a but not the libtor.so. My hack for now is to just add the JNI code to src/feature/api/tor_api.[ch], that works. The idea is to have ./configure --enable-jni. Here's that in progress code with the autotools changes:
https://gitlab.com/eighthave/tor/commit/a453e9faf3e4be946506111754b18f9fcf1ac595

I already have a working Android CI build in gitlab-ci (plus I started porting the .travis.yml to .gitlab-ci.yml while I was at it and threw in a couple of distro builds). It should be relatively straightforward to port the Android CI job .travis.yml for someone who know that whole matrix setup.
https://gitlab.com/eighthave/tor/blob/dbeb6f7d8/.gitlab-ci.yml
And here's an example run on the latest master:
https://gitlab.com/eighthave/tor/pipelines/85003730

Last edited 13 months ago by eighthave (previous) (diff)

comment:4 Changed 13 months ago by nickm

I'd suggest src/app/jni instead of src/feature/jni: This is a top-level entry point into the Tor source code, so it belongs an app.

What code is responsible for making "libtor.so" ? As far as I know, that is some extension to Tor's build system that we do not currently maintain. It would be good to import that code into our build system as well, as a part of doing this.

comment:5 Changed 13 months ago by eighthave

Ok, as long as it can be optionally enabled and disabled, I'm fine with putting it wherever. For example, I think that this JNI code should only be included when someone specifies ./configure --enable-jni. That should be separate from ./configure --enable-android since some people might want to still build the tor daemon for Android for testing purposes.

As for making libtor.so, that's ./configure --enable-pic. Then src/app/tor is renamed to libtor.so. I think ./configure --enable-pic should change the resulting filename to libtor.so.

comment:6 Changed 13 months ago by nickm

Whoa, that's not how you're supposed to make an .so file, is it? I thought you were supposed to use a specific set of linker options in order to make a .so.

comment:7 Changed 13 months ago by eighthave

lol, yeah, not by design. I just tried ./configure --enable-pic and it works as a .so

comment:8 in reply to:  3 Changed 13 months ago by teor

Replying to eighthave:

...

I already have a working Android CI build in gitlab-ci (plus I started porting the .travis.yml to .gitlab-ci.yml while I was at it and threw in a couple of distro builds). It should be relatively straightforward to port the Android CI job .travis.yml for someone who know that whole matrix setup.
https://gitlab.com/eighthave/tor/blob/dbeb6f7d8/.gitlab-ci.yml
And here's an example run on the latest master:
https://gitlab.com/eighthave/tor/pipelines/85003730

I'm currently rewriting and simplifying our build matrix in #30860. It might be easier to port the .travis.yml after I'm done.

comment:9 Changed 13 months ago by eighthave

Also, FYI, OnionBrowser on iOS has been building and using tor like this for a while now.

comment:10 Changed 13 months ago by eighthave

Attached is the patch series to implement this all on top of master. 0003-Android-s-logging-mechanism-is-called-logcat.patch and the gitlab-ci patches could be omitted if it delays merging the first 2.

In order to make sure I didn't mess up ./configure on other platforms, I added tests to .gitlab-ci.yml for lots of GNU/Linux distros (the MinGW job isn't quite working yet, so ignore its failure). You can see a full run here:
https://gitlab.com/eighthave/tor/pipelines/87935658

I'm happy to submit the rest of the .gitlab-ci.yml changes as desired. You can see them in my gitlab fork.

comment:11 Changed 13 months ago by eighthave

Status: newneeds_review

comment:12 Changed 13 months ago by eighthave

The last piece, which is not implemented in the patches, is making the output filename be libtor.so. There is currently no support for different output file names in Makefile.am so it would be non-trivial to implement, and I'm not sure the approach that should be taken.

comment:13 in reply to:  12 Changed 13 months ago by sisbell

We can just rename to libtor.so in the build file for tor-browser-build. The naming needn't be in the make file

Replying to eighthave:

The last piece, which is not implemented in the patches, is making the output filtrivialename be libtor.so. There is currently no support for different output file names in Makefile.am so it would be non-trivial to implement, and I'm not sure the approach that should be taken.

comment:14 Changed 13 months ago by nickm

Would it be feasible to have this as a github PR? Right now github is what we're using for our code reviews, mostly.

comment:16 Changed 13 months ago by eighthave

@sisbell one of my goals here is to remove as many of the "papercuts" as possible, building Tor apps on Android is currently "death by a million papercuts". Renaming the resulting file is one of those papercuts. The easier it is for some Android or iOS developer to work with Tor, the more people we'll have working on tor mobile. And for the existing apps, it means less complexity to manage when keeping things current. For example, there was lots of good work done in core tor to improve things on mobile. It took us a long time to integrate and ship that work, mostly because of the current vast complexity between the core tor source repo and actually shipping it in an Android app.

comment:17 Changed 12 months ago by dgoulet

Reviewer: nickm

comment:18 Changed 12 months ago by nickm

Status: needs_reviewneeds_revision

I've had a look over the changes, except for the gitlab CI stuff which I'm not competent to evaluate. Since others asked for that, maybe they could weigh in whether it seems right to them? (I was about to suggest doing gitlab CI in a separate branch, but I see that somebody else requested it, so I probably should defer to them.)

I've left some comments on the PR. Additionally, this needs a file in the changes/* directory to explain what it is when we generate the changelog. The doc/HACKING/CodingStandards.md file explains how those look.

Please let me know if you have any questions. Thanks!

comment:19 Changed 12 months ago by eighthave

I included the gitlab-ci changes there to make it easy to see which run matches the pull request. They could be merged separately, if that's easier. That's why I included all those commits on top of the code changes.

comment:20 Changed 12 months ago by eighthave

Status: needs_revisionneeds_review

I removed the "logcat" name change in favor of #32181, and otherwise addressed the comments in github.

comment:21 Changed 12 months ago by nickm

Status: needs_reviewneeds_revision

Okay, I've responded on the github comments again.

Are there changes that you forgot to push? I don't see any changes to the code on github since the force-push 8 days ago. Am I missing them?

comment:22 Changed 12 months ago by eighthave

Arg, yes, sorry about that. I pushed now.

comment:23 Changed 12 months ago by eighthave

Status: needs_revisionneeds_review

comment:24 Changed 12 months ago by eighthave

Status: needs_reviewnew

Re: https://trac.torproject.org/projects/tor/ticket/32191#comment:27

We're going to maintain the Android and iOS build systems in this fork:
https://github.com/guardianproject/tor

Please take anything from there as you see fit, under the license you require.

comment:25 Changed 12 months ago by teor

Status: newneeds_review

If this PR can be merged as-is, we should probably merge it.

comment:26 Changed 11 months ago by nickm

Owner: set to nickm
Status: needs_reviewaccepted

The PR can't be merged as-is. The android log parts are now #32181, where we're taking a simpler approach and just removing them. That leaves these parts:

  • Changing --enable-android to imply a suite of other options
  • Removing USE_ANDROID.

I think removing USE_ANDROID is fine to do after #32181 is merged, since we'll have a conflict there.

This leaves changing --enable-android to imply --disable-asciidoc --disable-html-manual --disable-manpage --enable-pic --disable-system-torrc --disable-tool-name-check --disable-systemd. For that, I'd like to survey a couple of other android integrators to see if they'd also want those defaults, and whether they'd think that a "contrib/configure_android.sh" script was adequate. If they _do_ want something like this:

  1. Then the configure option should change the defaults, not override other settings.
  2. Probably it should get a new name other than --enable-android, since most of the changes have nothing to do with being android-specific, and since --enable-android used to mean something else.

I'll take that on.

comment:27 Changed 9 months ago by nickm

Keywords: 043-deferred added

All 0.4.3.x tickets without 043-must, 043-should, or 043-can are about to be deferred.

comment:28 Changed 9 months ago by nickm

Milestone: Tor: 0.4.3.x-finalTor: 0.4.4.x-final

comment:29 Changed 5 months ago by nickm

Keywords: 044-deferred added
Milestone: Tor: 0.4.4.x-finalTor: unspecified

Bulk-remove tickets from 0.4.4. Add the 044-deferred label to them.

Note: See TracTickets for help on using tickets.