这是indexloc提供的服务,不要输入任何密码
Skip to content

enhance(libllvm) Using relative path for sysroot. #25178

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

MohammedKHC0
Copy link
Contributor

@MohammedKHC0 MohammedKHC0 commented Jun 26, 2025

This makes us one step closer to achieve dynamic prefix. (#24407)
A relative path for the sysroot was first discussed on WebAssembly/wasi-sdk#58
And got implemented on the upstream libllvm back in 2020 https://reviews.llvm.org/D76653

@MohammedKHC0 MohammedKHC0 requested a review from finagolfin as a code owner June 26, 2025 08:16
@MohammedKHC0 MohammedKHC0 force-pushed the patch-1 branch 3 times, most recently from 2355d26 to 26696c5 Compare June 26, 2025 08:36
@MohammedKHC0
Copy link
Contributor Author

MohammedKHC0 commented Jun 26, 2025

ERROR: Version mismatch between libllvm and flang. libllvm=20.1.7:1, flang=20.1.7:0
Should i apply TERMUX_PKG_REVISION to flang as well? I guess so.
Edit: I added it to flang.

This makes us one step closer to achieve dynamic prefix. termux#24407
A relative path for the sysroot was first discussed on WebAssembly/wasi-sdk#58
And got implemented on the upstream libllvm back in 2020
https://reviews.llvm.org/D76653
@robertkirkman
Copy link
Contributor

Very good idea! I really think this is a very cool idea and I hope it works.

There is just one potential problem, which is that I am pretty sure some applications that are packaged in Termux cannot work properly if there are relative paths in some of their internal directories.

When it finishes I will try testing your PR and I will let you know what happens and if anything has an error!

@MohammedKHC0
Copy link
Contributor Author

MohammedKHC0 commented Jun 26, 2025

@robertkirkman Glad that you liked the idea.

There is just one potential problem, which is that I am pretty sure some applications that are packaged in Termux cannot work properly if there are relative paths in some of their internal directories.

I think that there should be no problems because llvm automaticlly converts the sysroot to absolute path.
From https://reviews.llvm.org/D76653#change-zuw0O32XbYXO:

if ((!SysRoot.empty()) && llvm::sys::path::is_relative(SysRoot)) {
    // Prepend InstalledDir if SysRoot is relative
    SmallString<128> P(InstalledDir);
    llvm::sys::path::append(P, SysRoot);
    SysRoot = std::string(P);
}

SysRoot is set to the absolute path. So there shouldn't be any side effects.
But feel free to correct me if i am wrong.

@MohammedKHC0
Copy link
Contributor Author

The CI finally finished .. After only 5 hours 🥴

@TomJo2000
Copy link
Member

The CI finally finished .. After only 5 hours 🥴

That is to be expected.

Copy link
Contributor

@robertkirkman robertkirkman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tested this with building some packages on-device and some software both with and without build-package.sh, and I actually don't see any problems introduced by this. Good job!

I thought at first that these commands might be able to produce a problem:

cd $HOME
mkdir -p code
cd code
cp $PREFIX/bin/clang-20 .
./clang-20 -c test.c

however, those commands currently produce an error no matter whether this PR is used or not, so if this PR helps you, it is good since it doesn't seem to break anything I have found so far.

Copy link
Member

@finagolfin finagolfin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bit busy right now, give me some time to look into this next week.

@agnostic-apollo
Copy link
Member

Busy with this? Ewww!

https://forums.swift.org/t/announcing-the-android-workgroup/80666

@MohammedKHC0 MohammedKHC0 requested a review from finagolfin June 27, 2025 09:51
@MohammedKHC0
Copy link
Contributor Author

That's alright. Take your time.

@agnostic-apollo
Copy link
Member

agnostic-apollo commented Jun 27, 2025

Before this and even now, this is assuming that usr merge format is being used, and $TERMUX__ROOTFS != $TERMUX__PREFIX, otherwise default path would get set to parent directory of rootfs. Relative path should be set depending on if they are equal or not.

I assume lib/ and usr/lib/ paths under sysroot are both looked by llvm.

@finagolfin
Copy link
Member

Busy with this?

Let us know when the rewrite of Termux-app in Swift starts, looking forward to trying it. 😛

@agnostic-apollo
Copy link
Member

Eww eww ewww!!! Don't even say those words! Gosh!

@MohammedKHC0
Copy link
Contributor Author

MohammedKHC0 commented Jun 27, 2025

Before this and even now, this is assuming that usr merge format is being used, and $TERMUX__ROOTFS != $TERMUX__PREFIX, otherwise default path would get set to parent directory of rootfs. Relative path should be set depending on if they are equal or not.

I assume lib/ and usr/lib/ paths under sysroot are both looked by llvm.

I don't think that i completely understand but is this right?

Installed dir (The path) Sysroot
/data/data/com.termux/files/usr/bin /data/data/com.termux/files
/data/user/10/com.termux/files/usr/bin /data/user/10/com.termux/files
/system/bin /

When it is installed inside /system/bin the sysroot should be /system maybe?
Is this what do you mean or did i completely get it wrong?

I tried to search for usages for TERMUX__ROOTFS inside packages to understand better but I didn't find any

@agnostic-apollo
Copy link
Member

I have seen certain forks use something like /data/data/com.termux/files/{bin,home,lib} and there is no usr directory. That normally shouldn't be used as distros have moved to usr merge format, where /bin is symlink to /usr/bin, etc. We have always used the right structure so didn't need to switch.

So if both TERMUX__PREFIX and TERMUX__ROOTFS are set to /data/data/com.termux/files, then going up two directories from /data/data/com.termux/files/bin would use /data/data/com.termux as sysroot instead of /data/data/com.termux/files.

So if prefix and rootfs are equal, then go one directory up .., otherwise two ../... That should probably work.

When it is installed inside /system/bin the sysroot should be /system maybe?

Yes.

I tried to search for usages for TERMUX__ROOTFS inside packages to understand better but I didn't find any

Yeah, rootfs variable isn't really used other than some termux internal packages as it was recently added and packages are more concerned with bin/lib, etc and those are referred by prefix variable.

@MohammedKHC0
Copy link
Contributor Author

MohammedKHC0 commented Jun 27, 2025

@agnostic-apollo I applied the changes that you suggested.

if [ "$TERMUX__PREFIX" = "$TERMUX__ROOTFS" ]; then
    DEFAULT_SYSROOT=".."
else
    DEFAULT_SYSROOT="../.."
fi

@agnostic-apollo
Copy link
Member

Thanks, that should work, I'll let package maintainers test it further.

@MohammedKHC0
Copy link
Contributor Author

@finagolfin Sorry for the ping. But what do you think?
I also think that the binaries run path should be relative to $ORIGIN
(Kinda like how you used patchelf to patch the run path on https://github.com/finagolfin/swift-android-sdk ;) )

Finally, copy libc++_shared.so from the NDK and modify the cross-compiled Swift corelibs to include $ORIGIN and other relative directories in their rpaths:

cp /home/yourname/android-ndk-r27c/toolchains/llvm/prebuilt/linux-x86_64/sysroot/usr/lib/aarch64-linux-android/libc++_shared.so swift-release-android-aarch64-24-sdk/usr/lib
patchelf --set-rpath $ORIGIN/../..:$ORIGIN swift-release-android-aarch64-24-sdk/usr/lib/swift/android/lib*.so

@finagolfin
Copy link
Member

I'll take a look this week, got delayed.

@finagolfin
Copy link
Member

OK, got some time today, will take a look.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants