-
Notifications
You must be signed in to change notification settings - Fork 237
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!
@@ -32,7 +32,7 @@ onoe() { | |||
fi | |||
if [[ $# -eq 0 ]] | |||
then | |||
/bin/cat >&2 | |||
cat >&2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can submit this change to Homebrew/brew.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -324,7 +324,7 @@ def check_cellar | |||
def symlink_ld_so | |||
ld_so = HOMEBREW_PREFIX/"lib/ld.so" | |||
return if ld_so.readable? | |||
sys_interpreter = ["/lib64/ld-linux-x86-64.so.2", "/lib/ld-linux.so.3", "/lib/ld-linux.so.2", "/lib/ld-linux-armhf.so.3"].find do |s| | |||
sys_interpreter = ["/lib64/ld-linux-x86-64.so.2", "/lib/ld-linux.so.3", "/lib/ld-linux.so.2", "/lib/ld-linux-armhf.so.3", "/system/bin/linker"].find do |s| | |||
Pathname.new(s).executable? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can submit this change to Linuxbrew/brew.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please submit this change to Linuxbrew/brew in a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -42,7 +42,8 @@ | |||
(OS.mac? ? "~/Library/Logs/Homebrew/" : "~/.cache/Homebrew/Logs")).expand_path | |||
|
|||
# Must use /tmp instead of $TMPDIR because long paths break Unix domain sockets | |||
HOMEBREW_TEMP = Pathname.new(ENV.fetch("HOMEBREW_TEMP", "/tmp")) | |||
# IF ANDROID | |||
HOMEBREW_TEMP = Pathname.new(ENV.fetch("HOMEBREW_TEMP", "/data/data/com.termux/files/usr/tmp")) | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Setting HOMEBREW_TEMP
can go in https://github.com/Linuxbrew/brew/wiki/Android
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to just use $TMPDIR, but the line above dissuaded me.
The strategy I've used in past for such a problem is to create a short symlink (e.g. /tmp/t) that points to the arbitrary-length TMPDIR, then use the shorter path as TMPDIR. Of course, if /tmp isn't writable, it seems best to use TMPDIR as is.
Do you think the above suggestion could fit upstream?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My guess is that they'd prefer to keep the existing behaviour, using HOMEBREW_TEMP
and ignoring TMPDIR
. An Android specific installer for Linuxbrew could set the environment variables needed for Android.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -10,6 +10,9 @@ def locate(tool) | |||
path | |||
elsif File.executable?(path = "/usr/bin/#{tool}") | |||
Pathname.new path | |||
# IF ANDROID | |||
elsif File.executable?(path = "/data/data/com.termux/files/usr/bin/#{tool}") | |||
Pathname.new path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add to https://github.com/Linuxbrew/brew/wiki/Android
ln -s /data/data/com.termux/files/usr/bin/* ~/.linuxbrew/bin/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@@ -79,6 +82,9 @@ def clang_build_version | |||
if (path = locate("clang")) && | |||
build_version = `#{path} --version`[/clang-(\d{2,})/, 1] | |||
Version.new build_version | |||
elsif (path = locate("clang")) && | |||
build_version = `#{path} --version`[/clang version (\d\.\d\.\d)/, 1] | |||
Version.new build_version |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest removing this chunk and changing line 83 to (untested)
build_version = Utils.popen_read(path, "--version")[/clang(-| version )(\d{2,})/, 2]
If clang --version
also displays clang version x.x.x
on Ubuntu, this change can go upstream to Homebrew/brew.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to!
$ clang --version
clang version 4.0.1-6 (tags/RELEASE_401/final)
Target: x86_64-pc-linux-gnu
Thread model: posix
InstalledDir: /usr/bin
$ which clang
/usr/bin/clang
$ dpkg -S /usr/bin/clang
clang: /usr/bin/clang
$ dpkg -l clang
Desired=Unknown/Install/Remove/Purge/Hold
| Status=Not/Inst/Conf-files/Unpacked/halF-conf/Half-inst/trig-aWait/Trig-pend
|/ Err?=(none)/Reinst-required (Status,Err: uppercase=bad)
||/ Name Version Architecture Description
+++-====================-===============-===============-=============================================
ii clang 1:4.0-37~exp3ub amd64 C, C++ and Objective-C compiler (LLVM based)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if keep?(path) | ||
args << "#{wl}--enable-new-dtags" | ||
args << "#{wl}-rpath=" | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change cannot be sent upstream. Since the Android dynamic linker is open source, playing the long game, I'd suggest contributing a patch to Android to add support for DT_RPATH
. Alternatively, I'd suggest the Termux ld
tool enable --enable-new-dtags
by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't imagine Android being amenable to that since "rpath is a design bug". I'll look into option B.
@@ -255,6 +258,7 @@ class Cmd | |||
args += ["#{wl}--dynamic-linker=#{dynamic_linker_path}"] if dynamic_linker_path | |||
args << "-B#{@opt}/glibc/lib" unless mode == :ld | |||
args += path_flags("-L", library_paths) | |||
args << "#{wl}--enable-new-dtags" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto
@@ -99,7 +99,7 @@ brew_repo_version="$(quiet_safe_cd "$SCM_DIR/../../../../bin" && pwd -P)/$SCM_FI | |||
safe_exec "$brew_repo_version" "$@" | |||
|
|||
IFS=$'\n' | |||
for path in $(/usr/bin/which -a "$SCM_FILE" 2>/dev/null) | |||
for path in $(command -v "$SCM_FILE" 2>/dev/null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This patch can go upstream to Homebrew/brew. which
is not an essential utility on Debian and is not installed by default in the fedora
Docker image. command -v
is POSIX and universal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you suggest a separate CL for each of these little bits?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's a CL? I'd suggest a separate PR for each change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, yes. "CL" is our jargon at work for PR. You'll see a bunch of small PRs for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -66,11 +66,12 @@ done | |||
# test-bot does environment filtering itself | |||
if [[ -z "$HOMEBREW_NO_ENV_FILTERING" && "$1" != "test-bot" ]] | |||
then | |||
PATH="/usr/bin:/bin:/usr/sbin:/sbin" | |||
PATH="/usr/bin:/bin:/usr/sbin:/sbin:/usr/bin/applets" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Symlink
|
||
FILTERED_ENV=() | ||
# Filter all but the specific variables. | ||
for VAR in HOME SHELL PATH TERM LOGNAME USER CI CIRCLECI TRAVIS SSH_AUTH_SOCK SUDO_ASKPASS \ | ||
LD_LIBRARY_PATH ANDROID_ROOT ANDROID_DATA \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This patch cannot go upstream. As a workaround, you can use HOMEBREW_NO_ENV_FILTERING=1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hope to convince termux to switch to using DT_RUNPATH, then we don't need this any more.
I added ANDROID_ROOT/DATA to silence some warnings, but I didn't dig into it much, and I suspect they're not essential.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the environment variable to the android wiki page, and suggested upstream that termux enable RUNPATH by default like the rest of Linux. IMHO Homebrew should as well, but I'll leave well enough alone :)
ported from Linuxbrew#621
The --enable-new-dtags option to ld causes it to emit a RUNPATH rather than RPATH entry in the elf header. Because the Android linker supports RUNPATH but not RPATH, this means many things can now Just Work. This should (eventually) eliminate the need to set LD_LIBRARY_PATH. Prior art: This [became the default for Linux in 2013](https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commitdiff;h=822b8bf) but I think termux isn't getting it because it reports "Android" instead of "Linux"? Or maybe the way it works changed since then. Regardless, [Debian has been using RUNPATH by default since December 2016.](https://sources.debian.org/src/binutils/2.27.90.20161231-1/debian/patches/ld-new-dtags-by-default.diff/?hl=27#L27) Their [newest configuration](https://sources.debian.org/src/binutils/2.30-7/debian/rules/#L362) uses the newer configuration option seen in this patch. The [suse and gentoo](https://web.archive.org/web/20160101182307/http://comments.gmane.org/gmane.comp.gnu.binutils/57379) maintainers said they did the same in 2004 and 2005, respectively. So it seems well battle-tested, to me. For the curious: This came up because I'm working on [getting Homebrew working under Termux](Linuxbrew/brew#621).
Between my six pull requests on three projects and the new wiki page, I think I got it all. |
Thanks, Buck! |
The --enable-new-dtags option to ld causes it to emit a RUNPATH rather than RPATH entry in the elf header. Because the Android linker supports RUNPATH but not RPATH, this means many things can now Just Work. This should (eventually) eliminate the need to set LD_LIBRARY_PATH. Prior art: This [became the default for Linux in 2013](https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commitdiff;h=822b8bf) but I think termux isn't getting it because it reports "Android" instead of "Linux"? Or maybe the way it works changed since then. Regardless, [Debian has been using RUNPATH by default since December 2016.](https://sources.debian.org/src/binutils/2.27.90.20161231-1/debian/patches/ld-new-dtags-by-default.diff/?hl=27#L27) Their [newest configuration](https://sources.debian.org/src/binutils/2.30-7/debian/rules/#L362) uses the newer configuration option seen in this patch. The [suse and gentoo](https://web.archive.org/web/20160101182307/http://comments.gmane.org/gmane.comp.gnu.binutils/57379) maintainers said they did the same in 2004 and 2005, respectively. So it seems well battle-tested, to me. For the curious: This came up because I'm working on [getting Homebrew working under Termux](Linuxbrew/brew#621).
ported from Linuxbrew#621
Migrated from Linuxbrew/brew#621
This is the result of discussion in #598
More context here: https://docs.google.com/document/d/1FNMnPTKIrXkloFFoHtTXZl2yAgvXm4G-0hHAd0RbLVY/edit#heading=h.mkvoio42su01