nixpkgs icon indicating copy to clipboard operation
nixpkgs copied to clipboard

angular-language-server-bin: init at 18.2.0

Open tricktron opened this issue 1 year ago • 1 comments

Packages the angular language server as binary because packaging from source is especially difficult (see https://github.com/NixOS/nixpkgs/pull/349521 for more details).

Closes https://github.com/NixOS/nixpkgs/issues/244019.

Things done

  • Built on platform(s)
    • [ ] x86_64-linux
    • [ ] aarch64-linux
    • [ ] x86_64-darwin
    • [x] aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • [ ] sandbox = relaxed
    • [x] 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
  • [x] 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
  • [x] Fits CONTRIBUTING.md.

Add a :+1: reaction to pull requests you find important.

tricktron avatar Oct 21 '24 14:10 tricktron

We are not arch Linux, please drop the bin suffix.

I have put this because there are already some packages with the -bin suffix in nixpkgs, e.g. vlc-bin, vault-bin and all the prebuilt jdks-bins. Do we have another naming convention to differentiate binary builds from source builds for the same package?

tricktron avatar Oct 22 '24 06:10 tricktron

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

nixos-discourse avatar Oct 22 '24 14:10 nixos-discourse

I have put this because there are already some packages with the -bin suffix in nixpkgs, e.g. vlc-bin, vault-bin and all the prebuilt jdks-bins.

If there is no source variant for those, the suffix there should also go.

Do we have another naming convention to differentiate binary builds from source builds for the same package?

yes, set meta.sourceProvenance. We don't name pacakages differently because we usually do not want both variants even. Why have a binary package if there is a source one? There are only very little exceptions.

SuperSandro2000 avatar Oct 23 '24 13:10 SuperSandro2000

I have put this because there are already some packages with the -bin suffix in nixpkgs, e.g. vlc-bin, vault-bin and all the prebuilt jdks-bins.

If there is no source variant for those, the suffix there should also go.

Do we have another naming convention to differentiate binary builds from source builds for the same package?

yes, set meta.sourceProvenance. We don't name pacakages differently because we usually do not want both variants even. Why have a binary package if there is a source one? There are only very little exceptions.

@SuperSandro2000 I dropped the -bin suffix and added the meta.sourceProvenance attribute. Thank you, I learnt something new 👍 .

tricktron avatar Oct 23 '24 13:10 tricktron

IMHO this is ready to be merged. @lelgenio @NotAShelf do you have time to build this on your machines for a review?

tricktron avatar Oct 25 '24 13:10 tricktron

I can try later today.

NotAShelf avatar Oct 25 '24 13:10 NotAShelf

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

1 package built:
  • angular-language-server

lelgenio avatar Oct 28 '24 01:10 lelgenio

I think this is ready to merge.

Pinging @wegank and @SuperSandro2000 with merge rights.

Btw: Is there a better general approach to ping people with merge rights? E.g. is there a nixos group with merge rights?

tricktron avatar Oct 31 '24 07:10 tricktron

Reviewed points
  • [x] package path fits guidelines
  • [x] package name fits guidelines
  • [x] package version fits guidelines
  • [x] package builds on aarch64-darwin
  • [x] executables tested on aarch64-darwin
  • [x] meta.description is set and fits guidelines
  • [x] meta.license fits upstream license
  • [ ] meta.platforms is set
  • [x] meta.maintainers is set
  • [x] meta.mainProgram is set, if applicable.
  • [x] build time only dependencies are declared in nativeBuildInputs
  • [x] source is fetched using the appropriate function
  • [ ] the list of phases is not overridden
  • [x] when a phase (like installPhase) is overridden it starts with runHook preInstall and ends with runHook postInstall.
  • [x] patches have a comment describing either the upstream URL or a reason why the patch wasn't upstreamed
  • [x] patches that are remotely available are fetched rather than vendored
Possible improvements

Set meta.platforms to lib.platforms.unix to drop the 10.rebuild-darwin: 0 label.

Comments

LGTM except the point above.

wegank avatar Oct 31 '24 09:10 wegank

Set meta.platforms to lib.platforms.unix to drop the 10.rebuild-darwin: 0 label.

@wegank Done.

tricktron avatar Nov 01 '24 14:11 tricktron

Friendly ping for @wegank and @SuperSandro2000 with merge rights.

tricktron avatar Nov 06 '24 19:11 tricktron

#354195

SuperSandro2000 avatar Nov 07 '24 10:11 SuperSandro2000