-
Notifications
You must be signed in to change notification settings - Fork 770
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
Conversation
great, thanks for contributing! some notes: IMO the most invasive change should be checked, mentioned/discussed - the removal of |
ah, great! missed that. fwiw, np removing deprecated notes/comments like this!
is that "official"? if not, I don't care, as long as it works, don't touch would be my preference .. |
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) |
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" as in: depending on "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) ... |
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? |
yes indeed, deliberately / by design (currently), so leaving it alone is appreciated, thanks!
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
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 ? @meejah any opinions how to move on?
|
for comparison, this PR also triggers the aio issue (but not the other things failing here ...) #1630 |
look like the same failures to me - just variation in which checks run and fail first, and which get cancelled as a result? |
Yeah , could be.
David Hotham ***@***.***> schrieb am So., 24. März 2024,
14:44:
… look like the same failures to me - just variation in which checks run and
fail first, and which get cancelled as a result?
—
Reply to this email directly, view it on GitHub
<#1632 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABY67C2DT6Y4DBRTWV2FI3YZ3KEJAVCNFSM6AAAAABFEWD572VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMJWHAYTINZVGA>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
"In general" moving away from The changes here look plausible to me (but of course it's also less terrifying to merge when things are green :) ) |
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
@oberstet my real motivation in fixing the unit tests was to get this green and - hopefully - merged |
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! |
sounds good, thanks. Though I am slightly confused, this pull request is not yet merged? |
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;) |
I noticed in passing that this claim in setup.py
has not been true for some time, but left it alone