lark-parser / lark

Lark is a parsing toolkit for Python, built with a focus on ergonomics, performance and modularity.
MIT License
4.88k stars 414 forks source link

Moved the metadata into `PEP 621`-compliant `pyproject.toml`. #1157

Closed KOLANICH closed 1 year ago

erezsh commented 2 years ago

Thanks, but I think we'll stick to setup.py until pyproject.toml can extract the version from lark/__init__.py.

KOLANICH commented 2 years ago

but I think we'll stick to setup.py until pyproject.toml can extract the version from lark/init.py.

It is a feature not of pyproject.toml, but of build backends and plugins like read_version. setuptools can do it, and this feature is used here. But I think that the version should be extracted from git, not from source files.

erezsh commented 2 years ago

@KOLANICH I think it's great that you can do lark.__version__ and get the version, very helpful for debugging. I'm actually annoyed that a lot of packages don't do that these days.

KOLANICH commented 2 years ago

setuptools_scm has a feature to generate a python source code file for a version extracted from git tags. I can utilize it instead of fetching version from __init__.py.

erezsh commented 2 years ago

That sounds like a roundabout solution, but I'm willing to consider it.

henryiii commented 2 years ago

See https://scikit-hep.org/developer/pep621#versioning for an example with hatchling (which works with setuptools_scm). Most of https://scikit-hep.org/developer/packaging#git-tags-official-pypa-method is valid if you want to stick with setuptools, just ignore the setup.cfg mentions. :)

KOLANICH commented 1 year ago

if some limits setuptools<60 in a non isolated environment (cough, NumPy, cough)

I heard something about it (only a bit, I have not yet lurked about the issue), but I think it is an issue of numpy. If some package uses < constraints, it is that package that is broken (it doesn't mean the ones that require <= shouldn't lover the version on RHS of the expression).

You don’t need wheel - it’s added by setuptools via PEP 517 and it’s likely to be going away at some point.

Thanks for noting this, I'll fix it.

erezsh commented 1 year ago

Cool, that looks elegant! Do you think the transition will be entirely seamless, or are there any details to be aware of?

KOLANICH commented 1 year ago

Do you think the transition will be entirely seamless, or are there any details to be aware of?

It depends on your assumptions on Python and setuptools versions users have and on their workflows. Since setuptools has dropped old Python versions. I mean if we say that users of certain versions and/or certain workflows (i.e. the ones who hardcode python3 ./setup.py in their scripts, now they would have to switch to using python3 -m build) are out of scope, then yes, maybe it can be considered "seamless" for some.

Every change breaks someone's workflow and there is ain't no such a thing as "seamless".

xkcd "Workflow"

erezsh commented 1 year ago

This is the first time I hear of python3 -m build.

Is there a guide or tutorial for transitioning from setup.py, for those of us who are out of scope? ;)

KOLANICH commented 1 year ago

Here are the docs: https://pypa-build.readthedocs.io/en/latest/

python3 -m build is a backend-neutral way to trigger build (now setuptools is just one of the backends). PEP 517 and PEP 518 are used to specify the backend, be it setuptools, or something else (like flit or hatchling). Those backends just have no setup.py. PEP 621 is just a backend-neutral format for metadata instead of the total mess that happenned after PEP 517 got implemented in pip, when each backend used to have an own format. I cannot say the mess is fully cleaned, but PEP 621 is surely an improvement.

If one wants to migrate other projects, I recommend to do the following 2-step process (I did it myself for all my projects, and also for this one):

  1. use setuptools_py2cfg to move the metadata into setup.cfg. Create a PEP 617-compliant pyproject.toml. Delete setup.py.
  2. use ini2toml to convert the metadata into pyproject.toml. Delete setup.cfg.

After each step manual tweaking of the configuration may be needed and you should be sure that the project builds. It is better to introduce setuptools_scm after the step 1.

henryiii commented 1 year ago

There’s also https://blog.ganssle.io/articles/2021/10/setup-py-deprecated.html

https://scikit-hep.org/developer is a good place for modern packaging suggestions. Also I’m fond of hatch new --init, which converts setup.py/setup.cfg directly to hatchling.

Many PyPA packages have moved, like packaging and wheel. So it’s pretty safe.

henryiii commented 1 year ago

And personally I almost always use pipx run build. :)

erezsh commented 1 year ago

Hello! Sorry it took me this long to look into this. Thank you for the info and the links. I found scikit hep especially interesting.

I'm generally in favor of the change.

Just one question - if I understand correctly, users are now expected to run "pytest" instead of "setup.py test". Does it mean we should also include pytest in the dev-requirements?

Or is it up to the user to install it, or use pipx run pytest?

henryiii commented 1 year ago

FYI, scikit-hep.org/developer has been mostly migrated to https://learn.scientific-python.org/development/

Yes, setup.py test has been deprecated for years. You do want pytest in dev-requirements (I usually have a [test] extra, so a user can install -e.[test]).

Usually you don't want use pipx for pytest, since pytest imports your package, its requirements, and plugins; it's more like a library than an app. In some simple cases it does work, but it's generally designed to run alongside your package.

KOLANICH commented 1 year ago

My approach to testing is writing test suites using vanilla unittest and sometimes with standalone libs not related to pytest (to allow them be run without pytest which is large and slow and a dependency, and pytest CLI tool is smart enough to understand such tests), but to run tests using pytest on CI in order to generate reports (on GitLab CI these reports can be integrated into GUI by pointing the report file in the config). Locally I usually use pytest --lf or just ./tests/tests.py depending on what is faster (pytest initialization is quite slow).

erezsh commented 1 year ago

So, everyone okay with merging this as-is? We can add a [test] extra in a separate PR.

@MegaIng Feel free to chime in too, if you have something to add.

MegaIng commented 1 year ago

I don't have a lot of concrete experience with this stuff and therefore don't have anything to add.

I am okay with merging.

erezsh commented 1 year ago

Thanks everyone!