search

Klipper3d / klipper

Klipper is a 3d-printer firmware
GNU General Public License v3.0
8.95k stars 5.16k forks source link

Update python-can to 4.3.x for Python 3.12 and later #6557

Closed nelsongraca closed 1 month ago

nelsongraca commented 2 months ago

This PR is a follow up on https://github.com/Klipper3d/klipper/pull/6550

In Python 3.12 setuptools is no longer installed by default on virtual environment creation (see https://docs.python.org/3/whatsnew/3.12.html and https://github.com/python/cpython/issues/95299) This is breaking Klipper when connecting through CAN due to missing dependencies.

Latest python-can does not depend on setuptools, a conditional requirement was added for Python 3.12 and later, the same way it is currently done with greenlet.

I have tested on my setup and no issues, CAN connects and I can print.

flowerysong commented 2 months ago

python-can 4.3 supports Python 3.8 and later, not just 3.12.

nelsongraca commented 2 months ago

@flowerysong I just want to fix it from the version where it broke forward (3.12), there may be a side effect I haven't experienced and the less margin for error the better.

flowerysong commented 2 months ago

If you can't do sufficient testing to be sure that this won't break things, it shouldn't be listed as a dependency for any version. If you can, it should be used as widely as possible.

nelsongraca commented 2 months ago

So keep CAN completely broken on Python 3.12 and later?

As mentioned before, I did not find any issues but I'm not going to test every other Python version, there may be any sort of configuration that I'm not aware and break.

nelsongraca commented 2 months ago

Any chance this will ever be merged? or will be left like many other PRs becoming stale

nelsongraca commented 2 months ago

@KevinOConnor I'm not fond of tagging you directly but I'd like your take on this, if anyone installs Klipper with a new Python 3.12 venv and is using CAN it will be broken unless the user manually updates or installs a dependency, IMO being something that breaks a specific area of Klipper (CAN) makes it a little urgent.

KevinOConnor commented 1 month ago

Thanks. I'm leery of adding conditional requirements, as it increases maintenance costs (when troubleshooting what versions users are running become important as users run different software). If there is no version of python-can that runs on all python versions of interest, is there a version of setuptools that runs on all python versions of interest?

-Kevin

nelsongraca commented 1 month ago

Hey Kevin, thank you for the feedback.

That was more the route I was taking with the previous PR https://github.com/Klipper3d/klipper/pull/6550

I believe we can just add setuptools, I would do it conditionally for Python 3.12 or later since previous versions it's installed by default on venv creation, but there might be no harm if done on every version as it's am effective dependency because of python-can.

KevinOConnor commented 1 month ago

Okay, thanks. I think it would be preferable if we can find a version of setuptools that works everywhere.

-Kevin

github-actions[bot] commented 1 month ago

It looks like this GitHub Pull Request has become inactive. If there are any further updates, you can add a comment here or open a new ticket.

Best regards, ~ Your friendly GitIssueBot

PS: I'm just an automated script, not a human being.