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: add build-system via pyproject.toml #25

Merged
merged 1 commit into from
Jul 21, 2023

Conversation

s0h3yl
Copy link
Contributor

@s0h3yl s0h3yl commented Jul 20, 2023

Changes

Allows the project to be built w/o the manual installation of cython.

Allows the project to be built w/o the manual installation of `cython`.
@thequilo
Copy link
Member

We initially intentionally didn't add a pyproject.toml file because it used to destroy --user -e installs. But I tested it just now and it seems to work, at least for me.

@boeddeker can you test this on your machine?

We could also push a release to PyPI once you can install it without having to manually install cython before.

@boeddeker
Copy link
Member

It looks like the user install might be solved (pypa/pip#7953).
But I remember, that some packages require an older setuptools version, that may not have this fix.
But I forgot which package that was.

I am not sure, if it is worth to add the pyproject.toml:

  • The import error has a clear error message and most people know, that they have to type then pip install ... (Those that don't know it, can easily find the solution via a search engine).
  • The errors messages that are triggered by pyproject.toml are confusing and usually it is not trivial to solve them.

Although in a scientific environment, Cython should already be installed, so this issue usually happens in CI instances.

@s0h3yl
Copy link
Contributor Author

s0h3yl commented Jul 21, 2023

@boeddeker I agree, that reading the README.md makes it clear.

Yet, we're using bazel as the build system, which needs PEP-518 compliant packages in order to make them installable - or rather packages that won't need manual steps to be done beforehand.

But I remember, that some packages require an older setuptools version, that may not have this fix.

If you are able to identify that package, you could add version constraints to the given setuptools package.

requires = ["setuptools<68.0.0", "wheel", "Cython"]

In any case, I respect the decision that will be made here. In the event that this PR isn't making it in, we'll use the forked version of this repository and maintain our patch separately.

Thanks in advance! 🙇

@boeddeker
Copy link
Member

Yet, we're using bazel as the build system, which needs PEP-518 compliant packages in order to make them installable - or rather packages that won't need manual steps to be done beforehand.

That is an argument, I was waiting for. I didn't know, that software exists, that requires the pyproject.toml.
Until now, it was for me just an ideological change.

If you are able to identify that package, you could add version constraints to the given setuptools package.

We try to delay to breaking old environments, when there is no advantage.
That is the reason, why I wanted to delay the usage of pyproject.toml as long as possible to keep old envs running.

For this package, there is no critical dependency. Our experiment code depends on several packages, and some have difficult version constraints.
Since it is working in Thilos ens, my older envs, and we have a real argument, I think we should merge this PR.

@thequilo thequilo merged commit 2f877ff into fgnt:main Jul 21, 2023
@s0h3yl
Copy link
Contributor Author

s0h3yl commented Jul 21, 2023

Thank you, @thequilo and @boeddeker! 🙇

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.

3 participants