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

astro-language-server: init at 2.10.0 #309100

Closed
wants to merge 1 commit into from

Conversation

pyrox0
Copy link
Member

@pyrox0 pyrox0 commented May 4, 2024

Description of changes

Creates the astro-language-server package, moving it from nodePackages. I also regenerated node-packages.nix to remove it from that package set, but if that's not wanted to be in this PR, I can remove that. This also updates from 2.8.3 to 2.10.0, see the changelog.

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.05 Release Notes (or backporting 23.05 and 23.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.

Add a 👍 reaction to pull requests you find important.

@ofborg ofborg bot added the 8.has: package (new) This PR adds a new package label May 4, 2024
@ofborg ofborg bot requested a review from Luflosi May 4, 2024 20:31
@getchoo getchoo self-requested a review May 5, 2024 07:50
@pyrox0
Copy link
Member Author

pyrox0 commented May 5, 2024

This makes use of PNPM, so it can be improved with #290715 being merged. If that's merged before this, I'll be glad to update this.

@doronbehar
Copy link
Contributor

I think it'll take time for #290715 :).

@doronbehar
Copy link
Contributor

CI isn't green BTW - hash mismatch.

@pyrox0 pyrox0 force-pushed the astro-ls-buildNpmPackage branch from 5f2260d to 4c70200 Compare May 7, 2024 19:57
@pyrox0
Copy link
Member Author

pyrox0 commented May 7, 2024

CI isn't green BTW - hash mismatch.

Confirmed the error and fixed.

@pyrox0 pyrox0 force-pushed the astro-ls-buildNpmPackage branch from 4c70200 to 23a5be6 Compare May 7, 2024 20:09
@pyrox0 pyrox0 requested a review from doronbehar May 7, 2024 20:10
@gepbird
Copy link
Contributor

gepbird commented May 7, 2024

Also version 2.9.0 is out, consider bumping.

@pyrox0 pyrox0 force-pushed the astro-ls-buildNpmPackage branch from 23a5be6 to cbe36c9 Compare May 8, 2024 02:30
@pyrox0 pyrox0 changed the title astro-language-server: init at 2.8.4 astro-language-server: init at 2.9.0 May 8, 2024
@pyrox0
Copy link
Member Author

pyrox0 commented May 8, 2024

Also version 2.9.0 is out, consider bumping.

Updated, but now I'm getting ERR_PNPM_NO_OFFLINE_META errors when trying to build the package(speciflcally, it happens in installPhase), and I'm unable to figure out how to solve it. Build log is here

@doronbehar
Copy link
Contributor

Damn... Another a hash mismatch in ofborg:

error: hash mismatch in fixed-output derivation '/nix/store/z59ba5lgklsrfjc7rvvh0lfxy70lyb3c-astro-language-server-pnpm-deps-2.9.0.drv':
         specified: sha256-S4CGTnXyY1ICU801hAOQpc0MBNPulkJVBctlao5XPfY=
            got:    sha256-Y6CPjZZrIgMpu54511G3EXOuSfJ/nTS8fLYQmhQaT4g=

I wonder what @Scrumplex will think about it.. @pyrox0 could you run please

nix build --rebuild .#astro-language-server.pnpm-deps

And/Or somehow try to find the culprit for the hash difference?

pnpm config set store-dir $out
# use --ignore-script and --no-optional to avoid downloading binaries
# use --frozen-lockfile to avoid checking git deps
pnpm install --frozen-lockfile --no-optional --ignore-script
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
pnpm install --frozen-lockfile --no-optional --ignore-script
pnpm install --frozen-lockfile --no-optional --ignore-script --force

this is the result of the hash mismatch. without --force, pnpm will only install dependencies for the current architecture. we might want to extract from #290715 or vesktop instead

Copy link
Member Author

@pyrox0 pyrox0 May 8, 2024

Choose a reason for hiding this comment

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

Edited this in and fixed the hash. Thanks for catching this!

Also edited in some other changes from #290715, but still doesn't resolve the build issue.

@pyrox0 pyrox0 force-pushed the astro-ls-buildNpmPackage branch 2 times, most recently from be6faf1 to 899cea0 Compare June 27, 2024 21:20
@pyrox0
Copy link
Member Author

pyrox0 commented Jun 27, 2024

I see now the ERR_PNPM_NO_OFFLINE_META error at ofborg's :/. Seems like an instance of upstream's pnpm/pnpm#5315 ... Not sure what to do.

This is only an error if we use pnpm deploy. To work around this issue we can use the "binary" generated by pnpm build from packages/language-server/bin/nodeServer.js.

I see that, but it mentions that workspaces are not supported atm, and this is a workspace. I'll try to at least use the fetchDeps script, but I can't guarantee I can use it until workspaces are properly supported(if possible)

Even though the new pnpm tooling doesn't support building only a single package from a workspace, we can use it to build all packages if we don't mind wasting some resources.

I packaged using the methods I mentioned above in gepbird@2f17100, tried it in neovim and looks like everything works. The build time is not too bad, approximately 2 minutes, but it produces 400M of files.

Thanks for the ideas in that! I kept the original dependencies process and added some filtering, managed to get it down to ~215 mb. Can you test if this still works? I don't see any reason it shouldn't, but a double-check would be great.

@pyrox0 pyrox0 force-pushed the astro-ls-buildNpmPackage branch from 899cea0 to a5de681 Compare June 27, 2024 21:29
Copy link
Contributor

@gepbird gepbird left a comment

Choose a reason for hiding this comment

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

Tested it and works as expected! You're right, we probably shouldn't adopt the new pnpm tooling until it supports workspaces to save on performance, this builds (approximately) 2x faster with half as much filesize.

pkgs/by-name/as/astro-language-server/package.nix Outdated Show resolved Hide resolved
@pyrox0 pyrox0 changed the title astro-language-server: init at 2.9.0 astro-language-server: init at 2.10.0 Jun 28, 2024
pnpm config set side-effects-cache false
pnpm config set update-notifier false
pnpm config set dedupe-peer-dependents false
pnpm install --filter=@astrojs/language-server --frozen-lockfile --no-optional --ignore-script --force
Copy link
Contributor

Choose a reason for hiding this comment

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

So Do I understand correctly that this --filter argument is what you would miss support for in pnpm.fetchDeps?

Copy link
Member Author

Choose a reason for hiding this comment

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

The filter thing is important, but also setting dedupe-peer-dependants above, which is required for this package to build properly.

Copy link
Contributor

Choose a reason for hiding this comment

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

The filter thing is important, but also setting dedupe-peer-dependants above, which is required for this package to build properly.

Hmm it'd be nice to make pnpm.fetchDeps accept arguments that would allow to add arbitrarily such config options.. cc @Scrumplex

pkgs/by-name/as/astro-language-server/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/as/astro-language-server/package.nix Outdated Show resolved Hide resolved
pnpm config set package-manager-strict false
pnpm config set dedupe-peer-dependents false

pnpm install --filter=@astrojs/language-server --offline --frozen-lockfile --no-optional --ignore-script
Copy link
Contributor

Choose a reason for hiding this comment

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

For sure pnpm.configHook will gain benefit if you could add arbitrary flags to it... What do you think about that @Scrumplex ?

@pyrox0 pyrox0 force-pushed the astro-ls-buildNpmPackage branch from a5de681 to 0f97ef1 Compare June 29, 2024 22:02
@pyrox0
Copy link
Member Author

pyrox0 commented Jun 29, 2024

R.E bin/astro-ls not being executable:

That isn't the case here, both the symlink and the file it points to are both marked executable. I'm still not sure why patchShebangs doesn't work.

@pyrox0 pyrox0 force-pushed the astro-ls-buildNpmPackage branch from 0f97ef1 to 21c7157 Compare June 29, 2024 22:31
@doronbehar
Copy link
Contributor

OK This looks good in principal, but I tend to think it will be beneficial to add the missing options to the pnpm.fetchDeps function and to the pnpm.configHook. Do you think you can work on that @pyrox0 ? This would make this packaging much more elegant and it may be beneficial for future usages of these attributes, especially for packages that use workspaces.

@pyrox0
Copy link
Member Author

pyrox0 commented Jun 29, 2024

I can try to do that at least for the filter attribute, but the extra config options may be a bit more difficult. I'll see what I can do, and will submit it as a separate pull request.

@SuperSandro2000
Copy link
Member

Please rebase

@doronbehar
Copy link
Contributor

#323493 (which also needs a reabase :)) is a better alternative. I'll close this one in favor of it.

@doronbehar doronbehar closed this Aug 2, 2024
@pyrox0 pyrox0 deleted the astro-ls-buildNpmPackage branch August 4, 2024 23:20
doronbehar added a commit to pyrox0/nixpkgs that referenced this pull request Aug 5, 2024
@venikx venikx mentioned this pull request Sep 10, 2024
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nodejs 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: 1 This PR was reviewed and approved by one reputable person
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants