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

pnpm.configHook / npmHooks.npm{Install,Build}Hook: Support pnpm-workspace.yaml projects #316908

Closed
doronbehar opened this issue Jun 3, 2024 · 7 comments

Comments

@doronbehar
Copy link
Contributor

Per the issue mentioned here:

#290715 (comment)

And the docs (link will be correct once #290715 reaches the channels).

We need to think about a way to support projects that use pnpm-workspace.yaml.

cc @Scrumplex .

@nathanscully
Copy link
Contributor

nathanscully commented Jun 6, 2024

I did a test for workspaces using the existing tooling and linked it the other week: #290715 (comment). Repo here: https://github.com/nathanscully/pnpm-monorepo-test/blob/pnpm-nixpkgs/flake.nix - note it probably needs a rebase.

It's probably non-ideal as its using the inbuilt pnpm filter spread syntax ( pnpm run --filter server... build) but it does work. Can we get away just documenting that approach for now rather than having to change the tooling to much to get the first PR in?

@doronbehar
Copy link
Contributor Author

For bash-language-server, I tried in the buildPhase:

pnpm --filter server run compile
pnpm run --filter server compile

And all of them gave me:

No projects matched the filters in "/build/source"

Which is peculiar... Running without a --filter server gave me the same errors as using npmHooks.npmBuildHook gave me...

@nathanscully
Copy link
Contributor

nathanscully commented Jun 6, 2024

This seems to work for me. Can you try it? Might need to move around the out folders but it builds.

{
  lib,
  stdenv,
  fetchFromGitHub,
  pnpm_9,
  nodejs,
  npmHooks,
  writeScriptBin,
}:

stdenv.mkDerivation (finalAttrs: {
  pname = "bash-language-server";
  version = "5.3.3";

  src = fetchFromGitHub {
    owner = "bash-lsp";
    repo = "bash-language-server";
    rev = "server-${finalAttrs.version}";
    hash = "sha256-4+imeVjosEspFhWzjtUp2IZx7aZIx56g8M03V3dEGVs=";
  };

  nativeBuildInputs = [
    nodejs
    pnpm_9.configHook
  ];
  pnpmDeps = pnpm_9.fetchDeps {
    inherit (finalAttrs) pname version src;
    hash = "sha256-W7BrUh0tGoG5vTuR3hXDbUTJFDiKET+QvIvCwewfZ2I=";
  };
  preBuild = ''
    rm -r vscode-client
    substituteInPlace tsconfig.json \
      --replace '{ "path": "./vscode-client" },' ""
  '';
  postBuild = ''
    pnpm run compile
    pnpm --offline deploy --frozen-lockfile  --ignore-script  --filter=bash-language-server server-deploy
  '';
  installPhase = ''
    cp -r server-deploy $out
  '';
})
$ node result/out/cli.js
Usage:
  bash-language-server start             Start listening on stdin/stdout
  bash-language-server -h, --help        Display this help and exit
  bash-language-server -v, --version     Print the version and exit

Environment variables:
  BASH_IDE_LOG_LEVEL                     Set the log level (default: info)

Further documentation: https://github.com/bash-lsp/bash-language-server

@doronbehar
Copy link
Contributor Author

Wow Thanks @nathanscully ! The main insight you bring, is that the argument --filter gets, is not the name of the directory of the subproject, but the name (defined in the folder's package.json) - which wasn't clear for me when reading pnpm's docs (https://pnpm.io/filtering ). The PR for bash-language-server is ready now at #317954 .

I conclude that we need a pnpm.installHook that will take care of such details (related to #317927 ).

@nathanscully
Copy link
Contributor

No worries. I ran into the same issue earlier when I picked up using pnpm. Glad I could help.

@dezren39 dezren39 mentioned this issue Jul 1, 2024
13 tasks
doronbehar pushed a commit to pyrox0/nixpkgs that referenced this issue Jul 3, 2024
doronbehar pushed a commit to pyrox0/nixpkgs that referenced this issue Jul 7, 2024
doronbehar pushed a commit to pyrox0/nixpkgs that referenced this issue Jul 21, 2024
doronbehar pushed a commit to pyrox0/nixpkgs that referenced this issue Aug 2, 2024
doronbehar pushed a commit to pyrox0/nixpkgs that referenced this issue Aug 2, 2024
doronbehar pushed a commit to pyrox0/nixpkgs that referenced this issue Aug 5, 2024
emilazy pushed a commit to emilazy/nixpkgs that referenced this issue Aug 25, 2024
emilazy pushed a commit to emilazy/nixpkgs that referenced this issue Aug 25, 2024
GiovanniGrieco pushed a commit to GiovanniGrieco/nixpkgs that referenced this issue Aug 31, 2024
presto8 pushed a commit to presto8/nixpkgs that referenced this issue Sep 1, 2024
@GetPsyched
Copy link
Member

Pinging people subscribed to this issue about #323493 being merged. I myself was waiting for pnpm workspaces support and had no idea that it was already added a month ago.

Unsure if this issue includes additions other than workspaces support, but if not, should probably be closed.

@doronbehar
Copy link
Contributor Author

Thanks @GetPsyched for writing here, indeed I forgot to close this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants