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

pip installation fixed #433

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

yaroslavyaroslav
Copy link

@yaroslavyaroslav yaroslavyaroslav commented Aug 30, 2024

In general the idea is to generate all missed files as the first step of installation process, which is allowed by python ecosystem.

I'd like to emphasise that beyond that i've said above I have a little sense of what is happening in this code, because I'm kinda lame in python. But now pip install . runs successfully on my side with brew install tree-sitter command beforehand. Thus I believe pip install git+https://github.com/alex-pinkus/tree-sitter-swift.git will so.

Closes: #432

@alex-pinkus
Copy link
Owner

Hmm, this is an interesting tradeoff. It feels a bit iffy for the Python build logic to direct the user to npm for the CLI; it feels like there’s a rubicon we’re crossing by pointing to another language and ecosystem. I’m not sure that I prefer this over directing the user to the with-generated-files branch (despite what I said on #432).

The alternate I’m considering is:

  1. Update the with-generated-files branch with a one-time commit to add setup.py.
  2. Validate that pip install git+https://github.com/alex-pinkus/tree-sitter-swift.git@with-generated-files succeeds with that file in place.
  3. Update the setup.py on main so that it fails fast with a message directing towards @with-generate-files.

I could be convinced that this “run-the-cli” logic is acceptable if there are other well-architected Python libraries that require the use of NPM to build. Do you know of any? What do you think of my alternate proposal?

@yaroslavyaroslav
Copy link
Author

Agreed about nmp ref there. But as I checked just yet tree-sitter could be installed by cargo install tree-sitter-cli which is a common case in Python ecosystem coz there's quite an amount of packages that's nothing but a wrapper around some c/cpp/rust lib (still painful though, but still unavoidable since this packets is useless without tree-sitter lib installed (afaik)). Or even by brew if the user is on Mac.

So usually the responsibility between Python package and user environment setup is separated right there: all configuration/artifacts building for a package performed by the package itself all tooling for that (say llvm/gcc) should be installed beforehand by an OS package manager.

So I could replace this error message with cargo reference. Actually usually this kind of errors are even more opaque, like "tree-sitter-cli missing" so it leaves completely up to user to figure out how to install any.

Speaking about pointing it to a separate constantly outdated branch, I consider it as a last resort if any other fails by some reason. But after all it's not me who'll have to maintain it further 💁🏻‍♂️

@alex-pinkus
Copy link
Owner

The branch gets auto updated on every new release so I’m not worried about maintenance there (if it looks stale it’s because I haven’t had a reason to do a release in a while).

What existing Python packages require installation of something separate through Cargo first? I would certainly prefer that if there’s prior art but that’s just because I like Rust more than I like js.

The tree-sitter lib and tree-sitter cli are two different beasts, as I understand it, but I haven’t looked at all into how it works with Python. Unless the Python binding is some unholy wrapper around the cli, someone could absolutely be plausibly using it the lib without the cli. If the Python binding is an unholy cli wrapper, it would be great to simply call out to that.

@alex-pinkus
Copy link
Owner

Also, just to add, I really appreciate you doing this! I hope my comments aren’t discouraging — I’m happy that you are using this and trying to get it working with Python, I just want to make sure we don’t impose an undue burden on future users.

@yaroslavyaroslav
Copy link
Author

Nah, it's all good. You're raising valid concerns about PR in the repo you own. Also I've missed that the tool that I'm using in the building script is the cli not a lib, so I agree that this is no go option in it's current state.

So I dug a bit further here. And here's two examples which I consider as more appropriate: https://github.com/openai/tiktoken and https://github.com/ijl/orjson are both having a rust core which are could be obtained by user by either stored in pip as an prebuilt binary (to install by pip without local compilation) or (it's an assumption of mine, haven't checked yet) got setup instruction of how to build included rust code and (link?) it with python wrapper around it.

So the next question is, is there a way to generate those missed in the main files with a pure rust without tree-sitter-cli installed?

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.

Failing to install in python
2 participants