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

a couple of packaging fixes #1632

Merged
merged 1 commit into from
Apr 13, 2024
Merged

a couple of packaging fixes #1632

merged 1 commit into from
Apr 13, 2024

Conversation

dimbleby
Copy link
Contributor

  • use pep 508 markers to statically express dependencies
  • remove deprecated use of setup.py as a command line tool

I noticed in passing that this claim in setup.py

 # wsaccel does not provide wheels: https://github.com/methane/wsaccel/issues/12

has not been true for some time, but left it alone

@oberstet
Copy link
Contributor

oberstet commented Mar 24, 2024

great, thanks for contributing! some notes: IMO the most invasive change should be checked, mentioned/discussed - the removal of setuptools (the package dep). I am not sure where it is used/needed, probably not only for installation ...

@oberstet
Copy link
Contributor

has not been true for some time, but left it alone

ah, great! missed that. fwiw, np removing deprecated notes/comments like this!

remove deprecated use of setup.py as a command line tool

is that "official"? if not, I don't care, as long as it works, don't touch would be my preference ..

@dimbleby
Copy link
Contributor Author

dimbleby commented Mar 24, 2024

https://packaging.python.org/en/latest/discussions/setup-py-deprecated/

I did not remove the package dependency on setuptools (iirc it is needed because of use of pkg-resources)

@oberstet
Copy link
Contributor

oberstet commented Mar 24, 2024

ok, thanks a lot for digging and for the link!

this link should be in the doc comments somewhere .. so that the any change here can be traced back to the reason that triggered the change

However, python setup.py and the use of setup.py as a command line tool are deprecated.

"however" as in: depending on setuptools is not deprecated or discouraged

"the problem is: this PR does both (remove use of setup.py and setuptools)" - well, actually this is WRONG;) sorry, missed it. anyways, we should IMO definitely keep the setuptools package (for now) ...

@dimbleby
Copy link
Contributor Author

Setuptools is a default build requirement and does not need to be set explicitly for installation. The proof of that pudding is the successful wheel builds.

This project also uses setuptools as a runtime requirement, and I have left this alone. The proof of that pudding should be in passing unit tests - I don't think the changes here can be responsible for the CI failures?

@oberstet
Copy link
Contributor

oberstet commented Mar 24, 2024

This project also uses setuptools as a runtime requirement, and I have left this alone.

yes indeed, deliberately / by design (currently), so leaving it alone is appreciated, thanks!

The proof of that pudding should be in passing unit tests

yes, I agree, and it (ABPy without the PR) might not have coverage =( well .. "should" .. I definitely agree! that would allow easy checking of why setuptools is a run-time dep anyways - which I forgot, but there was "something" .. I seem to remember

I don't think the changes here can be responsible for the CI failures?

nope. it is related to breaking changes in asyncio / asyncio-test related to loop handling :(

the flake8 and twisted tests run all fine, so this strongly suggests this PR is good .. I am just not sure how to handle that: should we

a) wait until the aio thing is fixed after which eg this PR should run 100% green (after merging any changes from the former into this PR), OR should we
b) merge this PR right aways as it is "likely" good even though not 100% green (because of the unrelated aio stuff)

?

@meejah any opinions how to move on?

also, complaining: why doesn't GH allow me to request reviewers anymore? did they break their shit, or what's going on? anyways. stuff.

@oberstet
Copy link
Contributor

for comparison, this PR also triggers the aio issue (but not the other things failing here ...) #1630

@dimbleby
Copy link
Contributor Author

look like the same failures to me - just variation in which checks run and fail first, and which get cancelled as a result?

@oberstet
Copy link
Contributor

oberstet commented Mar 24, 2024 via email

@meejah
Copy link
Contributor

meejah commented Mar 27, 2024

@meejah any opinions how to move on?

"In general" moving away from setup.py is a good thing. Best-practice now is to use "configuration only" for most tasks (e.g. via pyproject.toml). Unfortunately, there's now even more Python packaging tools available than last time I made a new project haha 😢 but there are also PEPs to make them play more-nicely together.

The changes here look plausible to me (but of course it's also less terrifying to merge when things are green :) )

@meejah
Copy link
Contributor

meejah commented Mar 27, 2024

Also, is there a bug specifically about the asyncio changes?

I assume that refers to the "event loop is closed" errors in the logs. It would be ideal to tie this back to the specific asyncio changes related to this...

@oberstet
Copy link
Contributor

Also, is there a bug specifically about the asyncio changes?

I assume that refers to the "event loop is closed" errors in the logs. It would be ideal to tie this back to the specific asyncio changes related to this...

I think this the change in pytest-asyncio discussed here triggered the regression #1628 (comment)

- use pep 508 markers to statically express dependencies
- remove deprecated use of setup.py as a command line tool
@dimbleby
Copy link
Contributor Author

@oberstet my real motivation in fixing the unit tests was to get this green and - hopefully - merged

@oberstet
Copy link
Contributor

my real motivation ...

sure, I get that - it's fixed and merged! the minimum twisted version is also bumped, and it'll all be in the next autobahn release. thanks for contributing!

@dimbleby
Copy link
Contributor Author

sounds good, thanks. Though I am slightly confused, this pull request is not yet merged?

@oberstet oberstet merged commit 50e6297 into crossbario:master Apr 13, 2024
11 checks passed
@oberstet
Copy link
Contributor

Though I am slightly confused, ...

sorry, I got confused ... merged that other PR ... which actually (now that you force pushed onto this PR) now also makes this one green ... so I (only) now merged this one as well;)

anyways, as @meejah notes, moving to configuration based setup etc is "welcome/progress" - and I hope there won't be lots of fallout;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants