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

fix(build): don't statically link against the "musl" C standard library #4818

Closed
wants to merge 5 commits into from

Conversation

usagi-flow
Copy link
Contributor

@usagi-flow usagi-flow commented Nov 19, 2022

By default, rustc always statically links against musl (*), unlike on glibc systems where the resulting binary is dynamically linked against glibc.

This has a very problematic side effect: Binaries which statically link against musl do not support the dlopen() call (which we do through the "libloading" crate) (**). This in turn makes it impossible to load treesitter grammars.

The result is an error in the log and the user is left without syntax highlighting, among others:

helix_core::syntax [ERROR] Failed to load tree-sitter parser for language "toml": Error opening dynamic library "/opt/helix-src/runtime/grammars/toml.so"

This PR ensures the resulting binary is never statically linked against a C standard library. This configuration should have no impact in environments where the resulting binary would be dynamically linked by default.


(*) Tested with v1.65.0. See also: rust-lang/rust#34987 / rust-lang/compiler-team#422

(**) I found it out by testing a call to Library::new(...).unwrap() in a statically and dynamically linked application. I couldn't find much information regarding my claim, but I found these:
https://www.openwall.com/lists/musl/2012/12/08/4
https://www.openwall.com/lists/musl/2014/08/27/14

I tested it with the following code:

use libloading::Library;

fn main() {
    let name = "/opt/helix-src/runtime/grammars/toml.so";

    println!("Loading: {}", name);
    let lib = unsafe { Library::new(name).unwrap() };

    println!("Unloading: {}", name);
    lib.close().unwrap();

    println!("Done");
}

Output (when built on musl with default options):

Loading: /opt/helix-src/runtime/grammars/toml.so
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: DlOpen { desc: "Dynamic loading not supported" }', src/main.rs:8:4

@kirawi kirawi added C-bug Category: This is a bug A-packaging Area: Packaging and bundling S-waiting-on-review Status: Awaiting review from a maintainer. labels Nov 19, 2022
@archseer
Copy link
Member

This shouldn't be the default for all targets, just musl.

@archseer
Copy link
Member

We should check what the alpine package does since that does build against musl and seems to work fine

@archseer
Copy link
Member

Hmm OK gtk-rs/gtk-rs-core#387 (comment)

@kbridge
Copy link

kbridge commented Mar 11, 2023

I met the same problem today, by (manually) building helix on alpine linux.

And I solved it using the same technique you provided here.

i.e.

export RUSTFLAGS='-C target-feature=-crt-static'

I suggest this be mentioned in the installation chapter of the documentation.

@kbridge
Copy link

kbridge commented Mar 11, 2023

I don't know who to ping. Guess pinging the author is not a bad idea. @archseer

@pascalkuthe
Copy link
Member

I already added a note to the docs about this a while ago. You need to look at the master docs

@usagi-flow
Copy link
Contributor Author

Sorry for the delay, I nearly forgot about the existence of this PR, until I wanted to deploy a new Helix built from source on my raspberry pi nodes...

I changed the configuration file such that the compile flags apply to musl targets only, as suggested by @archseer .
I like that much more than simply including the flags for all builds, but it comes with the downside of having to hardcode different musl targets (I limited myself to common ARM targets used by RPi's, among others, and x86 targets), but the complete list is a bit bigger and probably not constant:

 $ rustc --print target-list | rg musl
aarch64-unknown-linux-musl
arm-unknown-linux-musleabi
arm-unknown-linux-musleabihf
armv5te-unknown-linux-musleabi
armv7-unknown-linux-musleabi
armv7-unknown-linux-musleabihf
hexagon-unknown-linux-musl
i586-unknown-linux-musl
i686-unknown-linux-musl
mips-unknown-linux-musl
mips64-openwrt-linux-musl
mips64-unknown-linux-muslabi64
mips64el-unknown-linux-muslabi64
mipsel-unknown-linux-musl
powerpc-unknown-linux-musl
powerpc64-unknown-linux-musl
powerpc64le-unknown-linux-musl
riscv32gc-unknown-linux-musl
riscv64gc-unknown-linux-musl
s390x-unknown-linux-musl
thumbv7neon-unknown-linux-musleabihf
x86_64-unknown-linux-musl

If someone knows how to configure this in a more elegant way: Be my guest. :)

@usagi-flow usagi-flow changed the title fix(build): never statically link against the C standard library fix(build): don't statically link against the "musl" C standard library Mar 26, 2023
pascalkuthe
pascalkuthe previously approved these changes Jul 7, 2023
Copy link
Member

@pascalkuthe pascalkuthe left a comment

Choose a reason for hiding this comment

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

I think this kind of hard coding is fine. RISCV would be nice to add tough.

Therer are probably very few people compiling for weird architectures and musl and those that do probably know how to add these flags themselves. I think you could actually remove the note about musl from the dos with this PR. It has been confusing some windows people that don't know what musl or glibc are

Copy link

@polarathene polarathene left a comment

Choose a reason for hiding this comment

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

comes with the downside of having to hardcode different musl targets
If someone knows how to configure this in a more elegant way: Be my guest. :)

Just a suggestion for a simpler representation :)

.cargo/config.toml Outdated Show resolved Hide resolved
@usagi-flow
Copy link
Contributor Author

I simplified the configuration as per @polarathene 's suggestion. Also wanted to remove the note from the docs as suggested by @pascalkuthe , but it seems that was done in the meanwhile already.

@the-mikedavis
Copy link
Member

Those docs moved here:

helix/book/src/install.md

Lines 185 to 189 in 585402d

If you are using the `musl-libc` standard library instead of `glibc` the following environment variable must be set during the build to ensure tree-sitter grammars can be loaded correctly:
```sh
RUSTFLAGS="-C target-feature=-crt-static"
```

@pascalkuthe
Copy link
Member

this pr is stale and I actually included it with #8021 as I had to touch the config anyway (which would have made it impossible to configure this manually). Thanks for contributing!

@pascalkuthe pascalkuthe closed this Apr 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-packaging Area: Packaging and bundling C-bug Category: This is a bug S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants