Opened 3 months ago

Closed 3 weeks ago

Last modified 43 hours ago

#27443 closed defect (fixed)

Update Firefox RBM config and build for Android

Reported by: sisbell Owned by: tbb-team
Priority: Very High 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

Update Firefox RBM config and build for Android

Child Tickets

Attachments (4)

android.patch (4.4 KB) - added by sisbell 5 weeks ago.
Firefox patch NDK Version
android.2.patch (2.2 KB) - added by sisbell 5 weeks ago.
Rust Patch
dl_iterate_phdr.patch (1007 bytes) - added by sisbell 5 weeks ago.
Firefox Patch (v2)
0001-Make-sure-dl_iterate_phdr-is-undefined-on-Android.patch (1.2 KB) - added by gk 4 weeks ago.

Download all attachments as: .zip

Change History (88)

comment:1 Changed 3 months ago by sisbell

Created a patch since dl_iterate_phdr doesn't exist on debian stretch

comment:3 Changed 3 months ago by boklm

Keywords: TorBrowserTeam201809R added; TorBrowserTeam201808 removed
Status: newneeds_review

comment:4 in reply to:  2 Changed 3 months ago by boklm

Keywords: TorBrowserTeam201809 added; TorBrowserTeam201809R removed
Status: needs_reviewneeds_revision

Replying to sisbell:

commit [config/build] : https://github.com/sisbell/tor-browser-build/commit/1b9052d6f98d1ffe8bc3c9108302750ca036a075

Some comments about this commit:

  • setup of android-toolchain should be done in var/setup defined in projects/android-toolchain/config.
  • it should not be needed to list android-toolchain as dependency if it is set as var/compiler.
  • ndk.zip should be downloaded as part of android-toolchain instead of in projects/firefox/config
  • why is there an exit 0 after the ./mach package line?

comment:5 Changed 3 months ago by sisbell

android-toolchain now handles the ndk.zip download. I still need to remove the ndk file entry from the firefox config. Getting firefox project working with the latest android-toolchain changes is next on my list.

The exit 0 is there since mach package builds the apk, which I believe is the final packaging we want. It didn't seem the Mozilla Archive that follows is relevant to Android.

comment:6 in reply to:  5 Changed 3 months ago by boklm

Replying to sisbell:

The exit 0 is there since mach package builds the apk, which I believe is the final packaging we want. It didn't seem the Mozilla Archive that follows is relevant to Android.

I think that ./mach package will create the apk in the build directory. However we exit immediately after that, so it seems to me we are just losing the result of the build after it finished. I think we are missing something to move the apk file, and probably the martools, to the destination directory.

comment:7 Changed 2 months ago by gk

Keywords: TorBrowserTeam201810 added; TorBrowserTeam201809 removed

Moving tickets to October

comment:8 Changed 2 months ago by sisbell

I made some additional fixes.

  1. Build packages the android dist directory.
  1.  No more exit 0 after build package.
  1. Uses a tarball of android archives so there is no network connection needed during build.
  1. Uses android-ndk packaged compilers.

Some additional minor cleanup items left.

  1. What to include in dist package. Just apks?
  1. Use a new var to indicate if mar tools should be built (rather than platform var)

comment:9 in reply to:  8 ; Changed 2 months ago by boklm

Replying to sisbell:

I made some additional fixes.

  • Could you push your new revisions to new branches instead of force-pushing on the same branch? This makes it harder to see what was changed since the previous revision.
  • Could you also rebase it on current master? It is currently based on a commit that is more than 2 months old.
  • I am not sure I understand what those lines are doing. This seems to be creating a gcc -> gcc symlink?
    +[% IF c('var/android') %]
    +  ln -s gcc $ANDROID_NDK_HOME/arm/bin/gcc
    +[% END -%]
    
  • could you add a comment explaining why we need the projects/firefox/gradle.patch? Maybe this is something we want to patch in tor-browser.git directly?
  • there is a trailing whitespace in projects/firefox/mozconfig-android-armv7 at the end of the --with-android-sdk= line
  • do we really need our own build of binutils for the android build of firefox?
  • commit 6af47ae272b6098eac1c9ba886ff9ab04e3fc441 seems to have the wrong bug number
  • could you explain why we need projects/firefox/android.patch? It seems to be removing some code behind #if defined(GP_OS_linux) and forcing use of the code previously behind #elif defined(GP_OS_android). Shouldn't defined(GP_OS_linux) be false and defined(GP_OS_android) be true in the android build?

Some additional minor cleanup items left.

  1. What to include in dist package. Just apks?

I think projects/firefox/build can just copy the generated apk files, for now. Later we will probably need to repackage them in projects/tor-browser/build to include other components such as tor and Orbot, but this can be done later.

  1. Use a new var to indicate if mar tools should be built (rather than platform var)

Don't we want to build mar files for android too? Or is the firefox updater based on mar files not working on android?

comment:10 in reply to:  9 Changed 8 weeks ago by gk

Replying to boklm:

Replying to sisbell:

  1. Use a new var to indicate if mar tools should be built (rather than platform var)

