Opened 18 months ago

Closed 7 weeks ago

#28765 closed defect (fixed)

LibEvent Build for Android

Reported by: sisbell Owned by: sisbell
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-mobile, tbb-rbm, tbb-parity, TorBrowserTeam202004R
Cc: sisbell, gk, tbb-team Actual Points:
Parent ID: #28704 Points: 0.25
Reviewer: sysrqb, boklm Sponsor:

Description


Child Tickets

Change History (34)

comment:1 Changed 15 months ago by gk

Keywords: tbb-parity added

tbb-parity items.

comment:2 Changed 8 months ago by sysrqb

Points: 0.25

comment:3 Changed 6 months ago by sysrqb

Keywords: TorBrowserTeam201912 added; TorBrowserTeam201812 removed
Owner: changed from tbb-team to sisbell
Status: newassigned

comment:4 Changed 6 months ago by sysrqb

Cc: tbb-team added

comment:6 Changed 6 months ago by sisbell

Keywords: TorBrowserTeam201912R added; TorBrowserTeam201912 removed
Status: assignedneeds_review

I verified it builds on 4 targets. I updated the version to 2.1.11

https://github.com/sisbell/tor-browser-build/commit/da222654e20ad1878f238fde0eb1f9caed905d53

If we continue with 2.1.8, we get the following error. We can apply a patch if we want to support this older version.

./.libs/libevent_core-2.1.so: error: undefined reference to 'arc4random_addrandom'

I have the other flags because sample and libevent regress are triggering tests which have unsupported classes (older versions of tor-android do the same thing)

test/test-dumpevents.c:104: error: undefined reference to 'stdout'
test/test-dumpevents.c:104: error: undefined reference to 'stderr'

If we need libevent regress, we could patch and remove the tests

AM_CONDITIONAL([BUILD_SAMPLES], [test "$enable_samples" = "yes"])
AM_CONDITIONAL([BUILD_REGRESS], [test "$enable_libevent_regress" = "yes"])

comment:7 Changed 6 months ago by gk

