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

WIP: statically linked build for non Alpine Linux x86_64 #33

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

kalinkrustev
Copy link

This was going to work very well, but the recent addition of tree-sitter broke the build, as it has few modules written in C++ and they fail to build with Rust and musl. For two of them I have found the git repo already contained code rewritten in C, so only Ruby didn't, so I disabled it in the fork. I could not find any solution how to build C++ with Rust and musl.


Below is the error related to C++ tree-sitter-ruby code. Similar ones happened for tree-sitter-elixir, tree-sitter-html

  running: "musl-g++" "-O3" "-ffunction-sections" "-fdata-sections" "-fPIC" "-m64" "-I" "src" "-Wall" "-Wextra" "-o" "/home/runner/work/llm-ls/llm-ls/target/x86_64-unknown-linux-musl/release/build/tree-sitter-ruby-7669b38d3e7a5c68/out/src/scanner.o" "-c" "src/scanner.cc"

  --- stderr


  error occurred: Failed to find tool. Is `musl-g++` installed?

@cmosguy
Copy link

cmosguy commented Oct 13, 2023

@kalinkrustev quick question, can I use the target x86_64-unknown-linux-gnu instead of the x86_64-unknown-linux-musl? My local servers do not support that g++ version.

@kalinkrustev
Copy link
Author

kalinkrustev commented Oct 14, 2023

The x86_64-unknown-linux-musl published in the fork works fine on older Ubuntu and probably many other distributions. The error above is a compilation error before removing the Ruby parser.

@@ -22,10 +22,10 @@ tree-sitter-bash = "0.20"
tree-sitter-c = "0.20"
tree-sitter-cpp = "0.20"
tree-sitter-c-sharp = "0.20"
tree-sitter-elixir = "0.1"
Copy link
Member

Choose a reason for hiding this comment

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

What happened here? 😄

Copy link
Author

Choose a reason for hiding this comment

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

As mentioned in the PR description, this version of the module used C++, so I changed it to use the git repository, where the module is rewritten in C, so that it can compile with musl.

Copy link

Choose a reason for hiding this comment

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

This is why the current musl version does not work -- it requires libstdc++.so.6.

Copy link

Choose a reason for hiding this comment

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

Probably should leave a comment above each of these tree-sitter dependencies to state why they are pulling in a specific version from github.

@cmosguy
Copy link

cmosguy commented Oct 16, 2023

@kalinkrustev and @McPatate see my comment, here: #25 (comment) still does not work with older versions of GLIBC

@spew
Copy link

spew commented Feb 7, 2024

I tested the version from @kalinkrustev's fork and was able to get it running fine. What do we need to change to get this PR merged?

@cmosguy
Copy link

cmosguy commented Feb 7, 2024

I tested the version from @kalinkrustev's fork and was able to get it running fine. What do we need to change to get this PR merged?

@McPatate I am really keen on using this fix as trying to orchestrate other forks is a bit cumbersome. Would be grateful if you could expedite the merge. Cheers!

@McPatate
Copy link
Member

McPatate commented Feb 8, 2024

Let me finish up some fixes for 0.5.0 and I'll merge this PR as well before the release :)

Copy link
Member

@McPatate McPatate left a comment

Choose a reason for hiding this comment

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

I'm not convinced we have to modify the CI yaml. The fix looks like it's about the tree sitter crates using C++

Comment on lines +36 to +38
- os: ubuntu-22.04
target: x86_64-unknown-linux-musl
code-target: linux-x64
Copy link
Member

Choose a reason for hiding this comment

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

Why did you add this target?

@spew
Copy link

spew commented Feb 9, 2024

I'm not convinced we have to modify the CI yaml. The fix looks like it's about the tree sitter crates using C++

Maybe we can break out the removal of the tree sitter C++ dependency into it's own PR and then we can reason about the changes seperately.

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