Don't we want to build mar files for android too? Or is the firefox updater based on mar files not working on android?

Using the Firefox updater for mobile is pretty much untested. I think we want to do that at some point for bundles we distribute on our website but it's okay to start without it.

comment:11 Changed 8 weeks ago by sisbell

I rebased to the latest code from trunk. It looks like the current gradle build has upgraded the sdk to 26. I made changes to update in android-toolchain but the firefox build is still broken. I'll need to investigate causes and fix.

comment:12 Changed 8 weeks ago by sisbell

Changes (android-1017)

  • Removed softlink for gcc
  • Removed trailing spaces in mozconfig
  • Removed binutils dependency for android
  • Added specific var/martool rather than using var/platform
  • Added in tor branding property into the mozconfig

comment:13 Changed 8 weeks ago by sisbell

Changes (android-1017)

  • Applied patch for downloading artifacts through script

comment:14 in reply to:  9 Changed 8 weeks ago by boklm

I think those comments are still valid for commit 2276a44b14edf82f0f44ac98c50597a8637e5883:

  • could you add a comment explaining why we need the projects/firefox/gradle.patch? Maybe this is something we want to patch in tor-browser.git directly?
  • could you explain why we need projects/firefox/android.patch? It seems to be removing some code behind #if defined(GP_OS_linux) and forcing use of the code previously behind #elif defined(GP_OS_android). Shouldn't defined(GP_OS_linux) be false and defined(GP_OS_android) be true in the android build?

And also:

  • the patch is adding an i after the creation of the martools zip file. Those lines should be indented for the IF.

comment:15 Changed 8 weeks ago by gk

Additionally:

-[% IF c("var/linux") %]
+[% IF c("var/linux")  %]

I think there is no need to change that line.

