Opened 7 months ago

Closed 7 weeks ago

#30461 closed defect (fixed)

Update tor-android-service Project to Use Android Toolchain (Firefox 68)

Reported by: sisbell Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-rbm, ff68-esr, tbb-9.0-must-alpha, TorBrowserTeam201910R
Cc: boklm, sisbell, gk Actual Points:
Parent ID: #30324 Points:
Reviewer: Sponsor: Sponsor44-can

Description

Update tor-android-service Project to use the new android toolchain. This will include updating gradle dependencies and changes to the api versions.

Child Tickets

Change History (45)

comment:1 Changed 6 months ago by gk

Keywords: TorBrowserTeam201906 added; TorBrowserTeam201905 removed

Moving tickets to June

comment:2 Changed 5 months ago by gk

Keywords: TorBrowserTeam201907 added; TorBrowserTeam201906 removed

Moving tickets to July

comment:4 Changed 5 months ago by sisbell

Cc: gk added

comment:5 Changed 5 months ago by gk

Keywords: TorBrowserTeam201907R added; TorBrowserTeam201907 removed

comment:6 Changed 5 months ago by cypherpunks

-org.gradle.jvmargs=-Xmx1536m?

comment:7 Changed 5 months ago by gk

Keywords: tbb-9.0-must-nightly added

Starting with 9.0 keywords

comment:8 Changed 4 months ago by gk

Keywords: TorBrowserTeam201908R added; TorBrowserTeam201907R removed

No July any longer.

comment:9 Changed 4 months ago by pili

Sponsor: Sponsor44-can

Adding Sponsor 44 to ESR68 tickets

comment:10 Changed 3 months ago by gk

Keywords: TorBrowserTeam201908 added; TorBrowserTeam201908R removed
Status: needs_reviewneeds_revision

Here are some comments:

1) Where is the remove-vpn.patch coming from? I doubt that it applies to our current HEAD. If we need to have patches for tor-android-service, please provide them against our repo. That's the canonical one now.

2) I doubt we still need export GRADLE_MAVEN_REPO="file://$gradle_repo" as the patches relying on that are gone?

3) How do I re-create the dependencies list? I tried now for a while but still failed. Please update the how-to-create-gradle-dependencies-list.txt so I can verify your work.

comment:11 in reply to:  10 Changed 3 months ago by sisbell

Replying to gk:

Here are some comments:

1) Where is the remove-vpn.patch coming from? I doubt that it applies to our current HEAD. If we need to have patches for tor-android-service, please provide them against our repo. That's the canonical one now.

I'll submit a merge request for the changes. The tor-android-service on my branch has moved the vpn into its own module, so this patch will remove it entirely from the build.

2) I doubt we still need export GRADLE_MAVEN_REPO="file://$gradle_repo" as the patches relying on that are gone?

Good point, we are now using command line to set repo. I'll remove this line.

3) How do I re-create the dependencies list? I tried now for a while but still failed. Please update the how-to-create-gradle-dependencies-list.txt so I can verify your work.

I'll add those changes now, but basically with gradle 4.10 download logs only show up in debug mode and I have a new grep command since the output is very different.

Change firefox/build
export GRADLE_FLAGS="--no-daemon --offline --debug"

cat logs/firefox-android-x86.log | grep "Performing HTTP" | grep -o "https://.*" | sort | uniq > downloads.log

comment:12 Changed 3 months ago by sisbell

Status: needs_revisionneeds_review

I need to get review and merge of tor-android-service. Branch 0801a contain all the latest changes from orbot to tor-android-service. There are a lot of changes here since I've been keeping up with the work going into Orbot.

https://github.com/sisbell/tor-android-service/commits/0801a

Revision 5c9583cf8e2b7002f71c2971aae7ebb44c067b42 on July 17th is the minimum I think we can use, so that would cut down a lot of review time. The changes after are not so critical for us.

How do we want to do this?

comment:13 in reply to:  12 ; Changed 3 months ago by gk

Replying to sisbell:

I need to get review and merge of tor-android-service. Branch 0801a contain all the latest changes from orbot to tor-android-service. There are a lot of changes here since I've been keeping up with the work going into Orbot.

https://github.com/sisbell/tor-android-service/commits/0801a

Revision 5c9583cf8e2b7002f71c2971aae7ebb44c067b42 on July 17th is the minimum I think we can use, so that would cut down a lot of review time. The changes after are not so critical for us.

Okay. I can see how that particular commit is relevant for us but I am not so sure about the previous ones. What about we pick just that one for now to unblock our nightly and alpha builds and think about the other ones later on?

How do we want to do this?

We can discuss this in one of our team meetings but here is how I think we should do it: we treat tor-android-service as any of our other project. That means proposed changes need a ticket on our bug tracker (right now this is Trac but it might become Gitlab or $foo). Then someone needs to write a patch, point to a branch (be that one on our own infrastructure, or Github, Gitlab or...) and find someone to review and merge the changes.

The patch should a) be based on our master branch as we own the canonical repo now and b) it should be written in a way that we don't have to suddenly apply patches to any of our other projects. E.g. it's nice to redo the VPN support part but the resulting patch should be written in a way that we not have to suddenly apply tor-browser-build patches to rip things out there. More precisely, it would be written in a way that projects can make use of VPN support but others can safely ignore that part to reduce attack surface etc. by setting a compile time option to just not compile that part in in the first place.

Does that make sense? If so, please file new tickets for the changes we should make in tor-android-service, point to the patches (please base them on our master branch to make merging easier) and set the tickets into needs_review.

comment:14 Changed 3 months ago by gk

Status: needs_reviewneeds_revision

comment:15 in reply to:  13 Changed 3 months ago by sisbell

Replying to gk:

Replying to sisbell:

I need to get review and merge of tor-android-service. Branch 0801a contain all the latest changes from orbot to tor-android-service. There are a lot of changes here since I've been keeping up with the work going into Orbot.

https://github.com/sisbell/tor-android-service/commits/0801a

Revision 5c9583cf8e2b7002f71c2971aae7ebb44c067b42 on July 17th is the minimum I think we can use, so that would cut down a lot of review time. The changes after are not so critical for us.

Okay. I can see how that particular commit is relevant for us but I am not so sure about the previous ones. What about we pick just that one for now to unblock our nightly and alpha builds and think about the other ones later on?

How do we want to do this?

We can discuss this in one of our team meetings but here is how I think we should do it: we treat tor-android-service as any of our other project. That means proposed changes need a ticket on our bug tracker (right now this is Trac but it might become Gitlab or $foo). Then someone needs to write a patch, point to a branch (be that one on our own infrastructure, or Github, Gitlab or...) and find someone to review and merge the changes.

The patch should a) be based on our master branch as we own the canonical repo now and b) it should be written in a way that we don't have to suddenly apply patches to any of our other projects. E.g. it's nice to redo the VPN support part but the resulting patch should be written in a way that we not have to suddenly apply tor-browser-build patches to rip things out there. More precisely, it would be written in a way that projects can make use of VPN support but others can safely ignore that part to reduce attack surface etc. by setting a compile time option to just not compile that part in in the first place.

Does that make sense? If so, please file new tickets for the changes we should make in tor-android-service, point to the patches (please base them on our master branch to make merging easier) and set the tickets into needs_review.

I think this makes sense but I don't think we would have time before first alpha. What we can do is just use latest tor-android-service from tor repo and I'll modify firefox to use it. It will also require regenerating the dependencies.

comment:16 Changed 3 months ago by sisbell

Status: needs_revisionneeds_review

comment:17 Changed 3 months ago by sisbell

There will also need to be a small change in firefox project: removing the android-jsocks.patch . Once that is done everything will build for the release.

comment:18 Changed 3 months ago by cypherpunks

+minVersion=21?

comment:19 Changed 3 months ago by gk

Keywords: tbb-9.0-must-alpha added; tbb-9.0-must-nightly removed

Move must-nightly items to must-alpha ones.

comment:20 Changed 3 months ago by gk

Keywords: TorBrowserTeam201909 added; TorBrowserTeam201908 removed

Moving must-alpha tickets to September.

comment:21 Changed 2 months ago by gk

Keywords: TorBrowserTeam201909R added; TorBrowserTeam201909 removed

comment:22 Changed 2 months ago by cypherpunks

require-api.patch obsolete?

comment:23 Changed 2 months ago by pili

Keywords: TorBrowserTeam201910R added; TorBrowserTeam201909R removed

We're now in October, moving September outstanding reviews to October

comment:24 in reply to:  22 Changed 2 months ago by sisbell

Replying to cypherpunks:

require-api.patch obsolete?

Yes, when we upgraded the support libraries for esr68, we no longer need this patch.

comment:25 Changed 2 months ago by sisbell

I'll need to add testing of the require-api patch to confirm its safe to remove. If so, I'll open another issue for that.

comment:26 in reply to:  18 Changed 2 months ago by sisbell

Replying to cypherpunks:

+minVersion=21?

minVersion is set to 16 in the gradle command line, overriding the gradle.properties file. So this is ok.

comment:27 in reply to:  25 Changed 2 months ago by gk

Replying to sisbell:

I'll need to add testing of the require-api patch to confirm its safe to remove. If so, I'll open another issue for that.

That's #31981.

comment:28 Changed 2 months ago by sisbell

Are we okay to close this issue?

comment:29 in reply to:  28 ; Changed 2 months ago by gk

Keywords: TorBrowserTeam201910 added; TorBrowserTeam201910R removed
Status: needs_reviewneeds_revision

Replying to sisbell:

Are we okay to close this issue?

Alas, not.

Now that #31568 is out of the way I gave the patch a fresh look. It seems the disable-daemon.patch is not required to build and run the .apks. What was the reason of including it? Please remove it.

Please file tickets for the proposed tor-android-service changes not picked up, so we can get them reviewed.

comment:30 in reply to:  29 Changed 2 months ago by sisbell

The original reason was to avoid the crash when the network was disabled: #31293. Its no longer needed after that fix.

Replying to gk:

Replying to sisbell:

Are we okay to close this issue?

Alas, not.

Now that #31568 is out of the way I gave the patch a fresh look. It seems the disable-daemon.patch is not required to build and run the .apks. What was the reason of including it? Please remove it.

Please file tickets for the proposed tor-android-service changes not picked up, so we can get them reviewed.

comment:32 Changed 2 months ago by sisbell

Status: needs_revisionneeds_review

comment:33 Changed 8 weeks ago by gk

Thanks this looks better. Why are we shipping the maven-local.patch and do not add this change to our repo directly? If we add mavenLocal() as last item then there should not be a functional difference to the status quo for projects fetching the dependencies from any of the other specified repositories.

There are still the tickets to file.

comment:34 in reply to:  33 ; Changed 8 weeks ago by sisbell

From a tor-android-service, mavenLocal is not directly needed but it doesn't hurt to add it directly to the gradle build file for tor build.

Replying to gk:

Thanks this looks better. Why are we shipping the maven-local.patch and do not add this change to our repo directly? If we add mavenLocal() as last item then there should not be a functional difference to the status quo for projects fetching the dependencies from any of the other specified repositories.

There are still the tickets to file.

comment:35 in reply to:  34 ; Changed 8 weeks ago by gk

Replying to sisbell:

From a tor-android-service, mavenLocal is not directly needed but it doesn't hurt to add it directly to the gradle build file for tor build.

What do you mean? If it's not needed then why do we have the additional complexity? If it *is* needed then it seems to me supporting the Tor build case directly in tor-android-service is a plus. Other folks might want to build tor-android-service the same way as we.

Replying to gk:

Thanks this looks better. Why are we shipping the maven-local.patch and do not add this change to our repo directly? If we add mavenLocal() as last item then there should not be a functional difference to the status quo for projects fetching the dependencies from any of the other specified repositories.

There are still the tickets to file.

comment:36 in reply to:  35 ; Changed 8 weeks ago by sisbell

I mean that developers who build tor-android-service outside of the tor-browser-build don't need the mavenLocal entry. Since we do need it in tor-browser-build, adding mavenLocal to tor-android-service is the right way to go.

Replying to gk:

Replying to sisbell:

From a tor-android-service, mavenLocal is not directly needed but it doesn't hurt to add it directly to the gradle build file for tor build.

What do you mean? If it's not needed then why do we have the additional complexity? If it *is* needed then it seems to me supporting the Tor build case directly in tor-android-service is a plus. Other folks might want to build tor-android-service the same way as we.

Replying to gk:

Thanks this looks better. Why are we shipping the maven-local.patch and do not add this change to our repo directly? If we add mavenLocal() as last item then there should not be a functional difference to the status quo for projects fetching the dependencies from any of the other specified repositories.

There are still the tickets to file.

comment:37 in reply to:  36 ; Changed 8 weeks ago by gk

Replying to sisbell:

I mean that developers who build tor-android-service outside of the tor-browser-build don't need the mavenLocal entry. Since we do need it in tor-browser-build, adding mavenLocal to tor-android-service is the right way to go.

You mean in our tor-browser-build environment? What about someone who wants to build tor-android-service without network access as we do? Why should that not be supported directly in tor-android-service? This seems to me a valid usecase we should support in general + it would get us rid of a patch: having patches directly in tor-browser-build is considered a bug we should fix, in particular if we own the repository for the respective project as we do with tor-android-service.

Replying to gk:

Replying to sisbell:

From a tor-android-service, mavenLocal is not directly needed but it doesn't hurt to add it directly to the gradle build file for tor build.

What do you mean? If it's not needed then why do we have the additional complexity? If it *is* needed then it seems to me supporting the Tor build case directly in tor-android-service is a plus. Other folks might want to build tor-android-service the same way as we.

Replying to gk:

Thanks this looks better. Why are we shipping the maven-local.patch and do not add this change to our repo directly? If we add mavenLocal() as last item then there should not be a functional difference to the status quo for projects fetching the dependencies from any of the other specified repositories.

There are still the tickets to file.

comment:38 in reply to:  37 ; Changed 8 weeks ago by sisbell

I'm agreeing with you, let's move mavenLocal to tor-android-service. I'll open the issue on tor-android-service and then remove the patch.

