angular-language-server-bin: init at 18.2.0
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:
- NixOS test(s) (look inside nixos/tests)
- and/or package tests
- or, for functions and "core" functionality, tests in lib/tests or pkgs/test
- made sure NixOS tests are linked to the relevant packages
- [ ] 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.
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?
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
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.
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 👍 .
IMHO this is ready to be merged. @lelgenio @NotAShelf do you have time to build this on your machines for a review?
I can try later today.
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?
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.descriptionis set and fits guidelines - [x]
meta.licensefits upstream license - [ ]
meta.platformsis set - [x]
meta.maintainersis set - [x]
meta.mainProgramis set, if applicable. - [x] build time only dependencies are declared in
nativeBuildInputs - [x] source is fetched using the appropriate function
- [ ] the list of
phasesis not overridden - [x] when a phase (like
installPhase) is overridden it starts withrunHook preInstalland ends withrunHook 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.
Set meta.platforms to lib.platforms.unix to drop the 10.rebuild-darwin: 0 label.
@wegank Done.
Friendly ping for @wegank and @SuperSandro2000 with merge rights.
#354195