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

Update to Poetry #156

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

pdietl
Copy link
Contributor

@pdietl pdietl commented Sep 16, 2022

The setup.py-style builds a deprecated. PEP 517 introduced the pyproject.toml way of packing Python projects. This PR switches to this new mechanism and uses the Poetry build system. The advantage of this system is that a Rust-inspired lock file is created to lock down exact dependencies and and all but guarantees that future users will be able to build and use the project.

I was inspired to make this change after running into a dependency problem where one package required Click<8.0.0 and another required Click>=8.0.0. The was probably not a problem at some point in the past, but because there was no lock file, a transitive dependency got updated and the conflict was introduced.

@pdietl pdietl force-pushed the pete/update_to_poetry branch 7 times, most recently from 8c6b63e to 0c717d3 Compare September 16, 2022 20:09
@pdietl
Copy link
Contributor Author

pdietl commented Sep 16, 2022

@ALSchwalm does the last GitHub Actions failure looks like a race condition bug in the test to you? (Also, Hi!)

@ALSchwalm
Copy link
Owner

Hey! Poetry is definitely an improvement and switching to it is probably a good idea. I had some trouble testing this due to some weird keyring expectations from poetry (seemingly relevant to python-poetry/poetry#1917), which for reference can be resolved by setting PYTHON_KEYRING_BACKEND=keyring.backends.null.Keyring, if you don't have a keyring because you keep all your passwords on a post-it note like me. Some questions:

  • Is there a requirement to drop 3.6 support? Even if the poetry itself doesn't support 3.6 (idk if it doesn't), I assume users installing a wheel should be fine, so we can still test against the older platforms. If dropping it is required, we'll need to update the docs as well
  • Is there a way to keep the version in the toml file in sync with the version in source? I don't know much about poetry conventions
    • Either way, this will definitely require a version bump

All of the tests are passing locally for me, so I'd presume any issues your hitting are related to the fact that qemu can't be run with hardware acceleration in CI, so it is very slow. The timeouts for things are generous but still can just expire sometimes. I can take a closer look in a bit.

@pdietl
Copy link
Contributor Author

pdietl commented Sep 18, 2022

Oh man the key ring issue is definitely no bueno. Are still ok using Poetry or would you rather we switch to some other build system?

Regarding Python 3.6 support, I am pretty sure we can keep support for it. I'll just have to play around more precisely with dependency versions to find a combination that works. This shouldn't be too difficult.

@ALSchwalm
Copy link
Owner

It seems like poetry has the momentum behind it (to my knowledge). The devs in that thread seem to indicate that these kinds of behaviors will be fixed over time, so I think I'm ok switching to it as long as a user that just pip installs the wheel from pypi isn't impacted.

@pdietl pdietl force-pushed the pete/update_to_poetry branch 2 times, most recently from 03db7a9 to 35afd27 Compare September 19, 2022 16:45
@pdietl
Copy link
Contributor Author

pdietl commented Sep 19, 2022

Dropping Python 3.6 seems necessary. Since it is no longer supported, some dependencies are not happy with Python 3,6 anymore.

The setup.py-style builds are deprecated. PEP 517 introduced the pyproject.toml
way of packing Python projects. This commit switches to this new mechanism and
uses the Poetry build system. The advantage of this system is that a
Rust-inspired lock file is created to lock down exact dependencies and all but
guarantees that future users will be able to build and use the project.

Further, this commit fixes up CI to use Poetry.

Support for Python 3.6 is dropped since it is no longer supported and the
gherkin-formatter does not want to play nice with 3.6.

Testing against Python 3.10 is added.

Other file changes are reformatting due to new formatter versions.
@pdietl
Copy link
Contributor Author

pdietl commented Oct 1, 2022

@ALSchwalm can you help me fix this type error?

@ALSchwalm
Copy link
Owner

It appears that type info has been added for lark which has revealed the incorrect type info in the build.py file. Probably not too bad to fix that with some helpers to check the types of the children of ast nodes and ensure things are as expected.

I can probably tackle this in the next few days. Looks like you've resolved everything else though! Thanks for working on this

@ALSchwalm
Copy link
Owner

Finally got around to this, should be fixed on your branch now

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.

2 participants