Opened 3 weeks ago

Closed 7 days ago

#32191 closed defect (fixed)

when cross-compiling, lzma and zstd will be detected on build system

Reported by: eighthave Owned by: teor
Priority: Medium Milestone: Tor: 0.4.2.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: Android, 042-backport, BugSmashFund
Cc: n8fr8, gk, sisbell, nickm Actual Points: 0.2
Parent ID: Points: 0.2
Reviewer: nickm Sponsor:

Description

When cross-compiling, for example for Android, ./configure will detect lzma and zstd setups from the build system, when it should be ignoring them. It should only look in the NDK and --prefix values.

$ sudo apt install liblzma-dev
$ export host_triplet=armv7a-linux-androideabi
$ export EXTERNAL_ROOT=`pwd`/external
$ export platform=21
$ export CC=${host_triplet}${platform}-clang
$ export PATH=$ANDROID_NDK_HOME/toolchains/llvm/prebuilt/linux-x86_64/bin:$PATH
$ ./configure --host="$host_triplet" --enable-android --enable-static-libevent --with-libevent-dir=$EXTERNAL_ROOT --enable-static-openssl --with-openssl-dir=$EXTERNAL_ROOT --prefix=$EXTERNAL_ROOT

[snip]

  CC       src/lib/evloop/compat_libevent.o
src/lib/compress/compress_lzma.c:24:18: fatal error: lzma.h: No such file or directory
 #include <lzma.h>
                  ^
compilation terminated.

The workaround is to add --disable-lzma --disable-zstd.

Child Tickets

TicketStatusOwnerSummaryComponent
#32298closedmake pkg-config a hard requirement for Android builds, since lzma requires itCore Tor/Tor

Change History (34)

comment:1 Changed 3 weeks ago by gk

Cc: gk sisbell added

comment:2 Changed 3 weeks ago by nickm

Keywords: 042-backport added
Milestone: Tor: 0.4.3.x-final

(These are libraries that we detect with pkg-config; an appropriate solution would probably depend on how pkg-config expects people to handle this.)

comment:3 Changed 3 weeks ago by eighthave

That reminds me, I saw an issue when running all these Android builds where ./configure --enable-lzma would fail to include without stopping with an error when running the build without pkg-config. I don't remember the exact details, unfortuately. ./configure would print out an error, but it would continue with the whole build, and the resulting tor binary did not have lzma.

comment:4 in reply to:  2 Changed 3 weeks ago by teor

Replying to nickm:

(These are libraries that we detect with pkg-config; an appropriate solution would probably depend on how pkg-config expects people to handle this.)

You can tell pkg-config to look in your cross-compile root using $PKG_CONFIG_PATH, $PKG_CONFIG_LIBDIR, and $PKG_CONFIG_SYSROOT_DIR:
https://linux.die.net/man/1/pkg-config

But I'm not even sure how you got to this stage.

lzma is not enabled by default:
https://gitweb.torproject.org/tor.git/tree/configure.ac#n1077

So how did it get enabled with that configure command line?
Did you edit configure, or change env vars?

comment:5 in reply to:  3 Changed 3 weeks ago by teor

Replying to eighthave:

That reminds me, I saw an issue when running all these Android builds where ./configure --enable-lzma would fail to include without stopping with an error when running the build without pkg-config. I don't remember the exact details, unfortuately. ./configure would print out an error, but it would continue with the whole build, and the resulting tor binary did not have lzma.

You reported that issue here:
https://trac.torproject.org/projects/tor/ticket/31922#comment:description

It is fixed in 0.4.2 and later, by logging a detailed warning message when --enable-lzma fails:
https://gitweb.torproject.org/tor.git/tree/configure.ac#n1096

We couldn't change it to an error, because that would break existing build scripts.

comment:6 Changed 3 weeks ago by eighthave

On a plain Debian/buster system, running the commands in the issue report should reproduce this. None of the PKG_CONFIG_* env vars are set. Seems to me that if ./configure depends on pkg-config, it should set those env vars when cross-compiling. Or it would probably be enough to use the value set by --prefix=. That's in effect what we are doing in our complete Android builds:

https://gitlab.com/guardianproject/tor-android/blob/c7953925/external/Makefile#L255

It would not be too much work for me to setup a cross-compiling test CI job in gitlab-ci if you would use it. I don't really know how to do that in travis, since you can start with a plain base image, AFAIK.

comment:7 Changed 3 weeks ago by eighthave

Or maybe skip pkg-config all together when cross-compiling, it seems to cause more problems than fix in that case. The dependencies are never going to come from the system.

