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

Setup nix with direnv #129

Merged
merged 2 commits into from
Jan 12, 2023
Merged

Setup nix with direnv #129

merged 2 commits into from
Jan 12, 2023

Conversation

ghostbuster91
Copy link
Contributor

@ghostbuster91 ghostbuster91 commented Jan 11, 2023

This PR setups up a nix shell for the project. I think that when native libraries are involved it is especially beneficial to use nix to guarantee reproducibility with ease.

In addition to that it also configures .envrc file that can be read by direnv when entering the project which will automatically put the user into the nix shell.

Otherwise it has to be invoked manually by calling nix-shell .

Todo:

  • update readme

@eed3si9n
Copy link
Collaborator

The description contains a todo. Is this ready for a review?

@ghostbuster91
Copy link
Contributor Author

@eed3si9n yes, I didn't want to spend time updating readme without knowing if you are ok in general with such approach. Sorry, I should've made it more clear. Let me know what you think and then I can update it.

@eed3si9n
Copy link
Collaborator

I am not against adding a Nix setup, but is the general intent to replace the existing GitHub Actions steps or our local development process with Nix? I am kind of curious since I know of Nix, but I have never used it, and I am not sure what the tradeoffs are.

@keynmol
Copy link
Collaborator

keynmol commented Jan 12, 2023

IMO - we don't need Nix on GHA because we want to rely on Tree Sitter's official bootstrap scripts which are all in the NPM package. To clarify - Tree Sitter CLI downloads the pre-built binary, and the parser itself is shipped as a set of C files, not as a set of binary artifacts. Therefore there aren't many moving parts where something like Nix can be helpful in any way.

That said, it seems to be the thing-of-the-day to have a Nix setup for development, so I'm happy to just have it available and documented

@ckipp01
Copy link
Collaborator

ckipp01 commented Jan 12, 2023

My two cents are basically the same. I don't mind having it here, with the hopes that a nix user will also maintain it. I wouldn't want to move CI to it or push that all users should have it locally to develop on this repo.

@ghostbuster91
Copy link
Contributor Author

ghostbuster91 commented Jan 12, 2023

My intention was only to make it slightly more convenient for nix user, not to enforce it on the rest.

I have to admit that I haven't thought yet about using it on CI but @keynmol already answered that part :) If I might add two-cents here, it is all about making sure that the same version are used. For the development of grammar users need two things: node and c compiler. Nix takes care of that, but I agree that it is not that hard to ensure that manually.

Seems like we are on the same page, hence I will update the readme with the relevant section for nix-based users.

Copy link
Collaborator

@eed3si9n eed3si9n left a comment

Choose a reason for hiding this comment

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

Thanks, @ghostbuster91!

@keynmol keynmol merged commit 960966a into tree-sitter:master Jan 12, 2023
@ghostbuster91 ghostbuster91 deleted the setup-nix branch January 12, 2023 19:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants