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

Configure GitHub Actions to build binary wheels and upload to pypi #117

Merged
merged 27 commits into from
Aug 2, 2023

Conversation

Akuli
Copy link
Contributor

@Akuli Akuli commented Aug 19, 2022

Discussed on #103. Still needs more work.

@Carreau
Copy link

Carreau commented Sep 7, 2022

I'd love to have this as well, I'm working on a project that uses tree-sitter, and some users have had problem building tree-sitter.

Is there anything I can do to help?

@Akuli
Copy link
Contributor Author

Akuli commented Sep 7, 2022

For now, you can use https://github.com/Akuli/py-tree-sitter-builds which installs tree_sitter and tree_sitter_languages without requiring a C compiler.

@maxbrunsfeld
Copy link
Contributor

This awesome! I can add PyPi credentials to the org if they are needed for publishing. When you get a chance, could you write up what you think still needs to be done?

@Akuli
Copy link
Contributor Author

Akuli commented Sep 7, 2022

I think a good next step would be including wheels for the tree_sitter module, but not the language binaries (currently tree_sitter_languages), for a couple reasons:

  • It is not needed to get tree-sitter working on systems without a C compiler. The existing tree-sitter-languages package depends on tree-sitter, and would install without requiring a C compiler if tree-sitter didn't require a C compiler.
  • Some languages are licensed under MIT. Others use Apache 2.0. This repo uses MIT license so we can't just bundle something that is Apache 2.0 licensed.
  • Including the languages makes the build slower.

To get this working:

  • We need to agree on when this workflow runs. Currently releasing is done by a script that is ran locally and pushes a tag. Should this run automatically when a tag is pushed, or be triggered manually from GitHub's UI, or both?
  • Someone from the tree-sitter org needs to add a PyPI api token as a secret to github actions. The secret should be named PYPI_API_TOKEN (although this doesn't really matter, just has to match the .yml file). It should have permission to upload to the tree-sitter project on pypi. Generating a token on pypi.org is quite straight-forward.
  • I need to copy my newer config from tree-sitter-builds to here, and delete the tree_sitter_languages related parts.
  • I currently use a patch file with git apply to make this project's tests work in cibuildwheel. I will need to figure out a cleaner way to do that, maybe an environment variable that is set when running under cibuildwheel or including the Python version in the language binary file name.
  • I need some way to test this pull request, after making all the changes. I believe there is a test version of pypi, so I can use that.

@lunixbochs
Copy link
Collaborator

This repo uses MIT license so we can't just bundle something that is Apache 2.0 licensed

You can certainly ship a wheel that has both MIT and Apache 2.0 code in it, just need to make sure all of the license files are present with correct attribution.

@maxbrunsfeld
Copy link
Contributor

maxbrunsfeld commented Sep 7, 2022

We need to agree on when this workflow runs. Currently releasing is done by a script that is ran locally and pushes a tag. Should this run automatically when a tag is pushed, or be triggered manually from GitHub's UI, or both?

Personally, I think just doing it based on any tag that starts with v and then a digit (or something like that) is a good approach.

Someone from the tree-sitter org needs to add a PyPI api token as a secret to github actions. The secret should be named PYPI_API_TOKEN (although this doesn't really matter, just has to match the .yml file). It should have permission to upload to the tree-sitter project on pypi. Generating a token on pypi.org is quite straight-forward.

I can take care of that.

I currently use a patch file with git apply to make this project's tests work in cibuildwheel. I will need to figure out a cleaner way to do that, maybe an environment variable that is set when running under cibuildwheel or including the Python version in the language binary file name.

I'm not familiar with cibuildwheel, maybe @lunixbochs can advise on that issue.

I need some way to test this pull request, after making all the changes. I believe there is a test version of pypi, so I can use that.

Maybe we just test by creating a tag with a qualifier like v0.20.2-beta0, -beta1 etc, so that we won't spam our PyPi package with bogus releases?

@maxbrunsfeld
Copy link
Contributor

Ok, the PYPI_API_TOKEN secret has been added, so it should be accessible to actions.

@Akuli
Copy link
Contributor Author

Akuli commented Sep 7, 2022

I'm not familiar with cibuildwheel

This is more of a thing with the tests than with cibuildwheel. The tests use Language.build_library() which looks at the modified time to check whether the library needs to be rebuilt. This works great, except that when cibuildwheel runs the tests with multiple Python versions, both 32-bit and 64-bit, the languages binary from the previous Python version is still there and so the tests fail to load it.

A few ways how I could work around it, not sure yet which I will choose:

  • Just delete the languages binary before running tests
  • Add an environment variable that forces the binary to be rebuilt, if it is set
  • Use an existing environment variable for this, probably some variable that is always set when running in cibuildwheel

@Akuli Akuli marked this pull request as draft September 7, 2022 19:21
@Carreau
Copy link

Carreau commented Sep 7, 2022

  • need some way to test this pull request, after making all the changes. I believe there is a test version of pypi, so I can use that.

You can just use GH upload action, and manually point pip to the locally downloaded wheels (or even download the wheel in another action).

Comment on lines 14 to 20
LIB_PATH = path.join(project_root, "build", "languages.so")
if os.getenv("PY_TREE_SITTER_TESTS_FORCE_REBUILD_LANGUAGES"):
try:
os.remove(LIB_PATH)
except FileNotFoundError:
pass

Copy link

Choose a reason for hiding this comment

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

Would it be worth it to have a .so name that contain the python version/platform in the same way that pyc files ends with for example cpython-310 ? That would avoid any conflicts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It actually has to be something like cpython-310-amd64, because 32-bit and 64-bit wheels are built in the same environment. I'll change this.

Copy link

Choose a reason for hiding this comment

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

Sure, and like also sys.platform to be sure ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Turns out that just placing the languages binary to current working directory works. This seems to be how cibuildwheel is meant to be used: it makes a new working directory for each wheel build just so that you can make a mess in it to be cleaned up before the next build.

@Akuli
Copy link
Contributor Author

Akuli commented Sep 7, 2022

@Carreau
Copy link

Carreau commented Sep 7, 2022

It is working: https://test.pypi.org/project/tree-sitter-test/

Working for me on mac M1 and Linux x86_64 for Python 3.10.

$ pip install tree-sitter-test --extra-index-url https://test.pypi.org/simple

If there are lurkers that don't know how to point pip at test-pypi

@Akuli Akuli marked this pull request as ready for review September 7, 2022 20:59
@Akuli
Copy link
Contributor Author

Akuli commented Sep 7, 2022

Ready for review!

@Akuli Akuli changed the title Configure GitHub Actions to build binary wheels for pypi Configure GitHub Actions to build binary wheels and upload to pypi Sep 7, 2022
Copy link
Contributor

@maxbrunsfeld maxbrunsfeld left a comment

Choose a reason for hiding this comment

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

I just left one minor question.

.github/workflows/pypi.yml Outdated Show resolved Hide resolved
@leotrs
Copy link

leotrs commented Dec 31, 2022

Would absolutely love to see this merged soon. It looks like it is ready to be merged?

EDIT: I have now been able to deploy a python app using tree-sitter-test from @Akuli's repository, whereas I had not been able to compile this repo's source on my docker image.

@amaanq amaanq merged commit 9239f3d into tree-sitter:master Aug 2, 2023
17 checks passed
@ofek ofek mentioned this pull request Aug 29, 2023
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.

6 participants