comment:8 in reply to:  7 Changed 3 weeks ago by teor

Replying to eighthave:

Or maybe skip pkg-config all together when cross-compiling, it seems to cause more problems than fix in that case. The dependencies are never going to come from the system.

Do the libraries in your cross-compiling root contain pkg-config files?
They end with ".pc".

comment:9 Changed 3 weeks ago by eighthave

from https://github.com/guardianproject/tor-android

tor-android $ ls -1 external/lib/pkgconfig/
libcrypto.pc
libevent_core.pc
libevent_extra.pc
libevent_openssl.pc
libevent.pc
libevent_pthreads.pc
liblzma.pc
libssl.pc
libzstd.pc
openssl.pc

comment:10 Changed 3 weeks ago by teor

Ok, then try these env vars, and let us know how you go:

  • PKG_CONFIG_SYSROOT_DIR=external

If that doesn't work:

  • PKG_CONFIG_LIBDIR=
  • PKG_CONFIG_PATH=

comment:11 in reply to:  6 Changed 3 weeks ago by teor

Replying to eighthave:

It would not be too much work for me to setup a cross-compiling test CI job in gitlab-ci if you would use it. I don't really know how to do that in travis, since you can start with a plain base image, AFAIK.

As far as I know, the network team monitors Jenkins, Travis, and Appveyor CI. That's enough for now. We might move to GitLab after the ticket tracker transition. Or a few people might be using GitLab for their own builds.

If you want to contribute GitLab configs, that's fine, but you should also find someone to run and monitor them.

comment:12 Changed 3 weeks ago by eighthave

PKG_CONFIG_SYSROOT_DIR doesn't seem right because it includes the prefix e.g. /usr. Here's the snipped from the docs:

"Modify -I and -L to use the directories located in target sysroot. this option is usefull when crosscompiling package that use pkg-config to determine CFLAGS anf LDFLAGS. -I and -L are modified to point to the new system root. this means that a -I/usr/include/libfoo will become -I/var/target/usr/include/libfoo with a PKG_CONFIG_SYSROOT_DIR equal to /var/target (same rule apply to -L)"

export PKG_CONFIG_PATH=$EXTERNAL_ROOT/lib/pkgconfig has been working in the tor-android project.

About gitlab-ci, I have the .gitlab-ci.yml updates already done. And they don't have to be monitored by the network team to be useful. They'll take affect on anyone who has a tor fork in a gitlab instance. That said, gitlab-ci can be set up like Travis-CI and Appveyor in GitHub pull requests.

comment:13 in reply to:  12 Changed 3 weeks ago by teor

Replying to eighthave:

PKG_CONFIG_SYSROOT_DIR doesn't seem right because it includes the prefix e.g. /usr. Here's the snipped from the docs:

"Modify -I and -L to use the directories located in target sysroot. this option is usefull when crosscompiling package that use pkg-config to determine CFLAGS anf LDFLAGS. -I and -L are modified to point to the new system root. this means that a -I/usr/include/libfoo will become -I/var/target/usr/include/libfoo with a PKG_CONFIG_SYSROOT_DIR equal to /var/target (same rule apply to -L)"

export PKG_CONFIG_PATH=$EXTERNAL_ROOT/lib/pkgconfig has been working in the tor-android project.

Ok, let's work out how to set PKG_CONFIG_PATH in configure?

comment:14 Changed 3 weeks ago by eighthave

I'd set it like this in --enable-android: PKG_CONFIG_PATH=$prefix/lib/pkgconfig

Last edited 3 weeks ago by eighthave (previous) (diff)

comment:15 Changed 3 weeks ago by teor

Status: newneeds_revision

I'd like a solution that works for all cross-compiles, if possible.

comment:16 Changed 3 weeks ago by eighthave

I think it makes sense to require setting --prefix for cross-compiling, set PKG_CONFIG_PATH=$prefix/lib/pkgconfig whenever cross-compiling. I've taken that approach now with tor-android, then each library is installed using make install:
https://github.com/guardianproject/tor-android/blob/c795392511e5d2a21be14f6096f2c17a6e75f989/external/Makefile#L171

I'm guessing the MinGW/Windows builds us a cross-compile too, I'm not aware of Tor's Windows build setup, so I can't speak to that, though I have maintained autotools build systems for Windows before.

comment:17 Changed 3 weeks ago by teor

Sounds good.

Let's think carefully about how to avoid breaking existing build setups.

Here's one possible design:

  1. If prefix is set, and PKG_CONFIG_PATH is not set, use prefix to derive a value for PKG_CONFIG_PATH