+  ./mach package
+    cp -a obj-*/dist/* $distdir/Browser/

Please make it

+  ./mach package
+  cp -a obj-*/dist/* $distdir/Browser/

Could you remove

+#ac_add_options --with-android-version=23
+#enable ccache to set amount of cache assigned for build.
+#ac_add_options --with-ccache

from the mozconfig file?

comment:16 Changed 7 weeks ago by sisbell

Changes (android-1024)

  • Removed gradle and android patches. With the correct version of NDK/SDK we don't need the android.patch
  • Use android-version 22 for the NDK. The target sdk is still 26. This is the highest version that correctly compiles the project w/o patches. Verified that the apk runs successfully,
  • Removed ac_add_options --disable-install-strip from mozconfig. This causes a runtime error of apk. After researching forums, this is the suggested solution.
  • Removed 'i' character at end of file
  • Removed commented out options from mozconfig
  • Fixed formatting issues in build file

After these fixes, we have a successful end-to-end build with the latest code and no patches needed. Apk runs on device. App branded with tor resources.

comment:17 Changed 7 weeks ago by sisbell

Status: needs_revisionneeds_review

comment:18 Changed 7 weeks ago by boklm

Status: needs_reviewneeds_revision

Looking at commit 370168b7b2b00dcd72e51e6a229d7720cfe69c5d, currently the output of the build is a tor-browser.tar.gz file containing two apk files in a Browser/ directory. Instead I think we could just copy the two apk files to the destination directory ([% dest_dir _ '/' _ c('filename') %]), after creating it. And after the apk files are copied, I think we can add a [% RETURN %] if none of the following lines apply to the android build (and in that case also remove all the var/martools changes).

comment:19 Changed 7 weeks ago by sisbell

Changes (android-1024)

  • Directly copy the apks to dest_dir (no archive)
  • Remove martools references
  • Return directly after packaging stage for android

comment:20 Changed 7 weeks ago by sisbell

Status: needs_revisionneeds_review

comment:21 Changed 7 weeks ago by gk

Keywords: TorBrowserTeam201810R added; TorBrowserTeam201810 removed

comment:22 Changed 6 weeks ago by boklm

Commit d40c9716e56abc5bc71ec3133d302d0a31cf5cb1 looks good to me.

comment:23 Changed 6 weeks ago by gk

Keywords: TorBrowserTeam201810 added; TorBrowserTeam201810R removed
Status: needs_reviewneeds_revision

Here are three things that I think that should get addressed:

1) mozconfig-android-armv7 is not in sync with the .mozconfig-android file in the tor-browser repo, which we use for building Tor Browser for Android right now. E.g. do you disable optimizations while we have that enabled in the .mozconfig file we use for official builds. MOZILLA_OFFICIAL is note exported either etc. Please double-check the file in the tor-browser repo and fix up the mozconfig file in the tor-browser-build repo.

2) Where are all those Gradle dependencies coming from? How do we make sure we got all in the .txt fils (and only those we need)? Can we document the process of generating/updating the .txt file in that file itself (both to preserve knowledge and make the process transparent)?

3) It seems to me not all the packages in arch_deps are needed (e.g. chrpath was just added for selfrando on Linux). Can we clean that up accordingly?

comment:24 in reply to:  23 Changed 6 weeks ago by gk

Replying to gk:

3) It seems to me not all the packages in arch_deps are needed (e.g. chrpath was just added for selfrando on Linux). Can we clean that up accordingly?

To be more specific here: building works for me just with the OpenJDK related package in arch_deps.

comment:25 Changed 5 weeks ago by gk

Keywords: TorBrowserTeam201811 added; TorBrowserTeam201810 removed

Moving our tickets to November.

comment:26 Changed 5 weeks ago by sisbell

Changes (android-1105)

  • config: Cleaned up arch_deps. Added ccache to support --with-ccache option in mozconfig-android-armv7
  • Added projects/firefox/how-to-create-dependencies-list.txt
  • mozconfig-android-armv7: Enabled optimization and cache options. I added the MOZILLA_OFFICIAL export to align with what is in tor-browser git repo. Although I'm not convinced this is really needed, it won't do any harm.

I verified that apk runs on device.

comment:27 Changed 5 weeks ago by sisbell

Status: needs_revisionneeds_review

comment:28 Changed 5 weeks ago by gk

Status: needs_reviewneeds_revision

I think we can disable the ccache related things. We are building within containers from scratch and ccache should not help much in that scenario. However, it might do so in local builds which is why it is there. Sorry, for not being more explicit here. :(

comment:29 Changed 5 weeks ago by boklm

I think a filename like how-to-create-gradle-dependencies-list.txt would be more clear (as this is only about the gradle dependencies). Wrapping lines to ~80 characters would make it easier to read.

comment:30 Changed 5 weeks ago by sisbell

I verified build was working this morning but within the last hour it stop building with errors:

  • Could not find junit:junit:4.12
  • Could not find org.robolectric:robolectric:3.5.1
  • org.mockito:mockito-core:1.10.19

These are easy to add but the question is why did these test dependencies show up? I'll look into the cause of the changes...

comment:31 Changed 5 weeks ago by sisbell

I looks like the export MOZILLA_OFFICIAL does change behavior of the build. It triggers the unit tests on the build. I'll add the test libraries to the gradle-dependencies and then get that checked in. I'll also add a comment in mozconfig explaining the reasons for the export.

comment:32 Changed 5 weeks ago by sysrqb

I'm excited for this!

In projects/firefox/mozconfig-android-armv7

+mk_add_options MOZILLA_OFFICIAL=1

We don't need this automake directive anymore (the export replaces it)

+ac_add_options --with-android-version=22

Do we need this? We've used the default value without a problem (the default is 9 for 32-bit CPU archs, and 21 for 64-bit).

+ac_add_options --with-ccache

I agree with gk in comment:28 - ccache won't help us here

In projects/firefox/config

+  android-armv7:
+    var:
+      arch_deps:
+        - openjdk-8-jdk
+        - ccache

nit: ccache here, too

projects/firefox/how-to-create-dependencies-list.txt
nit: can the lines be wrapped at some length? (72, 79, 80, 100?)

In projects/firefox/fetch-gradle-dependencies

+    echo "[% artifact.sha256sum %]  downloaded_file" | sha256sum -c

Does the script fail when this line returns non-zero? There's no error handling, so I'm guessing this is caught another way?

comment:33 Changed 5 weeks ago by sisbell

The MOZILLA_OFFICIAL export executes some tasks that handles dependencies in a different way. Basically the build just fails with missing artifacts, rather than trying to download them. Many of these artifacts are in different repositories than we were using on the first list of dependencies.

Its tedious tracking each dependency through manually so I'm looking for an easier way to handle this.

comment:34 in reply to:  32 Changed 5 weeks ago by sisbell

Replying to sysrqb:

I'm excited for this!

+ac_add_options --with-android-version=22

Do we need this? We've used the default value without a problem (the default is 9 for 32-bit CPU archs, and 21 for 64-bit).

Under the current build configuration, it defaults to 9 in all cases. Is there someway to tell the build that its for 32 or 64 bit?
From my testing, 22 is the highest version that works. This is the only way I'm aware of to configure the android version.

comment:35 Changed 5 weeks ago by sisbell

Changes (android-1105a)

  • Remove MOZILLA_OFFICIAL ac_add_options
  • Regenerated the gradle-dependencies-list to include test artifacts (around 150 additional entries)
  • Renamed file to how-to-create-gradle-dependencies-list.txt, wrapped lines
  • Updated how-to-create-gradle-dependencies-list.txt with new info
  • Remove ccache references

comment:36 Changed 5 weeks ago by gk

Keywords: TorBrowserTeam201811R added; TorBrowserTeam201811 removed
Status: needs_revisionneeds_review

comment:37 in reply to:  32 Changed 5 weeks ago by boklm

Replying to sysrqb:

In projects/firefox/fetch-gradle-dependencies

+    echo "[% artifact.sha256sum %]  downloaded_file" | sha256sum -c

Does the script fail when this line returns non-zero? There's no error handling, so I'm guessing this is caught another way?

There is a set -e at the beginning of the script, so the script should fail if one of the commands fail.

comment:38 in reply to:  35 Changed 5 weeks ago by boklm

Replying to sisbell:

  • Renamed file to how-to-create-gradle-dependencies-list.txt, wrapped lines

The file now contains many trailing white spaces, which should be removed.

And I think the commands should not be wrapped:

   export GRADLE_MAVEN_REPOSITORIES="file://$rootdir/[% 
c('input_files_by_name/gradle-dependencies') %]"
`cat logs/firefox-android-armv7.log | grep "Download http" | sed "s/^.*Download 
//g" > download-urls.txt`

In the line * sha256sum | url I think the * should be removed.

comment:39 Changed 5 weeks ago by boklm

Keywords: TorBrowserTeam201811 added; TorBrowserTeam201811R removed
Status: needs_reviewneeds_revision

comment:40 Changed 5 weeks ago by gk

Keywords: TBA-a2 added
Priority: MediumVery High

comment:41 Changed 5 weeks ago by sisbell

Changes (android-1106)

comment:42 Changed 5 weeks ago by sisbell

Status: needs_revisionneeds_review

comment:43 Changed 5 weeks ago by gk

Status: needs_reviewneeds_revision

Okay, I spent quite some time testing and reviewing the updated patch. I feel we are close. Here are three things that I found so far which should be fixed:

1) You have --disable-strip in the mozconfig file. Please use --enable-strip instead (even though the resulting binaries are indeed stripped), as this is less confusing.

2) Could you add a comment in the mozconfig file above

ac_add_options --with-libclang-path=/var/tmp/dist/android-toolchain/android-ndk/arm/lib64
ac_add_options --with-clang-path=/var/tmp/dist/android-toolchain/android-ndk/arm/bin/clang

explaining that we need that for the Stylo build part and the preferred way, using LLVM_CONFIG, is not available as there is no llvm-config in the ndk. (at least I could not find it)

3) The gradle deps download script mentions issues with setting LC_ALL=C. Please add the bug number as reference (it is #28117).

Finally, could you explain what breaks without setting --with-android-version=22? You mentioned "22 is the highest version that works" but what does that mean?

comment:44 Changed 5 weeks ago by gk

Oh, I forgot one and a half thing to fix actually:

4) I followed the instructions on how to create the gradle dependency list. In particular I tested:

It may also be the case that you wish to cleanup old versions of the artifacts.
For this you will need to run the build with an empty
gradle-dependencies-list.txt file and proceed to reconstruct the list from
scratch.

assuming "empty" means still there but without contents. That actually breaks the build. So, I am not sure which step you had in mind for cleaning up old versions of the artifacts. Could you reformulate that part?

And s/cleanup/clean up/.

Last edited 5 weeks ago by gk (previous) (diff)

comment:45 Changed 5 weeks ago by sisbell

Status: needs_revisionneeds_review

Changes (android-1107)

  • [mozconfig:L mozconfig: enable-strip option set]
  • mozconfig: added comment that llvm-config is not supported in android NDK
  • fetch-gradle-dependencies: add bug number to comment
  • how-to-create-gradle-dependencies-list.txt: clarified how to start dependency list from scatch
  • build: copy apk with a fixed file name (to make it easier to process in tor-browser)

comment:46 Changed 5 weeks ago by gk

Status: needs_reviewneeds_revision

So, the last thing that bothers me is --with-android-version=22. sisbell: what breaks here for you? I ran e.g. a built with version 21 and it worked well. We should at least document with a TODO what made us pick that option.

comment:47 Changed 5 weeks ago by gk

Okay, the errors I get are:

68:58.69 /var/tmp/build/firefox-d60cb3854f94/tools/profiler/core/shared-libraries-linux.cc:191: error: undefined reference to 'dl_iterate_phdr'
68:58.69 /var/tmp/build/firefox-d60cb3854f94/tools/profiler/core/shared-libraries-linux.cc:0: error: undefined reference to 'dl_iterate_phdr'
68:58.69 armv7-linux-androideabi/release/libgkrust.a(elf.o):elf.c:function backtrace_initialize: error: undefined reference to 'dl_iterate_phdr'
68:58.69 armv7-linux-androideabi/release/libgkrust.a(mmapio.o):mmapio.c:function backtrace_get_view: error: undefined reference to 'getpagesize'
68:58.69 clang++: error: linker command failed with exit code 1 (use -v to see invocation)
68:58.69 /var/tmp/build/firefox-d60cb3854f94/config/rules.mk:699: recipe for target 'libxul.so' failed
68:58.69 make[4]: *** [libxul.so] Error 1

comment:48 in reply to:  47 Changed 5 weeks ago by sisbell

Replying to gk:

Okay, the errors I get are:

68:58.69 /var/tmp/build/firefox-d60cb3854f94/tools/profiler/core/shared-libraries-linux.cc:191: error: undefined reference to 'dl_iterate_phdr'
68:58.69 /var/tmp/build/firefox-d60cb3854f94/tools/profiler/core/shared-libraries-linux.cc:0: error: undefined reference to 'dl_iterate_phdr'
68:58.69 armv7-linux-androideabi/release/libgkrust.a(elf.o):elf.c:function backtrace_initialize: error: undefined reference to 'dl_iterate_phdr'
68:58.69 armv7-linux-androideabi/release/libgkrust.a(mmapio.o):mmapio.c:function backtrace_get_view: error: undefined reference to 'getpagesize'
68:58.69 clang++: error: linker command failed with exit code 1 (use -v to see invocation)
68:58.69 /var/tmp/build/firefox-d60cb3854f94/config/rules.mk:699: recipe for target 'libxul.so' failed
68:58.69 make[4]: *** [libxul.so] Error 1

Yes, this is the error that shows up if we don't use 21 or 22 NDK. I previously had a patch that could make it run on other versions but I think it would be better to use a version of the NDK that doesn't require a patch. I'm ok with either 21 or 22, I just chose 22 since I default to the highest compile version is best, due to possible bug fixes.

I'll document this in the mozconfig file.

comment:49 Changed 5 weeks ago by sisbell

Changes (android-1108)

  • Changed android-version to 21 since sysrqb says this is the current default. Documented reason for needing to use this version(s).

comment:50 in reply to:  49 ; Changed 5 weeks ago by sysrqb

Replying to sisbell:

Yes, this is the error that shows up if we don't use 21 or 22 NDK. I previously had a patch that could make it run on other versions but I think it would be better to use a version of the NDK that doesn't require a patch. I'm ok with either 21 or 22, I just chose 22 since I default to the highest compile version is best, due to possible bug fixes.

Replying to sisbell:

Changes (android-1108)

  • Changed android-version to 21 since sysrqb says this is the current default. Documented reason for needing to use this version(s).

sisbell, can you post the patch needed for successfully building against android-9? In our current code the default is android-9 for non-x86 CPU targets, it is 21 for ['aarch64', 'x86_64', 'mips64'] cpu targets.

In August Mozilla bumped their versions:
https://hg.mozilla.org/mozilla-central/rev/2b2bd723ebc8

and now their default is android 16.

I prefer not making drastic changes like this (jumping from android-9 to android-22). Mozilla are very conservative about when they make these changes with good reason. For changes like this, i wish we had a working test suite. If we use this, then we need a lot of testing over the week before we release the next alpha version.

Changed 5 weeks ago by sisbell

Attachment: android.patch added

Firefox patch NDK Version

Changed 5 weeks ago by sisbell

Attachment: android.2.patch added

Rust Patch

Changed 5 weeks ago by sisbell

Attachment: dl_iterate_phdr.patch added

Firefox Patch (v2)

comment:51 in reply to:  50 Changed 5 weeks ago by sisbell

Replying to sysrqb:

Replying to sisbell:

Yes, this is the error that shows up if we don't use 21 or 22 NDK. I previously had a patch that could make it run on other versions but I think it would be better to use a version of the NDK that doesn't require a patch. I'm ok with either 21 or 22, I just chose 22 since I default to the highest compile version is best, due to possible bug fixes.

Replying to sisbell:

Changes (android-1108)

  • Changed android-version to 21 since sysrqb says this is the current default. Documented reason for needing to use this version(s).

sisbell, can you post the patch needed for successfully building against android-9? In our current code the default is android-9 for non-x86 CPU targets, it is 21 for ['aarch64', 'x86_64', 'mips64'] cpu targets.

In August Mozilla bumped their versions:
https://hg.mozilla.org/mozilla-central/rev/2b2bd723ebc8

and now their default is android 16.

I prefer not making drastic changes like this (jumping from android-9 to android-22). Mozilla are very conservative about when they make these changes with good reason. For changes like this, i wish we had a working test suite. If we use this, then we need a lot of testing over the week before we release the next alpha version.

I've attached the patches I was using to get things working before. They were more to get things working in the moment and aren't cleaned or reviewed but they illustrate where the problem was occurring.

comment:52 Changed 5 weeks ago by gk

Okay, so it seems not uncommon that the Android NDK does not have a symbol at a particular level but the headers are still exposing it. So, right, there is no dl_iterate_phdr below Android API 21. However, Mozilla coped with that and provided fallbacks in case that's needed. See:

https://hg.mozilla.org/mozilla-central/rev/3b862e2b9e0b
https://hg.mozilla.org/mozilla-central/rev/bb9548c09c19

The question we need to answer is why this is an issue in tor-browser-build but not when building outside it?

comment:53 Changed 5 weeks ago by gk

Taking the rust issues into account it seems like our toolchain is not correctly set up/used...

comment:54 in reply to:  53 ; Changed 5 weeks ago by sisbell

Replying to gk:

Taking the rust issues into account it seems like our toolchain is not correctly set up/used...

I'll investigate building rust under NDK 16.

comment:55 in reply to:  54 ; Changed 5 weeks ago by sisbell

Replying to sisbell:

Replying to gk:

Taking the rust issues into account it seems like our toolchain is not correctly set up/used...

I'll investigate building rust under NDK 16.

According to this link Mozilla is using rust 24 for firefox 60: https://wiki.mozilla.org/Rust_Update_Policy_for_Firefox

comment:56 in reply to:  55 Changed 5 weeks ago by sisbell

Replying to sisbell:

Replying to sisbell:

Replying to gk:

Taking the rust issues into account it seems like our toolchain is not correctly set up/used...

I'll investigate building rust under NDK 16.

According to this link Mozilla is using rust 24 for firefox 60: https://wiki.mozilla.org/Rust_Update_Policy_for_Firefox

I switched to NDK 16 and Rust to 1.24.1. It gets through rust build stage just fine. However, it fails in missing getpagesize during firefox build. So a partial success. I'll keep investigating

comment:57 Changed 5 weeks ago by gk

Okay, I spent quite some time to set up infrastructure to be able to compare a non-failing build with a build one by disabling piece-by-piece parts of the toolchain we built. It turns out that just using the Rust compiler is causing all the errors for me. That is: once I use rustup everything is fine (all other things being equal).

I'll double-check that as I am not sure yet why the non-rust files should be affected but that's what my testing currently gives me.

Regarding the Rust version: Yes, Firefox is officially using 1.24.1 but we bumped the version to 1.26.1 for all of our platforms for build/runtime issues. I don't think we should switch back to 1.24.1 for Android. If it is really a Rust version problem we should figure our what it is and probably add a fixup to 1.26.1 in our build.

Last edited 5 weeks ago by gk (previous) (diff)

comment:58 Changed 5 weeks ago by gk

Oh, one thing I noticed while fiddling with the mozconfig file: We should do something like ANDROID_BASE=/var/tmp/dist/android-toolchain in the mozconfig file and then replace all /var/tmp/dist/android-toolchain instances with $ANDROID_BASE.

comment:59 in reply to:  57 ; Changed 5 weeks ago by sisbell

Replying to gk:

Okay, I spent quite some time to set up infrastructure to be able to compare a non-failing build with a build one by disabling piece-by-piece parts of the toolchain we built. It turns out that just using the Rust compiler is causing all the errors for me. That is: once I use rustup everything is fine (all other things being equal).

Is your rust version (from rustup) using target arm-linux-androideabi or armv7-linux-androideabi

comment:60 in reply to:  59 Changed 5 weeks ago by gk

Replying to sisbell:

Replying to gk:

Okay, I spent quite some time to set up infrastructure to be able to compare a non-failing build with a build one by disabling piece-by-piece parts of the toolchain we built. It turns out that just using the Rust compiler is causing all the errors for me. That is: once I use rustup everything is fine (all other things being equal).

Is your rust version (from rustup) using target arm-linux-androideabi or armv7-linux-androideabi

armv7-linux-androideabi. I used rustup target add armv7-linux-androideabi to get it.

comment:61 in reply to:  57 ; Changed 5 weeks ago by gk

Replying to gk:

Okay, I spent quite some time to set up infrastructure to be able to compare a non-failing build with a build one by disabling piece-by-piece parts of the toolchain we built. It turns out that just using the Rust compiler is causing all the errors for me. That is: once I use rustup everything is fine (all other things being equal).

I'll double-check that as I am not sure yet why the non-rust files should be affected but that's what my testing currently gives me.

Confirmed. Just switching back to the pre-built 1.30 and leaving all other things the same gets rid of all the errors seen in comment:47.

comment:62 in reply to:  61 ; Changed 5 weeks ago by sisbell

Replying to gk:

Replying to gk:

Okay, I spent quite some time to set up infrastructure to be able to compare a non-failing build with a build one by disabling piece-by-piece parts of the toolchain we built. It turns out that just using the Rust compiler is causing all the errors for me. That is: once I use rustup everything is fine (all other things being equal).

I'll double-check that as I am not sure yet why the non-rust files should be affected but that's what my testing currently gives me.

Confirmed. Just switching back to the pre-built 1.30 and leaving all other things the same gets rid of all the errors seen in comment:47.

So I'm able to successfully build tor-browser with NDK 16 and Rust 1.28.0, which is what Mozilla is using to build firefox 63. Android APK installs and runs. Is there any objection to using this configuration?

comment:63 in reply to:  62 Changed 4 weeks ago by gk

Replying to sisbell:

Replying to gk:

Replying to gk:

Okay, I spent quite some time to set up infrastructure to be able to compare a non-failing build with a build one by disabling piece-by-piece parts of the toolchain we built. It turns out that just using the Rust compiler is causing all the errors for me. That is: once I use rustup everything is fine (all other things being equal).

I'll double-check that as I am not sure yet why the non-rust files should be affected but that's what my testing currently gives me.

Confirmed. Just switching back to the pre-built 1.30 and leaving all other things the same gets rid of all the errors seen in comment:47.

So I'm able to successfully build tor-browser with NDK 16 and Rust 1.28.0, which is what Mozilla is using to build firefox 63.

It seems Mozilla is using NDK r17b and *API level* 16 in Firefox 63, no? See: https://bugzilla.mozilla.org/show_bug.cgi?id=1482330.

Android APK installs and runs. Is there any objection to using this configuration?

Well, it seems we tested the last alpha releases with NDK r15c. So, I am very reluctant to change that unless it is really needed. And, in fact, that does not seem to be the case for me as I can compile Firefox for mobile fine by just bumping the Rust version to 1.28.0 and leaving the NDK version alone. Or are you indicating that a Tor Browser for Android compiled that way is encountering runtime issues (admittedly I did not test that)?

So, hopefully, that leaves the question what to do with the Rust version. I guess due to sysrqb's build script we have been using whatever Rust version was stable once we released a new Tor Browser for Mobile, so switching to 1.28.0 does not sound problematic from that perspective. However, there are two reasons which make me hesitate:

1) It adds complexity for our (alpha) build process as all the other alpha bundles get compiled with 1.26.1.

2) There is the risk we introduce new, uncaught reproducibility issues while we seem to have a good understanding about the problems in 1.26.1 (see the long story on that in #26475).

So, I try to pin down this week what exactly got fixed in 1.28.0. Ideally, it's a small patch we can backport for 1.26.1. If so, then that's the thing we should do. If not, or if the bisecting takes too long (I already encountered a bunch of bustage while trying to figure out the nightly version first, that fixed the problem), then we should go with 1.28.0 for this release and think about a better fix for the upcoming alpha releases.

comment:64 Changed 4 weeks ago by gk

It seems this problem got "solved" in 1.28.0 by upgrading libbacktrace (https://github.com/rust-lang/rust/pull/50955). Unforunately, that's not the small fix I hoped to find. So, I'll dig deeper tomorrow. Maybe we can backport the patch for libbacktrace that apparently fixed the problem...

comment:65 Changed 4 weeks ago by gk

Okay, I dug deeper. I am still working on a good fix, but, sisbell, for now we can disable building the backtrace feature which is I think okay. You can do --set=rust.backtrace=false in configure_opt for the Android build. At least that compiles for me. Could you try that out and update your patch accordingly (please add a note about why we do disable libbacktrace for now)?

Meanwhile I'll work on a better patch and hope to have this ready for next week's release.

comment:66 Changed 4 weeks ago by gk

Oh, and why is the linker set to clang if we compile with a GCC toolchain? Actually, I think we even don't need the linker line. What is breaking without it? (And need we the ar one)?

comment:67 in reply to:  65 Changed 4 weeks ago by sisbell

Replying to gk:

Okay, I dug deeper. I am still working on a good fix, but, sisbell, for now we can disable building the backtrace feature which is I think okay. You can do --set=rust.backtrace=false in configure_opt for the Android build. At least that compiles for me. Could you try that out and update your patch accordingly (please add a note about why we do disable libbacktrace for now)?

Meanwhile I'll work on a better patch and hope to have this ready for next week's release.

I did a complete build from rust project to tor-browser. Setting the backtrace option makes everything build perfectly.

comment:68 Changed 4 weeks ago by gk

Okay, I think that both problems you encountered are actual libbacktrace bugs. libbacktrace treats similar dl_iterate_phdr issues by making sure HAVE_DL_ITERATE_PHDR is not defined. That's essentially what the Rust folks did when updating libbacktrace for 1.28.0, so I think following that path is okay. I attached a patch for that.

I am not sure yet what's up with getpagesize but I think your approach is right using sysconf(_SC_PAGESIZE) instead.

Additionally, it seems we don't need the ar related option in configure_opt either. Could you remove that one (+ the linker related one) and add a dl_iterate_phdr + getpagesize patch in a separate commit? And on top of that one the one we thought for this bug? (I think everything you had was good, just the API line in the mozconfig-android-armv7 file should go aswell).

My current plan is to not spend more time on this issue (although it's weird that Mozilla did not run into those problems). I already shaved enough yaks to get that far. :)

comment:70 Changed 4 weeks ago by sisbell

Status: needs_revisionneeds_review

Changes (android-1115)

  • Copy Orbot aars into mobile/android/app directory before building
  • Remove --with-android-version from mozconfig. The version defaults to 9 (which is default for this version of Firefox), rather than previously defined version 16. 
  • Removed fetch-gradle-dependencies (now generalized and in common project)

Issues: When I run the final apk from this build, it starts up fine but still asks me to install Orbot. Am I placing the aars correctly?

comment:71 Changed 4 weeks ago by gk

Keywords: TorBrowserTeam201811R added; TorBrowserTeam201811 removed

comment:72 Changed 4 weeks ago by gk

Keywords: TorBrowserTeam201811 added; TorBrowserTeam201811R removed
Status: needs_reviewneeds_revision

We should not mix the Firefox build step with the bundling step if possible. If we need to bundle things (and we want to do that for the Orbot stuff) then this should happen in the tor-browser step where we already bundle things like extensions. And we need to open up the .apk anyway for that and recreate the signature. (So, the Orbot project should be a dependency for the tor-browser one not for the Firefox one).

comment:73 in reply to:  72 Changed 4 weeks ago by sysrqb

Replying to gk:

We should not mix the Firefox build step with the bundling step if possible. If we need to bundle things (and we want to do that for the Orbot stuff) then this should happen in the tor-browser step where we already bundle things like extensions. And we need to open up the .apk anyway for that and recreate the signature. (So, the Orbot project should be a dependency for the tor-browser one not for the Firefox one).

I chatted a little with gk about this on IRC. As a summary, this is complicated because Fennec and Orbot are now tightly coupled where Orbot is now a compile-time dependency of Fennec. Fennec uses some public constants defined by Orbot and Fennec now directly calls some Orbot classes. We may be able to add stub implementations of these and then rebuild/repackage the app in the tor-browser project with the real implementations, but that will need more time than we currently have for the upcoming release. We decided we can continue bundling Orbot in the Firefox build step and then re-evaluate this later.

comment:74 in reply to:  70 ; Changed 4 weeks ago by sysrqb

Replying to sisbell:

Changes (android-1115)

  • Copy Orbot aars into mobile/android/app directory before building
  • Remove --with-android-version from mozconfig. The version defaults to 9 (which is default for this version of Firefox), rather than previously defined version 16. 
  • Removed fetch-gradle-dependencies (now generalized and in common project)

Issues: When I run the final apk from this build, it starts up fine but still asks me to install Orbot. Am I placing the aars correctly?

Yes, that is a bug in our tor-browser code. I'll create a patch for that.

comment:75 Changed 4 weeks ago by sisbell

So far it looks like the remaining issue on this ticket is

  • Adding new patch for firefox that remove Install Orbot dialog
  • Deciding the API version in mozconfig[with:9 : 9 ,16, 26]

I'm going to vote for 16 since its conservative given our deadlines. The toolchain not assembling on 9 is a red flag for me.

comment:76 Changed 4 weeks ago by gk

Yes, lets use --api 16. However, I'd like to have the Orbot parts in one commit that makes it easier backing things out if needed and is cleaner. I think making the necessary firefox changes over in #27977 (I am fine with a separate commit for that) seems better to me. So, could you do here

a) remove the Orbot related pieces
b) remove the how-to-create gradle dep list
c) add a note in gradle-dependencies-list.txt to point to the doc in /common

?

comment:77 Changed 4 weeks ago by sisbell

Changes (android-1116)

  • Remove orbot references
  • Remove how-to
  • Added note in gradle-dependencies-list.txt
  • Changed to use api 16 (mozconfig)

comment:78 in reply to:  74 ; Changed 4 weeks ago by sysrqb

Replying to sysrqb:

Replying to sisbell:

Changes (android-1115)

  • Copy Orbot aars into mobile/android/app directory before building
  • Remove --with-android-version from mozconfig. The version defaults to 9 (which is default for this version of Firefox), rather than previously defined version 16. 
  • Removed fetch-gradle-dependencies (now generalized and in common project)

Issues: When I run the final apk from this build, it starts up fine but still asks me to install Orbot. Am I placing the aars correctly?

Yes, that is a bug in our tor-browser code. I'll create a patch for that.

Hrm. That should be commented out. Did you test this with the tor-browser branch I pushed for this? (https://git.torproject.org/user/sysrqb/tor-browser.git branch 28051_1)

comment:79 in reply to:  78 Changed 3 weeks ago by sisbell

Replying to sysrqb:

Replying to sysrqb:

Replying to sisbell:

Changes (android-1115)

  • Copy Orbot aars into mobile/android/app directory before building
  • Remove --with-android-version from mozconfig. The version defaults to 9 (which is default for this version of Firefox), rather than previously defined version 16.
  • Removed fetch-gradle-dependencies (now generalized and in common project)

Issues: When I run the final apk from this build, it starts up fine but still asks me to install Orbot. Am I placing the aars correctly?

Yes, that is a bug in our tor-browser code. I'll create a patch for that.

Hrm. That should be commented out. Did you test this with the tor-browser branch I pushed for this? (https://git.torproject.org/user/sysrqb/tor-browser.git branch 28051_1)

I'm testing off of what is configured in tor-browser-build, so I guess that's why its still showing up https://git.torproject.org/tor-browser.git

I can hack up a local version and test the build with your changes.

comment:80 Changed 3 weeks ago by sisbell

Status: needs_revisionneeds_review

comment:81 Changed 3 weeks ago by gk

Status: needs_reviewneeds_revision

tor-browser recently (see: #25013) got Torbutton as a submodule. We need to take that into account when building the mobile Firefox part inside tor-browser-build with something like:

 git_url: https://git.torproject.org/tor-browser.git
+git_submodule: 1
 gpg_keyring: torbutton.gpg

in projects/firefox/build. Otherwise the build breaks.

comment:82 Changed 3 weeks ago by sisbell

Changes (android-1119)

  • Add git_submodule field to config

comment:83 Changed 3 weeks ago by gk

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

Looks good, thanks! Added to master (commit b068c56f0ae00663e267ae0c6a2c89dd06f6ea20).

comment:84 Changed 43 hours ago by gk

Sponsor: Sponsor8

Sponsor 8 work.

Note: See TracTickets for help on using tickets.