tree-sitter: 0.17.1 -> 0.17.3; run make install#102763
Conversation
|
|
||
| postInstall = '' | ||
| export PREFIX=$out | ||
| make install |
There was a problem hiding this comment.
that looks wrong. make install should be called by default. If you need to export PREFIX, it can be done via configureFlags.
There was a problem hiding this comment.
nevermind it uses buildRustPackage.
There was a problem hiding this comment.
this seems to install both static and shared libary. we should pick one. I think you (or another) could propose a patch for their Makefile to be more granular (and for now remove the extra files via rm). Since homebrew is packaging it too, would be interesting to see how they've done it.
There was a problem hiding this comment.
In the arch community repo they package the shared library https://www.archlinux.org/packages/community/x86_64/tree-sitter/files/ (I'm actually a little confounded as to why the static libraries do not appear in this file listing because the PKGBUILD seems to just run Arch makepkg strips static libraries by default)make install also https://github.com/archlinux/svntogit-community/blob/b7a0f60fa46355ee73ac42ddf4fb419832bfeafe/trunk/PKGBUILD#L30
There was a problem hiding this comment.
I went to the liberty of addressing this in a follow-up commit; thanks for your contribution @colemickens!
@teto can you re-review and then I can merge?
|
Overlay for the impatient: self: super:
{
tree-sitter = super.tree-sitter.overrideAttrs (oldAttrs: {
postInstall = "PREFIX=$out make install";
});
} |
|
Result of 1 package marked as broken and skipped:
|
|
I noticed that the metadata sets |
There was a problem hiding this comment.
Not sure if this should be darwin.apple_sdk.frameworks.Security or darwin.Security; I have no way to test.
There was a problem hiding this comment.
Or maybe: what prompted you to add it if you don’t have a MacOS machine?
There was a problem hiding this comment.
I can testify that the build process indeed fails without it on MacOS. This dependency seem to be typical for Rust packages, like bat or git-and-tools.delta.
There was a problem hiding this comment.
@NickHu sorry I didn't clarify earlier, this is how I've normally seen this accomplished:
- add a parameter called
Securityat the top oftree-sitter/default.nix - set
buildInputs = lib.optionals stdenv.isDarwin [ Security ];intree-sitter/default.nix - update
top-level/all-packages.nixwith:
tree-sitter = callPackage ../development/tools/parsing/tree-sitter {
inherit (darwin.apple_sdk.frameworks) Security;
};
You can ping me when the change is made and I can test that it builds on macOS :)
There was a problem hiding this comment.
enableShared what? Not really clear what that means.
Do we need both options? Why is it configurable?
There was a problem hiding this comment.
If you're building static, maybe you want to not have the shared libraries - configurable by this flag
I didn't make up this convention, if you grep for enableShared in nixpkgs you'll find plenty of occurrences
There was a problem hiding this comment.
I had forgotten to set enableShared = false in static.nix; fixed now
There was a problem hiding this comment.
Or maybe: what prompted you to add it if you don’t have a MacOS machine?
pkgs/development/tools/parsing/tree-sitter/grammars/default.nix
Outdated
Show resolved
Hide resolved
|
Seems like this has gone quiet. I think I addressed everyone's concerns, so I'll just go ahead and merge this. If any follow-ups are required they can come in a separate PR |
Motivation for this change
Update
tree-sitterto latest.Also, this is necessary to actually install the lib/include/pkgconfig files. (neovim nightly now needs this and I know there are a number of us)
cc: @teto @mjlbach
Things done
sandboxinnix.confon non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"./result/bin/)nix path-info -Sbefore and after)