fish-lsp: init at 1.0.8-1

[petertriho]

Description of changes

Adds fish-lsp #305493

Uses yarnConfigHook and yarnBuildHook instead of mkYarnPackage

Supersedes #320463 #330320

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
  • sandbox = relaxed
  • sandbox = true
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• 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/)
• 24.11 Release Notes (or backporting 23.11 and 24.05 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.

---

Add a 👍 reaction to pull requests you find important.

[petertriho]

Note sure why both ofborg passthru.tests are stuck. Would appreciate help with this

[petertriho]

Result of nixpkgs-review pr 330996 run on x86_64-linux 1

2 packages built:

• fish-lsp
• nixpkgs-manual

[petertriho]

Looks like all tests have passed. Is there anything else I should do to get this merged?

[tye-exe]

Thanks for putting in the work to do this, i've just spent the last few hours trying to do it myself before i found this. Does anyone know the timeline until this will be merged?

[nixos-discourse]

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/4785

[Scrumplex]

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 330996

---

x86_64-linux

✅ 1 package built:

• fish-lsp

[Scrumplex]

are these needed at build time or at runtime?

[petertriho]

Hi @Scrumplex,

Thank you for having look at the PR. Definitely need it for build time. I think they may be required for runtime as well.

• fish is required for the sh:build-wasm script which runs https://github.com/ndonfris/fish-lsp/blob/master/scripts/build-fish-wasm.fish
  Error without fish

> Running phase: buildPhase
       > Executing yarnBuildHook
       > yarn run v1.22.22
       > $ run-s sh:build-time sh:build-wasm
       > warning You don't appear to have an internet connection. Try the --offline flag to use the cache for registry queries.
       > $ fish ./scripts/build-time.fish
       > /bin/sh: fish: not found
       > error Command failed with exit code 127.
       > info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
       > ERROR: "sh:build-time" exited with 127.
       > error Command failed with exit code 1.

• which is required to generate the completion. See https://github.com/ndonfris/fish-lsp/blob/master/src/utils/builtins.ts (findShell spawns which fish)
  Error without which

       > Running phase: installPhase
       > /bin/sh: which: not found
       > /nix/store/aam03bwd19cadd1ad6qw26gr7h7hkh7m-fish-lsp-unstable-2024-07-26/share/fish-lsp/out/utils/builtins.js:88
       >     return result.stdout.toString().split('\n');
       >                          ^
       >
       > TypeError: Cannot read properties of null (reading 'toString')
       >     at createFunctionNamesList (/nix/store/aam03bwd19cadd1ad6qw26gr7h7hkh7m-fish-lsp-unstable-2024-07-26/share/fish-lsp/out/utils/builtins.js:88:26)
       >     at Object.<anonymous> (/nix/store/aam03bwd19cadd1ad6qw26gr7h7hkh7m-fish-lsp-unstable-2024-07-26/share/fish-lsp/out/utils/builtins.js:90:29)
       >     at Module._compile (node:internal/modules/cjs/loader:1469:14)
       >     at Module._extensions..js (node:internal/modules/cjs/loader:1548:10)
       >     at Module.load (node:internal/modules/cjs/loader:1288:32)
       >     at Module._load (node:internal/modules/cjs/loader:1104:12)
       >     at Module.require (node:internal/modules/cjs/loader:1311:19)
       >     at require (node:internal/modules/helpers:179:18)
       >     at Object.<anonymous> (/nix/store/aam03bwd19cadd1ad6qw26gr7h7hkh7m-fish-lsp-unstable-2024-07-26/share/fish-lsp/out/utils/translation.js:38:20)
       >     at Module._compile (node:internal/modules/cjs/loader:1469:14)
       >
       > Node.js v20.17.0
       > installShellCompletion: installed shell completion file `/nix/store/aam03bwd19cadd1ad6qw26gr7h7hkh7m-fish-lsp-unstable-2024-07-26/share/fish/vendor_completions.d/fish-lsp.fish' does not exist or has zero size

[Scrumplex]

I think we should add those packages to the wrapper if they are sometimes used at runtime.

Like this patch:

diff --git a/pkgs/by-name/fi/fish-lsp/package.nix b/pkgs/by-name/fi/fish-lsp/package.nix
index bc9ceb51ecb8..cefbdbaf366a 100644
--- a/pkgs/by-name/fi/fish-lsp/package.nix
+++ b/pkgs/by-name/fi/fish-lsp/package.nix
@@ -36,9 +36,7 @@ stdenv.mkDerivation rec {
     nodejs
     installShellFiles
     makeWrapper
-    # fish-lsp dependencies
     fish
-    which
   ];
 
   yarnBuildScript = "setup";
@@ -54,7 +52,8 @@ stdenv.mkDerivation rec {
     cp -r . $out/share/fish-lsp
 
     makeWrapper ${lib.getExe nodejs} "$out/bin/fish-lsp" \
-      --add-flags "$out/share/fish-lsp/out/cli.js"
+      --add-flags "$out/share/fish-lsp/out/cli.js" \
+      --prefix PATH : "${lib.makeBinPath [fish which]}"
 
     ${lib.optionalString stdenv.buildPlatform.canExecute stdenv.hostPlatform ''
       installShellCompletion --cmd fish-lsp \

[Scrumplex]

Forgot parenthesis. Sorry!

Suggested change

	${lib.optionalString stdenv.buildPlatform.canExecute stdenv.hostPlatform ''
	${lib.optionalString (stdenv.buildPlatform.canExecute stdenv.hostPlatform) ''