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

vtsls: init at 0.2.3 #319501

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

vtsls: init at 0.2.3 #319501

wants to merge 2 commits into from

Conversation

wizardlink
Copy link
Contributor

Description of changes

This package provides a wrapper program for the TypeScript language server, this is an alternative option that a few distributions of NeoVIM are using, such as AstroNvim.

Things done

Added a binary for @vtsls/language-server and it's dependencies (@vtsls/language-service and @vtsls/vscode-fuzzy).

  • 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:
  • 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.

@wizardlink
Copy link
Contributor Author

I haven't ran the nixpkgs-review test successfully, but it fails at 05d7f5e, so not sure what I should do here.

Also haven't added myself to maintainers.nix, I don't think it would be relevant here? I also have a PR (#297598) with it being added, so maybe that should stay just there.

@wizardlink wizardlink changed the title nodePackages.@vtsls/language-service: init at 0.2.3 nodePackages.@vtsls/language-server: init at 0.2.3 Jun 13, 2024
Copy link
Member

@SuperSandro2000 SuperSandro2000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see #229475

@wizardlink
Copy link
Contributor Author

Awesome! Will adapt as soon as I'm able to @SuperSandro2000.

@wizardlink
Copy link
Contributor Author

As per #290715, pnpm.fetchDeps does not works in monorepos, which is the case of vtsls so the alternatives would be to just fetch it pre-compiled from the npm registry or maintain a fork of it without pnpm which I don't know if I'd have the time for.

What are your thoughts @SuperSandro2000?

@wizardlink wizardlink changed the title nodePackages.@vtsls/language-server: init at 0.2.3 vtsls: init at 0.2.3 Jun 16, 2024
@wizardlink
Copy link
Contributor Author

I went with the option of maintaining a package-lock.json for now, will wait for your thoughts on the approach.


sourceRoot = "${src.name}/packages/server";

npmDeps = importNpmLock {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry for the delay, I have been flooded with e-mails recently.

@SuperSandro2000 what do you mean with "IFD"? If it's the "import from derivation" feature, I'm not familiar with it and how it could relate to this issue.

I'm not quite sure why ofborg is failing to build it, I can locally via nix-build -A vtsls as well as calling the derivation in my system. Reading again the specifications for this, what I'm doing still makes sense to me, maybe I am misinterpreting something.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SuperSandro2000, can you give me an update on this?

@lxsymington
Copy link

Far from knowledgeable on this and apologies if this is unhelpful, but this comment feels relevant? #316908 (comment)

@wizardlink
Copy link
Contributor Author

No need for apologies, we're here to help each other.

vtsls is under a monorepo which in the javascript-section for pnpm it says that it's "[...] hard or impossible to produce a binary from such projects [...]", of which I couldn't myself get it to work, only through this method.

Perhaps I'm misunderstanding something, but through this method it's been working for me thus far.

@aca
Copy link
Contributor

aca commented Aug 11, 2024

I tried to package vtsls for personal usage. Didn't know this PR existed.
vtsls packages/service build script seems to rely on git submodule and I couldn't get it work. Here's what I tried..
I was able to get it work by using this PR.

{
  bash,
  fetchFromGitHub,
  lib,
  pkgs,
  stdenvNoCC,
}:
stdenvNoCC.mkDerivation (finalAttrs: {
  pname = "vtsls";
  version = "latest";

  src = fetchFromGitHub {
    owner = "yioneko";
    repo = "vtsls";
    rev = "33ab3a11a5fcb3038d10d4f47d91655683b21dbc";
    hash = "sha256-DKJzQj5oD9LN5IHPwaepqGsOWvJjIlkvyutpiInKpDg=";
    fetchSubmodules = true;
    leaveDotGit = true;
  };

  nativeBuildInputs = [
    pkgs.nodejs
    pkgs.unstable.pnpm_9.configHook
    pkgs.git
  ];

  pnpmDeps = pkgs.unstable.pnpm_9.fetchDeps {
    inherit (finalAttrs) pname version src;
    hash = "sha256-NYx8+SVJbB7MZ5Oaw893QqtDmz+AIvaxy1qT6d+oCRU=";
  };

  buildPhase = ''
    set -x
    pnpm install
    pnpm run build
  '';

    # git status
    # git submodule status vscode
    # ls -al packages/service/vscode/
    # runHook preBuild
    # cd packages/service
    # runHook postBuild
})

Comment on lines +21 to +24
npmDeps = importNpmLock {
npmRoot = "${src}/packages/server";
packageLock = lib.importJSON ./package-lock.json;
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
npmDeps = importNpmLock {
npmRoot = "${src}/packages/server";
packageLock = lib.importJSON ./package-lock.json;
};
npmDeps = importNpmLock {
package = lib.importJSON ./package.json; # you need to vendor also this file
packageLock = lib.importJSON ./package-lock.json;
};

IFD is a nix feature allowed by default in nix but forbidden in nixpkgs. My change above eliminates it but introduces other errors that I wasn't able to quickly fix.
Use

nix --option allow-import-from-derivation false build .#vtsls -L --no-eval-cache

from your nixpkgs directory to be sure that there are no IFDs (I'm also assuming flakes with the command above).
Anyway how did you get the package-lock.json file? Did you use npm i --package-lock-only? In that case it means that we are using different packages versions than upstream.

I saw that there is a package called pnpm-lock-export that should convert the original pnpm-lock.yaml file to a package-lock.json but it fails with that specific lockfile. There is also pnpm-lock-to-npm-lock but I haven't tried since it's not in Nixpkgs. However I believe you should document in a comment the needed to command to generate the lockfile, or even better, write an update script that does it automatically.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much for the insight @aciceri! Yes I had just created a lockfile myself, since I couldn't create one otherwise (as far as I knew).

I'm seeing this quite late however, so now there seems to be an official way to handle workspaces as @lxsymington mentioned promptly mentioned - I'll be looking into that now and follow up the PR with it if things go smooth.

@lxsymington
Copy link

vtsls is under a monorepo which in the javascript-section for pnpm it says that it's "[...] hard or impossible to produce a binary from such projects [...]", of which I couldn't myself get it to work, only through this method.

@wizardlink It looks as if this merged PR #323493 may address that? Again apologies if I'm being unhelpful!

@austinbutler
Copy link
Member

A few days ago another PR was opened for vtsls, already using pnpmWorkspace: #347284

@wizardlink
Copy link
Contributor Author

Just got some time to take a look into it, I can close this PR in favor of it if anything.

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