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

Build backend updated to poetry core #119

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

Conversation

danielemichilli
Copy link

This solves issue #118 and makes the repository compatible with poetry. To do so, it updates the style of pyproject.toml and uses its light weight, fully compliant, self-contained package allowing PEP 517 compatible build frontends, solving the error indicated in the issue

@jrs65
Copy link
Collaborator

jrs65 commented May 7, 2022

Can you explain more why the issue you link to is an issue in bitshuffle? That poetry doesn't install the listed build requirements before trying to buld the package seems like an issue in poetry (quite possible this issue: python-poetry/poetry#2789).

Generally I'm interested in moving a bunch of packages to more modern build methods but in my (limited) past experience poetry is the most frustrating of them all. At the moment the current setup.py/setuptools builds mostly works fine so a high premium should be placed on changing it over.

Are there tweaks to the pyproject.toml file that would make poetry install Cython without changing the build backend to poetry?

@jrs65
Copy link
Collaborator

jrs65 commented May 7, 2022

Anyway @danielemichilli I'm on vacation for two weeks, but it would be good to figure out the pros and cons of this when I'm back. Otherwise Kiyo might be able to deal with it in the mean time.

@danielemichilli
Copy link
Author

danielemichilli commented May 9, 2022

This is not necessarily an issue with bitshuflle but I think making it poetry-compatible is a good idea if it does not cause problems with the module. I was able to install bitshuffle without issues after my edits with or without poetry but I am not aware of all the building methods that are necessary to be supported. I believe this would be a quick test to run, in the meantime, enjoy your vacation!

Are there tweaks to the pyproject.toml file that would make poetry install Cython without changing the build backend to poetry?

I have looked into this but I did not find a way to do it.

@shinybrar
Copy link

@danielemichilli @jrs65 -- looking into this issue and here are some observations.

  • The issue is not pyproject.toml -- it is setup with sufficient parameters.
  • The issue arises from the current layout of setup.py itself. You can replicate this issue by simply following the commands below:
cd bitshuffle
docker run -it -v $(pwd):/bitshuffle python:3.7-slim python /bitshuffle/setup.py --help
Traceback (most recent call last):
  File "/bitshuffle/setup.py", line 13, in <module>
    from Cython.Compiler.Main import default_options
ModuleNotFoundError: No module named 'Cython'

You need Cython installed in the environment to even evaluate the install and setup directives. This breaks any setup command other than install.

Most third-party dependency resolution systems, pipx, poetry, flit etc. use egg_info to fetch metadata, build tags and dependency links for the project. Note, this is outside the purview and different from install_requires and setup_requires dependencies which are correctly defined in pyproject.toml.

The solution is quite simple,

  1. Move the cython imports under a separate function
  2. Call the said function from within setup() which is invoked after setup_requires.

@shinybrar
Copy link

I would recommend closing this PR in favor of #123

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.

None yet

3 participants