Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Option to build packages with DT_RUNPATH preserved in build-package.sh #2429

Closed
monoidic opened this issue May 15, 2018 · 15 comments
Closed

Comments

@monoidic
Copy link
Contributor

monoidic commented May 15, 2018

(note: requires Android 7.0+)
I feel like this will probably have to be addressed at some point anyway (at the very least near when 6.0 is eventually dropped, as it seems like supporting Android 5 forever isn't a plan). Currently, there is no simple way to build packages with it preserved, which would make not having LD_LIBRARY_PATH in the environment (a common source of issues) possible.
It's not pretty, but is certainly possible to build (some) packages like this by commenting out some lines in the switch in the termux-elf-cleaner sources + recompiling it, commenting some lines that "update" it to the original version in build-package.sh, uncommenting the helpful line containing LDFLAGS+="-Wl,-rpath=$TERMUX_PREFIX/lib -Wl,--enable-new-dtags" plus maybe some other things.
I've had some success with it already (for example, "file" .deb, the binary works with LD_LIBRARY_PATH and LD_PRELOAD unset)
This would, hopefully, make it simpler to create secondary repos for people with Android 7.0 or above (of whom there are likely many, as 8.0 has been out for a while and 9.0 is coming out soon as well) since there are likely people that would not want to deal with LD_LIBRARY_PATH, plus maybe make it easier to find and deal with new issues that may arise from this change.
At the moment, though, by default, certain packages (readline, bash, likely others) refuse to compile with those LDFLAGS with error: cannot find -landroid-support
EDIT: Hmm... Err... Well, some of those errors may have been caused by the missing space in front of the LDFLAGS thingy. Adding that there, at the very least, should fix some dumb mistakes...

Okay, so, in short/note to self: Add option to termux-elf-cleaner to support preserving RUNPATH if needed not removing any DT_ entries other than RPATH (I don't know how to though :< ) and make build-package.sh pass termux-elf-cleaner with said option and add the LDFLAGS if provided with some option (I'll try to take a stab at that, at least)

@monoidic
Copy link
Contributor Author

monoidic commented May 16, 2018

Well, I have a small patch for the build scripts ready. I doubt I should make this a PR since it's more of a rough outline of my idea (and has some stuff that would break the script commented out at the moment). It'd ideally be accompanied by a patch to termux-elf-cleaner to add a --preserverun flag (which, I guess, would make sense to also preserve all the 6.0+ sections)

diff --git a/build-all.sh b/build-all.sh
index 613ce8a7..d2926806 100755
--- a/build-all.sh
+++ b/build-all.sh
@@ -14,14 +14,16 @@ _show_usage () {
 	echo "Build all packages."
 	echo "  -a The architecture to build for: aarch64(default), arm, i686, x86_64 or all."
 	echo "  -d Build with debug symbols."
+	echo "  -p Preserve DT_RUNPATH (requires Android 7.0+)."
 	exit 1
 }
 
-while getopts :a:hdDs option; do
+while getopts :a:hdDps option; do
 case "$option" in
 	a) TERMUX_ARCH="$OPTARG";;
 	d) TERMUX_DEBUG='-d';;
 	h) _show_usage;;
+	p) export PRESERVERUN=true;;
 esac
 done
 shift $((OPTIND-1))
diff --git a/build-package.sh b/build-package.sh
index 6da765f9..9fe3fa4d 100755
--- a/build-package.sh
+++ b/build-package.sh
@@ -190,23 +190,25 @@ termux_step_handle_arguments() {
 
 	# Handle command-line arguments:
 	_show_usage () {
-	    echo "Usage: ./build-package.sh [-a ARCH] [-d] [-D] [-f] [-q] [-s] PACKAGE"
+	    echo "Usage: ./build-package.sh [-a ARCH] [-d] [-D] [-f] [-p] [-q] [-s] PACKAGE"
 	    echo "Build a package by creating a .deb file in the debs/ folder."
 	    echo "  -a The architecture to build for: aarch64(default), arm, i686, x86_64 or all."
 	    echo "  -d Build with debug symbols."
 	    echo "  -D Build a disabled package in disabled-packages/."
 	    echo "  -f Force build even if package has already been built."
+	    echo "  -p Preserve DT_RUNPATH (requires Android 7.0+)."
 	    echo "  -q Quiet build"
 	    echo "  -s Skip dependency check."
 	    exit 1
 	}
-	while getopts :a:hdDfqs option; do
+	while getopts :a:hdDfpqs option; do
 		case "$option" in
 		a) TERMUX_ARCH="$OPTARG";;
 		h) _show_usage;;
 		d) TERMUX_DEBUG=true;;
 		D) local TERMUX_IS_DISABLED=true;;
 		f) TERMUX_FORCE_BUILD=true;;
