potassco / clingo

🤔 A grounder and solver for logic programs.
https://potassco.org/clingo
MIT License
606 stars 81 forks source link

Replace distutils with setuptools/sysconfig #464

Closed adamjstewart closed 11 months ago

adamjstewart commented 11 months ago

Distutils was deprecated in Python 3.10 and removed in Python 3.12.^1 This PR removes all uses of distutils to fix Python 3.12 support.^2

@haampie

rkaminsk commented 11 months ago

Thanks for the PR Adam. Actually, development happens in the wip branch of clingo where this issue (plus a few more) have already been fixed. It should be ready for Python 3.12.

See: https://github.com/potassco/clingo/blob/wip/cmake/python-site.py

adamjstewart commented 11 months ago

Interesting. Well the wip branch has the same bug, so I'll submit the PR for that branch instead.

rkaminsk commented 11 months ago

Interesting. Well the wip branch has the same bug, so I'll submit the PR for that branch instead.

It should not. Check the sys.version_info >= (3, 11) part.

adamjstewart commented 11 months ago

It works on 3.12, but the else block still contains the same bug and is using distutils instead. Is there any reason not to always use sysconfig?

rkaminsk commented 11 months ago

It works on 3.12, but the else block still contains the same bug and is using distutils instead. Is there any reason not to always use sysconfig?

This code came a long way and was already used for Python 2.7. It might have worked at some point with older setuptools versions.

haampie commented 11 months ago

The idea of showing users a clean and stable master branch as a landing page on Github sounds like a good idea, except that it is actually not very friendly for open source contributors, who do not know this wip branch exists.

I would suggest (again) to make the development branch the default, cause I've had this issue too: open a PR, oops wrong branch, close, rebase, open a new PR... it's a bit of a time waste. Also I backported the distutils workaround from the main branch to older clingo, only to realize it was broken, and the actual fix was only on the hidden wip branch.

So, it's giving more problems than it solves, people know how to click "releases" on Github for something stable.

rkaminsk commented 11 months ago

I had a look at the setuptools.sysconfig issue and it really dose not seem to exist. Maybe this was meant to be setuptools._distutils.sysconfig at some point.

rkaminsk commented 11 months ago

The idea of showing users a clean and stable master branch as a landing page on Github sounds like a good idea, except that it is actually not very friendly for open source contributors, who do not know this wip branch exists.

I would suggest (again) to make the development branch the default, cause I've had this issue too: open a PR, oops wrong branch, close, rebase, open a new PR... it's a bit of a time waste. Also I backported the distutils workaround from the main branch to older clingo, only to realize it was broken, and the actual fix was only on the hidden wip branch.

So, it's giving more problems than it solves, people know how to click "releases" on Github for something stable.

I still like the idea to have the master as the default. However, I also don't want to annoy contributors. For now, I'll add something to the readme and also put a PR template in place asking to use the wip branch as a base.

rkaminsk commented 11 months ago

It works on 3.12, but the else block still contains the same bug and is using distutils instead. Is there any reason not to always use sysconfig?

The reason is that it is working with distutils until Python 3.10. Clingo is build for quite a couple of platforms and the fine-tuning is a nightmare. Also functions like get_preferred_scheme are only available from python 3.10 onwards and would need a workaround.

It really looks like the try import block could be removed.

haampie commented 11 months ago

I still like the idea to have the master as the default. However, I also don't want to annoy contributors. For now, I'll add something to the readme and also put a PR template in place asking to use the wip branch as a base.

Thank you!

rkaminsk commented 11 months ago

Thanks.