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

Build fully statically linked rust-analyzer (docker-based) #4369

Closed
wants to merge 1 commit into from
Closed

Build fully statically linked rust-analyzer (docker-based) #4369

wants to merge 1 commit into from

Conversation

kuznero
Copy link

@kuznero kuznero commented May 7, 2020

A working Dockerfile with build.sh that builds fully statically linked executable.

Addresses #4354.

@matklad
Copy link
Member

matklad commented May 8, 2020

What's the difference here with what we were doing previously, before this commit

77de401

?

@kjeremy
Copy link
Contributor

kjeremy commented May 8, 2020

A top level build.sh is going to lead to confusion.

@kuznero
Copy link
Author

kuznero commented May 8, 2020

@kjeremy agree, but this is just an example of how it can be done. Please suggest the right structure and I will happily change it.

@kuznero
Copy link
Author

kuznero commented May 8, 2020

What's the difference here with what we were doing previously, before this commit

77de401

?

It seems that it should have worked, though some pieces are missing in environment variables and likely zlib (but not absolutely sure about that). What was the reason for this commit? Why musl target was reverted at all?

@lnicola
Copy link
Member

lnicola commented May 8, 2020

It was reverted because musl doesn't support dlopen and we need it for procedural macros.

@kuznero
Copy link
Author

kuznero commented May 8, 2020

Then why does it work with this Dockerfile? It is built without any errors.

@lnicola
Copy link
Member

lnicola commented May 8, 2020

The build works, but it fails at runtime when loading a proc macro.

@kuznero
Copy link
Author

kuznero commented May 8, 2020

Can you please give an example in which exactly scenario such binary will fail in runtime? It seems that I simply don't know enough about proc macros.

@lnicola
Copy link
Member

lnicola commented May 8, 2020

Procedural macro are Rust crates that are built as a shared library. They export a function taking a token stream and returning another. The compiler (and rust-analyzer) load the library and call the function with the source code to expand the macro. If we can't load the library, we can't do that, so crates like serde will not work.

Of course,

  • serde is not fully supported even today
  • there is a Rust musl toolchain on Alpine and I couldn't figure out how it works

See also the discussion here.

By the way, I've never looked into NixOS, but I've always thought that running random binaries from the Internet is somewhat against its philosophy. There's also this, which does seem to download the GitHub release, so I'm probably wrong. Could it be adapted to run patchelf?

@kuznero
Copy link
Author

kuznero commented May 8, 2020

The NixOS philosophy is to ensure executables are packaged to make the use of custom / any libc. But statically linked binaries don't have this issue, and it is OK to download any binary and run for users.

@kuznero
Copy link
Author

kuznero commented May 8, 2020

And in general it sounds like a huge limitation that statically linked binaries unable to use proc macros in runtime. Does this only affect nixos or is it general issue? I cannot find anything on that in the official docs for macros and static linking.

@matklad
Copy link
Member

matklad commented May 8, 2020

@kuznero please see #4354 (comment) for an explanation why static linking does not work for us at the moment.

Also please note that docker is unnecessary for building statically linked rust binaries for pure rust code-bases, which is the case for rust-analyzer.

@matklad matklad closed this May 8, 2020
@kuznero
Copy link
Author

kuznero commented May 8, 2020

@matklad thanks for your comment. Though do you know if this limitation is documented somewhere in the Rust docs?

@edwin0cheng
Copy link
Member

there is a Rust musl toolchain on Alpine and I couldn't figure out how it works

@lnicola See rust-lang/rust#58575 and https://github.com/mati865/rust/blob/451343e0f3d90904bdc2080cc8bc4eb00be0364e/src/ci/docker/dist-x86_64-musl/Dockerfile#L49

So rust host tool chain itself is dynamic link to musl, As your reddit link suggest, dlopen works in that case.

@kuznero
Copy link
Author

kuznero commented May 8, 2020

I ran a simple test now:

  1. created a simple project that uses serde derive (proc macro) like this:
use serde::{Deserialize, Serialize};

#[derive(Serialize, Deserialize, Debug)]
struct Point {
    x: i32,
    y: i32,
}

fn main() {
    let point = Point { x: 1, y: 2 };

    // Convert the Point to a JSON string.
    let serialized = serde_json::to_string(&point).unwrap();

    // Prints serialized = {"x":1,"y":2}
    println!("serialized = {}", serialized);

    // Convert the JSON string back to a Point.
    let deserialized: Point = serde_json::from_str(&serialized).unwrap();

    // Prints deserialized = Point { x: 1, y: 2 }
    println!("deserialized = {:?}", deserialized);
}
  1. I used exactly same Dockerfile approach (perhaps it can be done without it as there are no other dependencies as was mentioned by @matklad above) and built fully statically linked executable

  2. When I ran it it worked as well as when I ran the dynamically linked version. Here is the output (same in both examples):

serialized = {"x":1,"y":2}
deserialized = Point { x: 1, y: 2 }

So, could you please explain why my test case is wrong? I would like to reproduce this problem and see exactly why you say it does not work.

I am trying to make developer experience in NixOS as good as in Ubuntu for frequently updating developer tools like rust-analyzer, prost, etc.

@lnicola
Copy link
Member

lnicola commented May 8, 2020

Try enabling these in the Code configuration:

{
    "rust-analyzer.cargo.loadOutDirsFromCheck": true,
    "rust-analyzer.procMacro.enable": true
}

Then reopen that project.

@kuznero
Copy link
Author

kuznero commented May 8, 2020

I am using Vim. But let me try to setup VSCode instead.

@flodiebold
Copy link
Member

@kuznero The problem is the compiler, not the program being built. When building your example project, rustc compiles the serde proc macro and dlopens it to execute it. It can do that because it's not statically linked. The target architecture doesn't matter here. In the same way, rust-analyzer acts as the compiler, and needs to be able to dlopen proc macros.

@edwin0cheng
Copy link
Member

edwin0cheng commented May 8, 2020

We have a discussion forum and I think it js more suitable to discuss there.

BTW, I think there is some misunderstanding here. The dlopen requirement only apply for the compiler/ RA. Proc-macro is compile time thing such that without dopen support will not affect end-users. (Binary generated by compiler/RA)
but without dlopen support affect compiler / RA expanding proc-macro compiles your program such that it will ICE.

@kuznero
Copy link
Author

kuznero commented May 8, 2020

I can see there is "rust-analyzer.cargo.loadOutDirsFromCheck" settings, but no "rust-analyzer.procMacro.enable", @lnicola.

BTW, I think there is some misunderstanding here. The dlopen requirement only apply for the compiler/ RA. Proc-macro is compile time thing such that without dopen support will not affect end-users. (Binary generated by compiler/RA)

Thanks for clarifying, this was my understanding from the very beginning that it belongs to compile-time.

but without dlopen support affect compiler / RA expanding proc-macro compiling your problem such that it will ICE.

Obviously RA needs to dlopen proc-macros, but it cannot if was statically linked (thanks @flodiebold for explanation) :( Now it is more or less clear. Thanks to all for your patience.

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.

6 participants