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.

Sign up for GitHub

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

Jump to bottom

vtsls: init at 0.2.6 #347284

Merged
merged 2 commits into from
Nov 12, 2024
Merged

vtsls: init at 0.2.6 #347284

merged 2 commits into from
Nov 12, 2024

Conversation

kuglimon
Copy link
Contributor

@kuglimon kuglimon commented Oct 8, 2024

Provides vtsls, a typescript language server.

I've lightly used this for a couple of weeks on NixOS and nix-darwin.

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

@github-actions github-actions bot added the 8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` label Oct 8, 2024
@NixOSInfra NixOSInfra added the 12. first-time contribution This PR is the author's first one; please be gentle! label Oct 8, 2024
@kuglimon
Copy link
Contributor Author

kuglimon commented Oct 8, 2024 •
edited
Loading

nix-build --check complains and it's due to some lock file pnpm creates, not sure if those can be solved. pnpm during build add fields prunedAt and storeDir to node_modules/.modules.yaml and those change from build to build.

drupol
drupol requested changes Oct 9, 2024
View reviewed changes
pkgs/by-name/vt/vtsls/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/vt/vtsls/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/vt/vtsls/package.nix Outdated Show resolved Hide resolved
@kuglimon kuglimon force-pushed the vtsls branch 2 times, most recently from db02fca to 12644c9 Compare October 9, 2024 19:18
@kuglimon kuglimon requested a review from drupol October 10, 2024 15:37
@austinbutler austinbutler mentioned this pull request Oct 13, 2024
Closed
13 tasks
@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/4698

@kuglimon
Copy link
Contributor Author

@drupol rereview ping.

drupol
drupol requested changes Oct 20, 2024
View reviewed changes
pkgs/by-name/vt/vtsls/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/vt/vtsls/package.nix Outdated Show resolved Hide resolved
@jmartindf jmartindf mentioned this pull request Oct 21, 2024
Closed
@kuglimon
Copy link
Contributor Author

Raised nodejs to 22. I've been running it locally for a week or so without issues. 22 is going to be the next LTS on 29.10, might as well update.

@kuglimon
Copy link
Contributor Author

kuglimon commented Nov 2, 2024

Rereview ping @drupol

drupol
drupol approved these changes Nov 2, 2024
View reviewed changes
Copy link
Contributor

@drupol drupol left a comment

Choose a reason for hiding this comment

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

LGTM overall, but I wish I would have someone else to review it too.

pkgs/by-name/vt/vtsls/package.nix Outdated
runHook postInstall
'';

meta = with lib; {
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to get rid of this.

Suggested change
meta = with lib; {
meta = {

@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Nov 2, 2024
@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-already-reviewed/2617/2083

aqrln
aqrln approved these changes Nov 7, 2024
View reviewed changes
Copy link
Member

@aqrln aqrln left a comment

Choose a reason for hiding this comment

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

Built and tested on aarch64-linux, works great

@aqrln aqrln removed the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Nov 7, 2024
@aqrln aqrln added the 12.approvals: 2 This PR was reviewed and approved by two reputable people label Nov 7, 2024
@KamWithK
Copy link

KamWithK commented Nov 9, 2024

Used it from the patches and worked great with my NeoVim

@redxtech
Copy link
Contributor

Tested and confirmed working for me as well.

redxtech
redxtech approved these changes Nov 11, 2024
View reviewed changes
Copy link
Contributor

@redxtech redxtech left a comment

Choose a reason for hiding this comment

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

LGTM 👍

It would be nice to include the suggestion from drupol, I can re-approve after the change is made if necessary.

@kuglimon
Copy link
Contributor Author

@redtech fixed, thanks!

I'm not that familiar with nix, yet; Should I always avoid with expressions? Is it expensive to evaluate or something else?

@drupol
Copy link
Contributor

drupol commented Nov 12, 2024

@redtech fixed, thanks!

I'm not that familiar with nix, yet; Should I always avoid with expressions? Is it expensive to evaluate or something else?

We try to avoid using it in front of attribute sets indeed. I don't have all the reasons at the moment (I'm on the smartphone) but they can be found in the issue queue pretty easily.

@aqrln aqrln added 12.approvals: 3+ This PR was reviewed and approved by three or more reputable people and removed 12.approvals: 2 This PR was reviewed and approved by two reputable people labels Nov 12, 2024
@drupol drupol merged commit 4ac41e0 into NixOS:master Nov 12, 2024
29 of 30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@redxtech redxtech redxtech approved these changes

@aqrln aqrln aqrln approved these changes

@drupol drupol drupol approved these changes

Assignees
No one assigned
Labels
8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 10.rebuild-linux: 1 11.by: package-maintainer This PR was created by the maintainer of the package it changes 12.approvals: 3+ This PR was reviewed and approved by three or more reputable people 12. first-time contribution This PR is the author's first one; please be gentle!
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@kuglimon @nixos-discourse @KamWithK @redxtech @drupol @aqrln @wegank @NixOSInfra