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

Migrate from setup.py to poetry/pyproject.toml #169

Draft
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

white-gecko
Copy link
Contributor

Remove setup.py, add pyproject.toml.
I wanted to execute the tests with python setup.py test and received the message:

RuntimeError: Support for the test command was removed in Setuptools 72

I then tried to get pytest running directly but again got issues urllib3 version.
So I thought migrating to poetry would anyhow be a good benefit. I hope this suites you.

@white-gecko
Copy link
Contributor Author

The tests pass on my side: https://github.com/white-gecko/warcio/actions/runs/10368291162

@white-gecko white-gecko marked this pull request as draft August 13, 2024 12:55
@wumpus
Copy link
Collaborator

wumpus commented Aug 13, 2024

We can use pytest instead of "python setup.py test" without migrating -- @tw4l do you have a preferred direction that you're going towards for python testing? If not, I'm happy to make the smallest possible change.

@tw4l
Copy link
Member

tw4l commented Aug 13, 2024

We can use pytest instead of "python setup.py test" without migrating -- @tw4l do you have a preferred direction that you're going towards for python testing? If not, I'm happy to make the smallest possible change.

Hi Greg, making a small switch to calling pytest directly sounds great. We tend to do that and use GH Actions' in-built functionality for testing different Python versions and such rather than rely on something like tox.

@wumpus
Copy link
Collaborator

wumpus commented Aug 13, 2024

Thank you for weighing in @tw4l . @white-gecko I do appreciate this example of switching to poetry.

For "Skip test_capture_https_proxy" do you think there's an easy way to fix it? I faintly recall this is am urllib problem.

@white-gecko
Copy link
Contributor Author

I just thought it is easier to maintain dependencies using something like poetry and the now recommended pyproject.toml. But if you prefer to keep changes minimal for now, this is fine for me as well. I will try to rework my pull-request.

@white-gecko white-gecko marked this pull request as draft August 14, 2024 15:12
@tw4l
Copy link
Member

tw4l commented Aug 14, 2024

Ah looking at the context a little bit more, I'm certainly not opposed to moving to pyproject.toml. And poetry does seem nice, though we're not using it for any other Webrecorder tools currently. @ikreymer, do you have thoughts on this?

I'm curious to see how this ecosystem will evolve now that we have a draft PEP for standardized Python installation reproducibility via lock files.

@tw4l
Copy link
Member

tw4l commented Aug 14, 2024

For "Skip test_capture_https_proxy" do you think there's an easy way to fix it? I faintly recall this is am urllib problem.

Thankfully my past self thought to add the reminder into the last commit message:

Modify handling of proxies in test_capture_http_proxy module to account
for breaking changes in requests and urllib3 (note that this currently means
pinning urllib3==1.25.11, as more recent versions no longer allow using
http:// scheme URLs with the https key in the proxies dictionary; depending
on whether and how this is resolved, we may need to come back to this in the
future but for now it gets CI working again)

I remember it being a bit thorny, would be great to get those tests working again but I don't have a quick answer for how to best go about doing that at this moment.

@wumpus
Copy link
Collaborator

wumpus commented Aug 16, 2024

Thank you for splitting the PR!

Also I just accidentally pushed directly to master, don't follow my example (arrrrrrgh)

@white-gecko
Copy link
Contributor Author

Since this, somehow it the meta-issue of my changes. #168, #170, #171, #172, #173, #174, and #175 are ready for review and to be merged from my perspective. (#175 requires #172 to be merged before.) @tw4l @wumpus

@wumpus
Copy link
Collaborator

wumpus commented Aug 20, 2024

Thank you, I'm working through the PRs in order.

@wumpus
Copy link
Collaborator

wumpus commented Aug 23, 2024

I have now merged all of the pieces EXCEPT poetry.

  • do we still want to switch?
  • we need to revive windows testing
  • we should add darwin testing

I'm willing to do the latter two.

@wumpus
Copy link
Collaborator

wumpus commented Aug 23, 2024

Oh and should we release the current master branch as a new pypi version? @tw4l presumably you have pypi credentials for the package.

@white-gecko
Copy link
Contributor Author

Thank you for going through my pullrequests. If we still want to move to poetry, I can rework this pullrequest.

@tw4l
Copy link
Member

tw4l commented Aug 23, 2024

Pinging @ikreymer to weigh in on the release and Poetry questions. Thanks @white-gecko and @wumpus for all the work on those PRs!

@wumpus
Copy link
Collaborator

wumpus commented Aug 29, 2024

I added testing for windows and darwin to the CI, and fixed the long-standing brotli install confusion using the CI to test.

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