That should minimise breakage.

Normally, we'd add the new path to the front of the list of paths, but that could cause subtle bugs. (Like enabling the wrong lzma.)

I also wonder if Windows and some other cross-compiles want to use PKG_CONFIG_SYSROOT_DIR instead. We'll need to check what Tor Browser is doing.

comment:18 Changed 3 weeks ago by eighthave

"If prefix is set, and PKG_CONFIG_PATH is not set, use prefix to derive a value for PKG_CONFIG_PATH" seems fine with me.

comment:19 Changed 3 weeks ago by teor

Great!

Would you like to do a pull request?

(Let's deal with the GitLab and lzma issues you've mentioned in other tickets.)

comment:20 Changed 3 weeks ago by eighthave

Ok, I'll do the pull request

comment:21 Changed 3 weeks ago by eighthave

Now that I think about this, I think this could break some setups on UNIX since those might rely on the pkg-config PATH having multiple paths in it, and when cross-compiling, there should only be --prefix. It needs to be set only when cross-compiling. So I think this needs to set based on these conditions

  • if --host is set for cross-compiling
  • if --prefix is set
  • if PKG_CONFIG_PATH is not set

comment:22 Changed 2 weeks ago by eighthave

Status: needs_revisionneeds_review

PR: https://github.com/torproject/tor/pull/1481
branch: pkg-config-prefix

comment:23 Changed 2 weeks ago by dgoulet

Reviewer: teor

teor seems like the natural reviewer due to previous comments.

comment:24 Changed 2 weeks ago by teor

Thanks!

I left a comment to check an implementation detail, and to ask for an unnecessary check to be removed.

comment:25 Changed 2 weeks ago by teor

Status: needs_reviewneeds_revision

comment:26 Changed 2 weeks ago by eighthave

Can you provide an example of where that check is wrong? libevent is a hard requirement for building, and the idea here is that this is testing that the user ran make install into prefix for libevent. In that case, libevent.pc will be present. This error points out incomplete or missing installs, and is only enabled in very specific cases than can be overridden. This check is more isolated than enable_tool_name_check, and that check is totally broken on Android and iOS #32334.

comment:27 Changed 2 weeks ago by eighthave

Status: needs_revisionnew

I don't think it makes sense for me to contribute any more patches to the tor build system, my experience and point of view seems to only conflict with the opinions here. It seems that the tor devs have a very different conception of what ./configure should provide that is specific to tor. In my contributions to gnupg, openssl, ffmpeg, puredata, and others, I've built an understanding that ./configure is meant to provide a clean build interface for consumers first and foremost. Providing useful errors is an essential part of that. I've never worked on a build system that put all the possible customization options first, before the simple build.

comment:28 Changed 2 weeks ago by eighthave

We're going to maintain the Android and iOS build systems in this fork:

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

comment:29 Changed 8 days ago by teor

Cc: nickm added
Keywords: Android 042-backportAndroid, 042-backport
Owner: set to teor
Reviewer: teornickm
Status: newassigned
Type: defectenhancement

I made the requested changes, and cherry-picked to 0.4.2. Here are the PRs:

Test branches:

Nick, do you think we should backport this build fix to 0.4.2?

comment:30 Changed 8 days ago by nickm

Nick, do you think we should backport this build fix to 0.4.2?

Hm. Can we envision this breaking any reasonable build that would previously have succeeded? If not, I think a backport is reasonable.

comment:31 Changed 8 days ago by eighthave

Happy to see this stuff merged. At this point for the Android builds, we'll be working out of our fork for 0.4.1.x and 0.4.2.x. So we won't need the backports.

comment:32 in reply to:  30 Changed 8 days ago by teor

Status: assignedneeds_review

Replying to nickm:

Nick, do you think we should backport this build fix to 0.4.2?

Hm. Can we envision this breaking any reasonable build that would previously have succeeded? If not, I think a backport is reasonable.

It only sets PKG_CONFIG_PATH when it's unset. So it does override the default path compiled in to the pkg-config binary, but that path is wrong for cross-compiles. It never overrides user-configured paths.

comment:33 Changed 8 days ago by teor

Actual Points: 0.2
Keywords: BugSmashFund added
Points: 0.2
Type: enhancementdefect

comment:34 Changed 7 days ago by nickm

Milestone: Tor: 0.4.3.x-finalTor: 0.4.2.x-final
Resolution: fixed
Status: needs_reviewclosed

Merged to 0.4.2 and forward.

Note: See TracTickets for help on using tickets.