Open sei-dshepard opened 3 months ago
Fine with me, I don't know NixOS (nor do I know the history behind the patching). I don't know if you do @lnicola, thoughts on that?
No, @matklad does. I've seen people get confused about the way we patch the server on Nix in the past, so maybe it's not such a bad idea. But I don't know what the users will have to do once we remove it.
@sei-dshepard keep in mind that the VS Code extension comes with a bundled binary, and will use that instead of the one available on the system. This matters because our protocol extensions can cause incompatibilities if you're running different versions of the extension and server.
To further elaborate, NixOS attempts to be immutable and repeatable in all things involving the installation of software. The design feature of bootstrap.ts, which will actually overwrite and mutate state of the language server, is at odds with these goals, making the system less deterministic.
The extension doesn't modify itself, it makes a copy of the server binary. I think this is actually more deterministic, since the two components will always match.
I think all of these should work:
cargo xtask install
from sourceIf 1. breaks due to bootstrap logic, rust-analyzer should provide some easy way for Nix OS maintainers to neuter that logic when packaging for NixOS.
If 2. breaks, that needs to be fixed! If NixOS really insists that using non-nixos ways to manage extensions is the wrong way to go, than it should flat out disable extension marketplace for Code. But I think what should be happening here is that the use-case of downloading the binaries should be supported, because that's just how the upstream software is supposed to work. I think the behavior here should be equivalent to what NixOS's packaged rustup is doing: https://github.com/NixOS/nixpkgs/blob/master/pkgs/development/tools/rust/rustup/0001-dynamically-patchelf-binaries.patch. I wonder if we could anything on our side to make this work better? I guess the principle problem is that we need to be able to load proc macros, and that means that we must dynamically link sufficiently same glibc as used by rustc.
Not that I don't appreciate the attempts to support NixOS, but the package manager's packaging scripts are the correct layer of abstraction to support OS/distribution layer concerns. There are numerous edge cases for NixOS that are not correctly handled by the code implementing NixOS checks, specifically bootstrap.ts, which are causing numerous bug reports from users, in a multitude of forums. I am requesting that specific packaging handling for NixOS be removed from the Rust-Analyzer VSCode extension, to allow the package maintainers to consider the edge cases more clearly for their specific use cases.
To further elaborate, NixOS attempts to be immutable and repeatable in all things involving the installation of software. The design feature of bootstrap.ts, which will actually overwrite and mutate state of the language server, is at odds with these goals, making the system less deterministic. Additionally, there are use-cases, such as installing VS Code and Rust-Analyzer in a container, where the assumptions made by the script author do not hold true and are causing runtime failures.