Replying to gk:

Replying to sisbell:

I mean that developers who build tor-android-service outside of the tor-browser-build don't need the mavenLocal entry. Since we do need it in tor-browser-build, adding mavenLocal to tor-android-service is the right way to go.

You mean in our tor-browser-build environment? What about someone who wants to build tor-android-service without network access as we do? Why should that not be supported directly in tor-android-service? This seems to me a valid usecase we should support in general + it would get us rid of a patch: having patches directly in tor-browser-build is considered a bug we should fix, in particular if we own the repository for the respective project as we do with tor-android-service.

Replying to gk:

Replying to sisbell:

From a tor-android-service, mavenLocal is not directly needed but it doesn't hurt to add it directly to the gradle build file for tor build.

What do you mean? If it's not needed then why do we have the additional complexity? If it *is* needed then it seems to me supporting the Tor build case directly in tor-android-service is a plus. Other folks might want to build tor-android-service the same way as we.

Replying to gk:

Thanks this looks better. Why are we shipping the maven-local.patch and do not add this change to our repo directly? If we add mavenLocal() as last item then there should not be a functional difference to the status quo for projects fetching the dependencies from any of the other specified repositories.

There are still the tickets to file.

comment:39 in reply to:  38 ; Changed 7 weeks ago by gk

Replying to sisbell:

I'm agreeing with you, let's move mavenLocal to tor-android-service. I'll open the issue on tor-android-service and then remove the patch.

#32043 got merged. Could you fix the remaining issue (I think the tickets got filed as children of #32069) in tor-browser-build and then we can close the whole Android toolchain upgrade bug, too.

Replying to gk:

Replying to sisbell:

I mean that developers who build tor-android-service outside of the tor-browser-build don't need the mavenLocal entry. Since we do need it in tor-browser-build, adding mavenLocal to tor-android-service is the right way to go.

You mean in our tor-browser-build environment? What about someone who wants to build tor-android-service without network access as we do? Why should that not be supported directly in tor-android-service? This seems to me a valid usecase we should support in general + it would get us rid of a patch: having patches directly in tor-browser-build is considered a bug we should fix, in particular if we own the repository for the respective project as we do with tor-android-service.

Replying to gk:

Replying to sisbell:

From a tor-android-service, mavenLocal is not directly needed but it doesn't hurt to add it directly to the gradle build file for tor build.

What do you mean? If it's not needed then why do we have the additional complexity? If it *is* needed then it seems to me supporting the Tor build case directly in tor-android-service is a plus. Other folks might want to build tor-android-service the same way as we.

Replying to gk:

Thanks this looks better. Why are we shipping the maven-local.patch and do not add this change to our repo directly? If we add mavenLocal() as last item then there should not be a functional difference to the status quo for projects fetching the dependencies from any of the other specified repositories.

There are still the tickets to file.

comment:40 in reply to:  39 ; Changed 7 weeks ago by sisbell

Replying to gk:

Replying to sisbell:

I'm agreeing with you, let's move mavenLocal to tor-android-service. I'll open the issue on tor-android-service and then remove the patch.

#32043 got merged. Could you fix the remaining issue (I think the tickets got filed as children of #32069) in tor-browser-build and then we can close the whole Android toolchain upgrade bug, too.

I'm not clear which issue you are referring too. Is it related to this bug?

comment:41 in reply to:  40 Changed 7 weeks ago by gk

Status: needs_reviewneeds_revision

Replying to sisbell:

Replying to gk:

Replying to sisbell:

I'm agreeing with you, let's move mavenLocal to tor-android-service. I'll open the issue on tor-android-service and then remove the patch.

#32043 got merged. Could you fix the remaining issue (I think the tickets got filed as children of #32069) in tor-browser-build and then we can close the whole Android toolchain upgrade bug, too.

I'm not clear which issue you are referring too. Is it related to this bug?

We wanted to get rid of the maven-Local.patch in tor-browser-build and use the tor-android-service commit instead that contains the fix I merged yesterday.

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

comment:42 Changed 7 weeks ago by gk

As you said in comment:38

I'm agreeing with you, let's move mavenLocal to tor-android-service. I'll open the issue on tor-android-service and then remove the patch.

The "remove the patch" part is still missing.

comment:43 Changed 7 weeks ago by sisbell

Status: needs_revisionneeds_review

comment:44 Changed 7 weeks ago by sisbell

Keywords: TorBrowserTeam201910R added; TorBrowserTeam201910 removed

comment:45 Changed 7 weeks ago by gk

Resolution: fixed
Status: needs_reviewclosed

Looks good, thanks. Cherry-picked to master (commit 11672b3b3a615e7493add5d3b3dae0a7ed960551).

Note: See TracTickets for help on using tickets.