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 1 commit 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.

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.

4 participants