FWIW, I think bumping Libevent to 2.1.11 is the right thing to do (we would squash #31499 while doing so, which is nice).

comment:8 Changed 6 months ago by sysrqb

Reviewer: sysrqb, boklm

comment:9 Changed 5 months ago by sysrqb

Keywords: TorBrowserTeam202001R added; TorBrowserTeam201912R removed

comment:10 Changed 4 months ago by boklm

Keywords: TorBrowserTeam202001 added; TorBrowserTeam202001R removed
Status: needs_reviewneeds_revision

comment:11 Changed 4 months ago by pili

Keywords: TorBrowserTeam202002 added; TorBrowserTeam202001 removed

Moving tickets to February

comment:12 Changed 3 months ago by sysrqb

Replying to sisbell:

Other issues:

  1. There was a suggestion to move some of the fields in configure_opt up to rbm. OpenSSL doesn't use the same configure_host value as other projects so this will require some more discussion if we want to move forward with this suggestion.
  2. Information regarding libevent --disable-libevent-regress--disable-samples. I need to look back through my notes. I'll post in a follow up comment.

From ticket:28704#comment:26, do including these change the size of the resulting library? I'd rather disable these for all platforms in a separate ticket.

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

Replying to sysrqb:

Replying to sisbell:

Other issues:

  1. There was a suggestion to move some of the fields in configure_opt up to rbm. OpenSSL doesn't use the same configure_host value as other projects so this will require some more discussion if we want to move forward with this suggestion.
  2. Information regarding libevent --disable-libevent-regress--disable-samples. I need to look back through my notes. I'll post in a follow up comment.

From ticket:28704#comment:26, do including these change the size of the resulting library? I'd rather disable these for all platforms in a separate ticket.

The tar files themselves are a few bytes off (looks to be due to timestamp differences on files). But after unarchiving, the following directories all have the same size

  • libevent --disable-libevent-regress --disable-samples
  • libevent --disable-libevent-regress 
  • libevent --disable-samples
  • libevent

comment:14 Changed 3 months ago by sisbell

Status: needs_revisionneeds_review

Broke commit into its own branch, otherwise no changes since last commit.

https://github.com/sisbell/tor-browser-build/commits/bug-28765

comment:15 Changed 3 months ago by boklm

Keywords: TorBrowserTeam202002R added; TorBrowserTeam202002 removed

comment:16 in reply to:  14 ; Changed 3 months ago by boklm

Keywords: TorBrowserTeam202002 added; TorBrowserTeam202002R removed
Status: needs_reviewneeds_revision

Replying to sisbell:

Broke commit into its own branch, otherwise no changes since last commit.

https://github.com/sisbell/tor-browser-build/commits/bug-28765

When looking at commit f73f0125252cdf18596291c45ba3d92cb719b883:

You define var/configure_opt to --disable-static in project/libevent/config for all platforms except android. I think this is wrong as var/configure_opt already has some values defined in rbm.conf, so we override those values which will probably break the build on non-android platforms.

By the way in ticket:28704#comment:14 I said this:

it seems we could have a var/configure_opt for android in rbm.conf containing something like CC=clang --host=[% c("var/host") %] [% c("var/configure_opt_project") %], where var/configure_opt_project is defined in each project to define options specific to this project

I still think that would be a good idea. If you don't think so, then please at least explain why instead of just ignoring comments and not doing the changes suggested.

Regarding update to version 2.1.11, I think it would be a good idea to mention it in the commit message. Or maybe do it as a separate commit (with #31499 as the bug number) as this is a change that affect all platforms.

comment:17 in reply to:  16 ; Changed 3 months ago by sisbell

Replying to boklm:

Replying to sisbell:

Broke commit into its own branch, otherwise no changes since last commit.

https://github.com/sisbell/tor-browser-build/commits/bug-28765

When looking at commit f73f0125252cdf18596291c45ba3d92cb719b883:

You define var/configure_opt to --disable-static in project/libevent/config for all platforms except android. I think this is wrong as var/configure_opt already has some values defined in rbm.conf, so we override those values which will probably break the build on non-android platforms.

Ok that should be easy to fix.

By the way in ticket:28704#comment:14 I said this:

it seems we could have a var/configure_opt for android in rbm.conf containing something like CC=clang --host=[% c("var/host") %] [% c("var/configure_opt_project") %], where var/configure_opt_project is defined in each project to define options specific to this project

I still think that would be a good idea. If you don't think so, then please at least explain why instead of just ignoring comments and not doing the changes suggested.

I didn't ignore the comment. i previously stated the reason I didn't do it and requested more discussion around a solution.

"There was a suggestion to move some of the fields in configure_opt up to rbm. OpenSSL doesn't use the same configure_host value as other projects so this will require some more discussion if we want to move forward with this suggestion."

https://trac.torproject.org/projects/tor/ticket/28704#comment:23

Regarding update to version 2.1.11, I think it would be a good idea to mention it in the commit message. Or maybe do it as a separate commit (with #31499 as the bug number) as this is a change that affect all platforms.

comment:18 in reply to:  17 Changed 3 months ago by boklm

Replying to sisbell:

By the way in ticket:28704#comment:14 I said this:

it seems we could have a var/configure_opt for android in rbm.conf containing something like CC=clang --host=[% c("var/host") %] [% c("var/configure_opt_project") %], where var/configure_opt_project is defined in each project to define options specific to this project

I still think that would be a good idea. If you don't think so, then please at least explain why instead of just ignoring comments and not doing the changes suggested.

I didn't ignore the comment. i previously stated the reason I didn't do it and requested more discussion around a solution.

Ah, indeed, I missed that. Sorry.

"There was a suggestion to move some of the fields in configure_opt up to rbm. OpenSSL doesn't use the same configure_host value as other projects so this will require some more discussion if we want to move forward with this suggestion."

I think openssl not using var/configure_opt is not a reason for not using it in other projects. We are in the same situation on other platforms, where we define var/configure_opt, and use it in some projects, and not in others projects expecting different options like openssl.

comment:19 in reply to:  16 Changed 3 months ago by gk

Replying to boklm:

Replying to sisbell:

Broke commit into its own branch, otherwise no changes since last commit.

https://github.com/sisbell/tor-browser-build/commits/bug-28765

When looking at commit f73f0125252cdf18596291c45ba3d92cb719b883:

You define var/configure_opt to --disable-static in project/libevent/config for all platforms except android. I think this is wrong as var/configure_opt already has some values defined in rbm.conf, so we override those values which will probably break the build on non-android platforms.

By the way in ticket:28704#comment:14 I said this:

it seems we could have a var/configure_opt for android in rbm.conf containing something like CC=clang --host=[% c("var/host") %] [% c("var/configure_opt_project") %], where var/configure_opt_project is defined in each project to define options specific to this project

I still think that would be a good idea. If you don't think so, then please at least explain why instead of just ignoring comments and not doing the changes suggested.

Regarding update to version 2.1.11, I think it would be a good idea to mention it in the commit message. Or maybe do it as a separate commit (with #31499 as the bug number) as this is a change that affect all platforms.

+1 for a separate commit before the patch for this ticket (and making sure the other platforms are not exploding)

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

comment:20 Changed 3 months ago by sisbell

#31499 now includes the version change for libevent (I sitll need to test this for non-android platforms)

#33216 contains the CC=clang and host info

This commit now includes using a configure_opt_project variable. I verified this is building correctly for android. Again I still need to test with #31499 to verify all platforms.

https://github.com/sisbell/tor-browser-build/commits/bug-28765a

comment:21 Changed 3 months ago by sisbell

Keywords: TorBrowserTeam202003R added; TorBrowserTeam202002 removed
Status: needs_revisionneeds_review

comment:22 in reply to:  20 Changed 3 months ago by boklm

Keywords: TorBrowserTeam202003 added; TorBrowserTeam202003R removed
Status: needs_reviewneeds_revision

Replying to sisbell:

This commit now includes using a configure_opt_project variable. I verified this is building correctly for android. Again I still need to test with #31499 to verify all platforms.

I think it doesn't work on other platforms: the commit moves the --disable-static from projects/libevent/build to var/configure_opt_project, but the var/configure_opt from other platforms does not use var/configure_opt_project.

I think we should update the var/configure_opt in rbm.conf from all platforms to include var/configure_opt_project.

comment:23 Changed 2 months ago by sisbell

Keywords: TorBrowserTeam202003R added; TorBrowserTeam202003 removed
Status: needs_revisionneeds_review

I updated the other platforms to includevar/configure_opt_projectin the rbm.conf file. Built windows, mac and linux variant to make sure it didn't break

https://github.com/sisbell/tor-browser-build/commits/bug-28765b

comment:24 Changed 2 months ago by pili

Keywords: TorBrowserTeam202004R added; TorBrowserTeam202003R removed

We are no longer in March

comment:25 Changed 2 months ago by sysrqb

Status: needs_reviewneeds_revision

Is configure_opt_project included on linux-x86_64?

comment:26 in reply to:  25 Changed 2 months ago by boklm

Replying to sysrqb:

Is configure_opt_project included on linux-x86_64?

Yes, I think it is missing.

By the way, while doing this change, I think we can simplify configure_opt on linux-i686 by renaming var/configure_opt_i686 to var/configure_opt. I'm not sure why we have this configure_opt_i686, as it doesn't seem to be used anywhere else.

comment:27 Changed 8 weeks ago by boklm

Keywords: TorBrowserTeam202004 added; TorBrowserTeam202004R removed

comment:28 Changed 7 weeks ago by sisbell

Keywords: TorBrowserTeam202004R added; TorBrowserTeam202004 removed
Status: needs_revisionneeds_review

I added the 'configure_opt_project' in rbm.conf for linux-x86_64 so that it will pick now

I removed reference to 'var/configure_opt_i686' and just collapsed it into the 'var/configure_opt' field

https://github.com/sisbell/tor-browser-build/commits/bug-28765c

comment:29 in reply to:  28 Changed 7 weeks ago by boklm

Keywords: TorBrowserTeam202004 added; TorBrowserTeam202004R removed
Status: needs_reviewneeds_revision

Replying to sisbell:

I added the 'configure_opt_project' in rbm.conf for linux-x86_64 so that it will pick now

I removed reference to 'var/configure_opt_i686' and just collapsed it into the 'var/configure_opt' field

https://github.com/sisbell/tor-browser-build/commits/bug-28765c

The patch for #28765 looks good, but is difficult to test since it is based on a version of the patch for #31499 which doesn't build on Linux.

comment:30 Changed 7 weeks ago by sisbell

Keywords: TorBrowserTeam202004R added; TorBrowserTeam202004 removed
Status: needs_revisionneeds_review

I made the changes to #31499 so this is ready for review again.

comment:31 Changed 7 weeks ago by gk

There is still comment:12 unaddressed,no?

comment:32 in reply to:  31 Changed 7 weeks ago by sisbell

Replying to gk:

There is still comment:12 unaddressed,no?

See #33877

comment:33 Changed 7 weeks ago by sisbell

After breaking out #33877, this commit only contains the android config

https://github.com/sisbell/tor-browser-build/commits/bug-28765d

comment:34 Changed 7 weeks ago by boklm

Resolution: fixed
Status: needs_reviewclosed

This is fixed by commit 06bb0f2d4c91d7ac60ae180297d3c0707a004199 (see ticket:33877#comment:5).

Note: See TracTickets for help on using tickets.