+		p) local TERMUX_PRESERVERUN=true;;
 		q) export TERMUX_QUIET_BUILD=true;;
 		s) export TERMUX_SKIP_DEPCHECK=true;;
 		?) termux_error_exit "./build-package.sh: illegal option -$OPTARG";;
@@ -429,13 +431,19 @@ termux_step_start_build() {
 
 	local TERMUX_ELF_CLEANER_SRC=$TERMUX_COMMON_CACHEDIR/termux-elf-cleaner.cpp
 	local TERMUX_ELF_CLEANER_VERSION=$(bash -c ". $TERMUX_SCRIPTDIR/packages/termux-elf-cleaner/build.sh; echo \$TERMUX_PKG_VERSION")
-	termux_download \
-		https://raw.githubusercontent.com/termux/termux-elf-cleaner/v$TERMUX_ELF_CLEANER_VERSION/termux-elf-cleaner.cpp \
-		$TERMUX_ELF_CLEANER_SRC \
-		62c3cf9813756a1b262108fbc39684c5cfd3f9a69b376276bb1ac6af138f5fa2
-	if [ "$TERMUX_ELF_CLEANER_SRC" -nt "$TERMUX_ELF_CLEANER" ]; then
-		g++ -std=c++11 -Wall -Wextra -pedantic -Os "$TERMUX_ELF_CLEANER_SRC" -o "$TERMUX_ELF_CLEANER"
-	fi
+
+## update sources to have --preserverun flag?
+#	termux_download \
+#		https://raw.githubusercontent.com/termux/termux-elf-cleaner/v$TERMUX_ELF_CLEANER_VERSION/termux-elf-cleaner.cpp \
+#		$TERMUX_ELF_CLEANER_SRC \
+#		62c3cf9813756a1b262108fbc39684c5cfd3f9a69b376276bb1ac6af138f5fa2
+#	if [ "$TERMUX_ELF_CLEANER_SRC" -nt "$TERMUX_ELF_CLEANER" ]; then
+#		g++ -std=c++11 -Wall -Wextra -pedantic -Os "$TERMUX_ELF_CLEANER_SRC" -o "$TERMUX_ELF_CLEANER"
+#	fi
+
+#	if [ -n $TERMUX_PRESERVERUN ]; then
+#		TERMUX_ELF_CLEANER="$TERMUX_ELF_CLEANER --preserverun"
+#	fi
 
 	if [ -n "$TERMUX_PKG_BUILD_IN_SRC" ]; then
 		echo "Building in src due to TERMUX_PKG_BUILD_IN_SRC being set" > "$TERMUX_PKG_BUILDDIR/BUILDING_IN_SRC.txt"
@@ -555,7 +563,9 @@ termux_step_setup_toolchain() {
 	export STRIP=$TERMUX_HOST_PLATFORM-strip
 
 	# Android 7 started to support DT_RUNPATH (but not DT_RPATH), so we may want
-	# LDFLAGS+="-Wl,-rpath=$TERMUX_PREFIX/lib -Wl,--enable-new-dtags"
+	if [ -n "${TERMUX_PRESERVERUN:=""}" ]; then
+		LDFLAGS+=" -Wl,-rpath=$TERMUX_PREFIX/lib -Wl,--enable-new-dtags"
+	fi
 	# and no longer remove DT_RUNPATH in termux-elf-cleaner.
 
 	if [ "$TERMUX_ARCH" = "arm" ]; then

@monoidic
Copy link
Contributor Author

monoidic commented May 18, 2018

Good news! As of git head 26fc6e5 (i.e the current latest as of this moment), everything (except for Tor, patch below) managed to compile. cmake didn't want to at first, but after using the newer cmake on the host added with a recent update to build-package.sh and using that instead, it did. I'm hoping to stick the debs in a repository soon as well as actually test them out.
Didn't seem like PF_FILE was defined and including socket.h|libr/sflib/common/sftypes.h didn't work, so I just defined it the way it is in both of them. Slightly hacky and may be directly related to some change I made, so I'll leave that here as well.

removed patch goes here (unneeded)

@Grimler91
Copy link
Member

@monoidic which arch did you test build for?

@monoidic
Copy link
Contributor Author

monoidic commented May 18, 2018

AArch64. I guess I can't really test with other archs anyway and it's the most common one, though at least seeing if they compile might be good.
Well... It seems to work for some packages and not for some. Simple testing with readelf and binaries in $PREFIX/usr/bin in the docker image:
Working:
for i in /data/data/com.termux/files/usr/bin/*; do readelf -d $i 2> /dev/null | grep /data/data/com.termux/files/usr/lib >/dev/null && echo $i; done | wc -l => 813 files.
Not working:
for i in /data/data/com.termux/files/usr/bin/*; do readelf -d $i 2> /dev/null | grep /data/data/com.termux/files/usr/lib >/dev/null || echo $i; done | wc -l => 1774 files.
Probably should do a clean build as well since I have some leftovers in there from i.e nix, but it shouldn't skew things too much.
Among the not-working ones are, well, bash, dash, zsh and apt, for instance, so, not much of a success yet if you can't even really use it without LD_LIBRARY_PATH, which is sort of my goal.
Anyways, if anyone cares, working and not working.
Figuring out something not shared by the packages that work vs the ones that don't could be helpful, I guess.

EDIT: Actually, after starting relatively from scratch, dash, at the very least, seems to work. I'm guessing it was just some taint from the original binaries that were stripped getting packaged or something. Guess I'll try to leave it to build everything for every arch and try to make less dumb mistakes.

@termux termux deleted a comment from tolstman66 May 19, 2018
@monoidic
Copy link
Contributor Author

monoidic commented May 24, 2018

Yeah... I'm really lost.
readelf -d /data/data/com.termux/files/usr/bin/apt | grep RUNPATH
0x000000000000001d (RUNPATH) Library runpath: [/data/data/com.termux/files/usr/lib]
mkdir tmpdir && dpkg-deb -x debs/apt_1.4.8-2_aarch64.deb tmpdir && readelf -d tmpdir/data/data/com.termux/files/usr/bin/apt | grep RUNPATH
Nothing. Hashes don't match either, so it's clearly been modified. It's as if it's stripped by something after packaging. And I don't think it's termux-elf-cleaner doing it, since I neutered it. And it definitely isn't because it's an old deb, since deleting it and rebuilding the package entirely (as an example) doesn't work either.
How'd I even get a working deb before? I must be missing something really obvious.

@tomty89
Copy link
Contributor

tomty89 commented May 24, 2018

Well look into termux_step_massage?

Btw in your patch:

+#	if [ -n $TERMUX_PRESERVERUN ]; then
+#		TERMUX_ELF_CLEANER="$TERMUX_ELF_CLEANER --preserverun"
+#	fi

EDIT: Note that these lines have been commented out as well.

But TERMUX_ELF_CLEANER is executed as a single word:

find . -type f -print0 | xargs -r -0 "$TERMUX_ELF_CLEANER"

So it should have broken like this anyway:

$ oops="apt search"
$ echo termux-elf-cleaner | xargs "$oops"
xargs: apt search: No such file or directory
$

You either unquote it (the dirty way):

$ oops="apt search"
$ echo termux-elf-cleaner | xargs $oops
Sorting... Done
Full Text Search... Done
termux-elf-cleaner/stable 1.2 aarch64
  Cleaner of ELF files for Android

$

Or make it an array:

$ oops=(apt search)
$ echo termux-elf-cleaner | xargs "${oops[@]}"
Sorting... Done
Full Text Search... Done
termux-elf-cleaner/stable 1.2 aarch64
  Cleaner of ELF files for Android

$

@tomty89
Copy link
Contributor

tomty89 commented May 25, 2018

Also [ -n $TERMUX_PRESERVERUN ] is wrong:

$ TERMUX_PRESERVERUN=true
$ [ -n $TERMUX_PRESERVERUN ]; echo $?
0
$ unset TERMUX_PRESERVERUN
$ [ -n $TERMUX_PRESERVERUN ]; echo $?
0
$

Though I don't think you need [ -n "${TERMUX_PRESERVERUN:=""}" ] either:

$ TERMUX_PRESERVERUN=true
$ [ -n "$TERMUX_PRESERVERUN" ]; echo $?
0
$ unset TERMUX_PRESERVERUN
$ [ -n "$TERMUX_PRESERVERUN" ]; echo $?
1
$

@tomty89
Copy link
Contributor

tomty89 commented May 25, 2018

Uhm in build-all.sh:

+	p) export PRESERVERUN=true;;

where PRESERVERUN should have been TERMUX_PRESERVERUN.

But instead of passing on the option via an env var, you should really do it in the way like the d) case.

@monoidic
Copy link
Contributor Author

Err, yeah, I silently fixed that. And also fixed building with "-a all" on build-all.sh (and edited build-package.sh to use -j2 instead of -j$(nproc) so I can run it in the background more, but that's more so that I can just run it in the background more)
Also, currently building with termux-elf-cleaner just "return 0;"-ing since not having it clean stuff is really the goal.

diff --git a/build-all.sh b/build-all.sh
index 613ce8a7..b95b44f2 100755
--- a/build-all.sh
+++ b/build-all.sh
@@ -10,24 +10,27 @@ test -f $HOME/.termuxrc && . $HOME/.termuxrc
 : ${TERMUX_DEBUG:=""}
 
 _show_usage () {
-	echo "Usage: ./build-all.sh [-a ARCH] [-d]"
+	echo "Usage: ./build-all.sh [-a ARCH] [-d] [-p]"
 	echo "Build all packages."
 	echo "  -a The architecture to build for: aarch64(default), arm, i686, x86_64 or all."
 	echo "  -d Build with debug symbols."
+	echo "  -p Preserve DT_RUNPATH (requires Android 7.0+)."
 	exit 1
 }
 
-while getopts :a:hdDs option; do
+while getopts :a:hdDps option; do
 case "$option" in
 	a) TERMUX_ARCH="$OPTARG";;
 	d) TERMUX_DEBUG='-d';;
 	h) _show_usage;;
+	p) export TERMUX_PRESERVERUN=true;;
 esac
 done
+
 shift $((OPTIND-1))
 if [ "$#" -ne 0 ]; then _show_usage; fi
 
-if [[ ! "$TERMUX_ARCH" =~ ^(aarch64|arm|i686|x86_64)$ ]]; then
+if [[ ! "$TERMUX_ARCH" =~ ^(aarch64|arm|i686|x86_64|all)$ ]]; then
 	echo "ERROR: Invalid arch '$TERMUX_ARCH'" 1>&2
 	exit 1
 fi
diff --git a/build-package.sh b/build-package.sh
index ca75e490..ef94b9e5 100755
--- a/build-package.sh
+++ b/build-package.sh
@@ -190,23 +190,25 @@ termux_step_handle_arguments() {
 
 	# Handle command-line arguments:
 	_show_usage () {
-	    echo "Usage: ./build-package.sh [-a ARCH] [-d] [-D] [-f] [-q] [-s] PACKAGE"
+	    echo "Usage: ./build-package.sh [-a ARCH] [-d] [-D] [-f] [-p] [-q] [-s] PACKAGE"
 	    echo "Build a package by creating a .deb file in the debs/ folder."
 	    echo "  -a The architecture to build for: aarch64(default), arm, i686, x86_64 or all."
 	    echo "  -d Build with debug symbols."
 	    echo "  -D Build a disabled package in disabled-packages/."
 	    echo "  -f Force build even if package has already been built."
+	    echo "  -p Preserve DT_RUNPATH (requires Android 7.0+)."
 	    echo "  -q Quiet build"
 	    echo "  -s Skip dependency check."
 	    exit 1
 	}
-	while getopts :a:hdDfqs option; do
+	while getopts :a:hdDfpqs option; do
 		case "$option" in
 		a) TERMUX_ARCH="$OPTARG";;
 		h) _show_usage;;
 		d) TERMUX_DEBUG=true;;
 		D) local TERMUX_IS_DISABLED=true;;
 		f) TERMUX_FORCE_BUILD=true;;
+		p) local TERMUX_PRESERVERUN=true;;
 		q) export TERMUX_QUIET_BUILD=true;;
 		s) export TERMUX_SKIP_DEPCHECK=true;;
 		?) termux_error_exit "./build-package.sh: illegal option -$OPTARG";;
@@ -252,7 +254,8 @@ termux_step_handle_arguments() {
 termux_step_setup_variables() {
 	: "${ANDROID_HOME:="${HOME}/lib/android-sdk"}"
 	: "${NDK:="${HOME}/lib/android-ndk"}"
-	: "${TERMUX_MAKE_PROCESSES:="$(nproc)"}"
+#	: "${TERMUX_MAKE_PROCESSES:="$(nproc)"}"
+	: "${TERMUX_MAKE_PROCESSES:="2"}"
 	: "${TERMUX_TOPDIR:="$HOME/.termux-build"}"
 	: "${TERMUX_ARCH:="aarch64"}" # arm, aarch64, i686 or x86_64.
 	: "${TERMUX_PREFIX:="/data/data/com.termux/files/usr"}"
@@ -261,6 +264,7 @@ termux_step_setup_variables() {
 	: "${TERMUX_PKG_API_LEVEL:="21"}"
 	: "${TERMUX_ANDROID_BUILD_TOOLS_VERSION:="27.0.3"}"
 	: "${TERMUX_NDK_VERSION:="16"}"
+	: "${TERMUX_PRESERVERUN:="true"}"
 
 	if [ "x86_64" = "$TERMUX_ARCH" ] || [ "aarch64" = "$TERMUX_ARCH" ]; then
 		TERMUX_ARCH_BITS=64
@@ -429,6 +433,8 @@ termux_step_start_build() {
 
 	local TERMUX_ELF_CLEANER_SRC=$TERMUX_COMMON_CACHEDIR/termux-elf-cleaner.cpp
 	local TERMUX_ELF_CLEANER_VERSION=$(bash -c ". $TERMUX_SCRIPTDIR/packages/termux-elf-cleaner/build.sh; echo \$TERMUX_PKG_VERSION")
+
+## update sources to have --preserverun flag?
 	termux_download \
 		https://raw.githubusercontent.com/termux/termux-elf-cleaner/v$TERMUX_ELF_CLEANER_VERSION/termux-elf-cleaner.cpp \
 		$TERMUX_ELF_CLEANER_SRC \
@@ -437,6 +443,10 @@ termux_step_start_build() {
 		g++ -std=c++11 -Wall -Wextra -pedantic -Os "$TERMUX_ELF_CLEANER_SRC" -o "$TERMUX_ELF_CLEANER"
 	fi
 
+#	if [ -n $TERMUX_PRESERVERUN ]; then
+#		TERMUX_ELF_CLEANER="$TERMUX_ELF_CLEANER --preserverun"
+#	fi
+
 	if [ -n "$TERMUX_PKG_BUILD_IN_SRC" ]; then
 		echo "Building in src due to TERMUX_PKG_BUILD_IN_SRC being set" > "$TERMUX_PKG_BUILDDIR/BUILDING_IN_SRC.txt"
 		TERMUX_PKG_BUILDDIR=$TERMUX_PKG_SRCDIR
@@ -555,7 +565,9 @@ termux_step_setup_toolchain() {
 	export STRIP=$TERMUX_HOST_PLATFORM-strip
 
 	# Android 7 started to support DT_RUNPATH (but not DT_RPATH), so we may want
-	# LDFLAGS+="-Wl,-rpath=$TERMUX_PREFIX/lib -Wl,--enable-new-dtags"
+	if [ -n $TERMUX_PRESERVERUN ]; then
+		LDFLAGS+=" -Wl,-rpath=$TERMUX_PREFIX/lib -Wl,--enable-new-dtags"
+	fi
 	# and no longer remove DT_RUNPATH in termux-elf-cleaner.
 
 	if [ "$TERMUX_ARCH" = "arm" ]; then

@tomty89
Copy link
Contributor

tomty89 commented May 25, 2018

It's still wrong. If you have "${TERMUX_PRESERVERUN:="true"}", then you would need a switch to disable it but not enable it, no?

You don't need to have a default value assigned to it. The problem was (and still is) merely that you need to quote $TERMUX_PRESERVERUN when you test it, so [ -n "$TERMUX_PRESERVERUN" ], not [ -n $TERMUX_PRESERVERUN ], otherwise it will always return true.

@monoidic
Copy link
Contributor Author

monoidic commented May 25, 2018

I think that was actually because I wanted to force it on at some point so I could be sure I wasn't just forgetting the -p flag...

The problem was (and still is) merely that you need to quote $TERMUX_PRESERVERUN when you test it

Err... Did you test that theory?
./build-package.sh: line 568: TERMUX_PRESERVERUN: unbound variable
Variables HAVE to be set (even if just to "") due to the set -u at the top of the script and I don't plan on mucking with that.

From the manpage:

         -u      Treat unset variables and parameters other than the special parameters "@" and "*" as an error when perform‐   
                 ing  parameter  expansion.  If expansion is attempted on an unset variable or parameter, the shell prints an   
                 error message, and, if not interactive, exits with a non-zero status.                                          

@tomty89
Copy link
Contributor

tomty89 commented May 25, 2018

No, I didn't assume/notice set -u. It was really just about the quoting. If it needs to be set to an empty string, then set it:

$ set -u
$ TERMUX_PRESERVERUN=""
$ [ -n $TERMUX_PRESERVERUN ]; echo $?
0
$ [ -n "$TERMUX_PRESERVERUN" ]; echo $?
1
$ TERMUX_PRESERVERUN=true
$ [ -n $TERMUX_PRESERVERUN ]; echo $?
0
$ [ -n "$TERMUX_PRESERVERUN" ]; echo $?
0
$

@monoidic
Copy link
Contributor Author

monoidic commented May 25, 2018

But instead of passing on the option via an env var, you should really do it in the way like the d) case.

So, like, passing build-package.sh -p $otherargs instead instead of build-package.sh $otherargs from build-all.sh? Hmm, yeah, done done mk.2.

If it needs to be set to an empty string, then set it:

Err... The idea is for it to be "true" (or any non-empty string, but "true" makes it simple to understand) if it is enabled and "" (empty string, returns false for -n tests) otherwise. Checking how TERMUX_DEBUG is defined and used should give you an idea of that.

@tomty89
Copy link
Contributor

tomty89 commented May 25, 2018

Err... The idea is for it to be "true" (or any non-empty string, but "true" makes it simple to understand) if it is enabled and "" (empty string, returns false for -n tests) otherwise. Checking how TERMUX_DEBUG is defined and used should give you an idea of that.

I know right. What I meant was just, if it wasn't set -u, you can either leave it unset or set to empty string for it to be false; but if set -u requires it to be set, so be it. That wasn't my focus. It's just the quoting that you had been missing. Anyway you fixed it in https://0x0.st/s2Tq.patch as well, so I'm done here.

@monoidic
Copy link
Contributor Author

Ah, whatever, I think this is a lost cause, so I'm closing it. Maybe someone can salvage something from this later on, but it doesn't seem to really "work" for me.

@ghost ghost locked and limited conversation to collaborators Oct 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants