tree-sitter: 0.17.1 -> 0.17.3; run make install #102763

colemickens · 2020-11-04T14:18:27Z

Motivation for this change

Update tree-sitter to 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

teto · 2020-11-04T14:40:05Z

pkgs/development/tools/parsing/tree-sitter/default.nix

@@ -68,6 +68,11 @@ in rustPlatform.buildRustPackage {
    bash ./script/build-wasm --debug
  '';

+  postInstall = ''
+    export PREFIX=$out
+    make install


that looks wrong. make install should be called by default. If you need to export PREFIX, it can be done via configureFlags.

nevermind it uses buildRustPackage.

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.

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 make install also https://github.com/archlinux/svntogit-community/blob/b7a0f60fa46355ee73ac42ddf4fb419832bfeafe/trunk/PKGBUILD#L30 Arch makepkg strips static libraries by default)

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?

NickHu · 2020-11-04T16:30:55Z

Overlay for the impatient: ~/.config/nixpkgs/overlays/tree-sitter.nix

self: super:
{
  tree-sitter = super.tree-sitter.overrideAttrs (oldAttrs: {
    postInstall = "PREFIX=$out make install";
  });
}

SuperSandro2000 · 2020-11-05T19:38:10Z

Result of nixpkgs-review pr 102763 run on x86_64-darwin 1

1 package marked as broken and skipped:

tree-sitter

ipetkov · 2020-11-08T03:34:05Z

I noticed that the metadata sets broken = stdenv.isDarwin || stdenv.isAarch64;. I was able to build this successfully on macOS by adding buildInputs = lib.optionals stdenv.isDarwin [ darwin.apple_sdk.frameworks.Security ];, so it may be worth doing that in this PR.

pkgs/top-level/static.nix

NickHu · 2020-11-09T14:05:32Z

pkgs/development/tools/parsing/tree-sitter/default.nix

@@ -49,6 +51,8 @@ in rustPlatform.buildRustPackage {
  pname = "tree-sitter";
  inherit src version cargoSha256;

+  buildInputs = lib.optionals stdenv.isDarwin [ darwin.apple_sdk.frameworks.Security ];


Not sure if this should be darwin.apple_sdk.frameworks.Security or darwin.Security; I have no way to test.

What do other packages do?

Or maybe: what prompted you to add it if you don’t have a MacOS machine?

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.

@NickHu sorry I didn't clarify earlier, this is how I've normally seen this accomplished:

add a parameter called Security at the top of tree-sitter/default.nix

set buildInputs = lib.optionals stdenv.isDarwin [ Security ]; in tree-sitter/default.nix

update top-level/all-packages.nix with:

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 :)

@ipetkov ping

@NickHu 4d8adcc builds on macOS 👍

Profpatsch · 2020-11-09T15:00:38Z

pkgs/development/tools/parsing/tree-sitter/default.nix

+, enableShared ? true
+, enableStatic ? false


enableShared what? Not really clear what that means.

Do we need both options? Why is it configurable?

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

I had forgotten to set enableShared = false in static.nix; fixed now

Profpatsch · 2020-11-09T15:00:54Z

pkgs/development/tools/parsing/tree-sitter/default.nix

@@ -49,6 +51,8 @@ in rustPlatform.buildRustPackage {
  pname = "tree-sitter";
  inherit src version cargoSha256;

+  buildInputs = lib.optionals stdenv.isDarwin [ darwin.apple_sdk.frameworks.Security ];


What do other packages do?

Profpatsch · 2020-11-09T15:01:31Z

pkgs/development/tools/parsing/tree-sitter/default.nix

@@ -49,6 +51,8 @@ in rustPlatform.buildRustPackage {
  pname = "tree-sitter";
  inherit src version cargoSha256;

+  buildInputs = lib.optionals stdenv.isDarwin [ darwin.apple_sdk.frameworks.Security ];


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

NickHu · 2020-11-12T09:05:50Z

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

tree-sitter: 0.17.1 -> 0.17.3; run make install

562eaaf

ofborg bot requested a review from Profpatsch November 4, 2020 14:31

ofborg bot added 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 10.rebuild-linux: 1 labels Nov 4, 2020

teto reviewed Nov 4, 2020

View reviewed changes

mjlbach mentioned this pull request Nov 8, 2020

Fix latest builds by adding tree-sitter as dependency nix-community/neovim-nightly-overlay#50

Merged

NickHu mentioned this pull request Nov 8, 2020

config.public.json: add NickHu to trusted_users NixOS/ofborg#538

Closed

alyssais reviewed Nov 9, 2020

View reviewed changes

pkgs/top-level/static.nix Outdated Show resolved Hide resolved

NickHu force-pushed the tree-sitter branch from 49cbab3 to c27883e Compare November 9, 2020 14:04

NickHu reviewed Nov 9, 2020

View reviewed changes

Profpatsch requested changes Nov 9, 2020

View reviewed changes

NickHu added 3 commits November 9, 2020 18:41

tree-sitter: make shared/static libraries configurable

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

aea7f1b

tree-sitter: macOS compatibility

a63c722

tree-sitter: update grammars

4d8adcc

NickHu force-pushed the tree-sitter branch from c27883e to 4d8adcc Compare November 9, 2020 18:42

ofborg bot requested a review from Profpatsch November 9, 2020 19:01

NickHu merged commit a371c10 into NixOS:master Nov 12, 2020

This was referenced Nov 12, 2020

Use tree-sitter version from nixpkgs-unstable nix-community/neovim-nightly-overlay#54

Merged

Use tree-sitter version from nixpkgs-unstable nix-community/neovim-nightly-overlay#58

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tree-sitter: 0.17.1 -> 0.17.3; run make install #102763

tree-sitter: 0.17.1 -> 0.17.3; run make install #102763

colemickens commented Nov 4, 2020

teto Nov 4, 2020

teto Nov 4, 2020

teto Nov 4, 2020 •

edited

Loading

NickHu Nov 4, 2020 •

edited

Loading

NickHu Nov 6, 2020

NickHu commented Nov 4, 2020

SuperSandro2000 commented Nov 5, 2020

ipetkov commented Nov 8, 2020 •

edited

Loading

NickHu Nov 9, 2020

Profpatsch Nov 9, 2020

Profpatsch Nov 9, 2020

jupblb Nov 9, 2020

ipetkov Nov 9, 2020

NickHu Nov 9, 2020

ipetkov Nov 9, 2020

Profpatsch Nov 9, 2020

NickHu Nov 9, 2020 •

edited

Loading

NickHu Nov 9, 2020

Profpatsch Nov 9, 2020

Profpatsch Nov 9, 2020

NickHu commented Nov 12, 2020

tree-sitter: 0.17.1 -> 0.17.3; run make install #102763

tree-sitter: 0.17.1 -> 0.17.3; run make install #102763

Conversation

colemickens commented Nov 4, 2020

Motivation for this change

Things done

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teto Nov 4, 2020 • edited Loading

Choose a reason for hiding this comment

NickHu Nov 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickHu commented Nov 4, 2020

SuperSandro2000 commented Nov 5, 2020

ipetkov commented Nov 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickHu Nov 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickHu commented Nov 12, 2020

teto Nov 4, 2020 •

edited

Loading

NickHu Nov 4, 2020 •

edited

Loading

ipetkov commented Nov 8, 2020 •

edited

Loading

NickHu Nov 9, 2020 •

edited

Loading