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

vscode-langservers-extracted: repackage with buildNpmPackage #234888

Merged
merged 2 commits into from
Jun 15, 2023

Conversation

Lord-Valen
Copy link
Contributor

Description of changes

Repackages nodePackages.vscode-langervers-extracted to be built using buildNpmPackage as per #229475. It was previously broken, and probably was since it was first added since there is no build script. Due to the background nature of LSPs, I guess nobody thought to check. This doesn't expose vscode-eslint-language-server as that requires the vscode-eslint package, which was not in the scope of this PR.

CC @winterqt

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.05 Release Notes (or backporting 22.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/2333

@Lord-Valen Lord-Valen force-pushed the vscode-langservers-extracted branch from 5d9720b to 46d58de Compare June 14, 2023 21:08
@SuperSandro2000 SuperSandro2000 merged commit 6ab653a into NixOS:master Jun 15, 2023
@Lord-Valen Lord-Valen deleted the vscode-langservers-extracted branch June 15, 2023 21:31
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/nodepackages-vscode-langservers-extracted-not-building/29201/1

@ahmedelgabri
Copy link
Contributor

Looks like it's now broken on darwin

error: builder for '/nix/store/isw32dsxy7pqvg5xh8yckw6fcj7jl16q-vscode-langservers-extracted-4.7.0.drv' failed with exit code 2;
       last 10 log lines:
       > node_modules/read-pkg/node_modules/semver/bin/semver: interpreter directive changed from "#!/usr/bin/env node" to "/nix/store/1vlb8v8qgcsgahc88dwb76zvqng732vz-nodejs-18.16.0/bin/node"
       > rebuilt dependencies successfully
       > patching script interpreter paths in node_modules
       > Finished npmConfigHook
       > patchPhase completed in 2 minutes 37 seconds
       > configuring
       > no configure script, doing nothing
       > building
       > babel:
       >   --out-dir requires filenames
       For full logs, run 'nix log /nix/store/isw32dsxy7pqvg5xh8yckw6fcj7jl16q-vscode-langservers-extracted-4.7.0.drv'.
error: 1 dependencies of derivation '/nix/store/nvdaiwr73as8ip2jzggb37cxarj9gs2d-user-environment.drv' failed to build
error: 1 dependencies of derivation '/nix/store/5y61gsjwv0s41a0fyfqrrqpxjb34h18f-etc.drv' failed to build
error: 1 dependencies of derivation '/nix/store/y5nk07c7z3r2ykim8a0kxcafn6h066sw-darwin-system-23.11.20230617.7cc30fd+darwin4.7c16d31.drv' failed to build

@stasjok
Copy link
Contributor

stasjok commented Jun 18, 2023

Is vscode dependency necessary here? Now it requires to enable unfree packages in order to build it.

@Lord-Valen
Copy link
Contributor Author

Lord-Valen commented Jun 18, 2023

Is vscode dependency necessary here? Now it requires to enable unfree packages in order to build it.

Unless vscodium exposes the servers (it might, but that's a deviation from what this package is upstream), vscode is necessary.

Upstream builds the latest vscode and pulls the language servers from it. It doesn't actually have the source for the servers or have them as dependencies, it extracts them from the vscode build (hence the name) and thinly wraps them. This is always how the package worked, it was just broken before (apparently since it was first added to nodePackages). If you thought you were using vscode-langservers-extended prior to this, you were mistaken because the binaries were never actually built. Unfortunately, this package worked it's way into Doom Emacs documentation, which doesn't realize this. That's probably where most people discover it.

Now, Microsoft does actually publish the language servers individually, and they do get updated (despite the claims of upstream, which are, ironically, dated). They are also all MIT. It's probably better to toss them in nodePackages (they use yarn) and use them instead, eventually deprecating this package as redundant.

@Lord-Valen
Copy link
Contributor Author

Lord-Valen commented Jun 18, 2023

In fact, we already have all but the markdown server packaged. This all serves to highlight something of which I was already aware: this package was silly from the start 😅.

@traxys
Copy link
Contributor

traxys commented Jun 18, 2023

The packaged versions seem awfully out of date if they refer to the same versions as npm though :/

@stasjok
Copy link
Contributor

stasjok commented Jun 18, 2023

If you thought you were using vscode-langservers-extended prior to this, you were mistaken because the binaries were never actually built

I personally use only json-language-server. It was working fine. I'm sure that I was using nodePackages.vscode-langservers-extended.

In fact, we already have all but the markdown server packaged

Those are very old versions.

@stasjok
Copy link
Contributor

stasjok commented Jun 18, 2023

Unless vscodium exposes the servers (it might, but that's a deviation from what this package is upstream), vscode is necessary.

I overrided this package with prev.vscode-langservers-extracted.override {vscode = final.vscodium;} and looks like it is the same. json-language-server works fine. I'm not sure about others. When I run them, I get a really big dump of javascript code to console. I get the same behavior with either vscode or vscodium. Only json-language-server doesn't print it (except an error that I shouldn't just run it Error: Connection input stream is not set. Use arguments of createConnection or set command line parameters: '--node-ip c', '--stdio' or '--socket={number}').

@Lord-Valen
Copy link
Contributor Author

If there's an uncaught exception, it definitely isn't working.

@stasjok
Copy link
Contributor

stasjok commented Jun 18, 2023

Upstream builds the latest vscode and pulls the language servers from it. It doesn't actually have the source for the servers or have them as dependencies, it extracts them from the vscode build (hence the name) and thinly wraps them

If I understand correctly, that's true when we are using github repository as a source. It contains build scripts to extract it from vscode. But the package published to npm have the code included (because that's the purpose of it — provide an easy method to install language servers without vscode).

Usually I'm all for building from source. But I'm not sure about this case.

If there's an uncaught exception, it definitely isn't working.

Maybe someone else will comment. I tested only json-language-server. It works when builded either with vscode or vscodium.

IvarWithoutBones added a commit to IvarWithoutBones/dotfiles that referenced this pull request Jul 2, 2023
Fixes a build failure on Darwin, see:
NixOS/nixpkgs#234888 (comment)

As mentioned in that thread using a catch-all package did not make a lot
of sense in the first place, best to only pull in relevant packages.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants