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

Remove use of distutils #554

Merged
merged 1 commit into from
Jun 26, 2024

Conversation

terop
Copy link
Contributor

@terop terop commented Dec 30, 2023

As of Python 3.12 setuptools is no longer installed by default and if setuptools is not installed distutils is not found and SafeEyes fails to start.

Furthermore distutils is deprecated and should thus not be used anymore. The packaging module provides a replacement for LooseVersion and it is added in this commit.

Copy link
Contributor

@nifadyev nifadyev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, Thanks for your effort @terop. Please add link to PEP 632 with migration advices
Hopefully, building using setuptools will be replaced as well.
Tested with Python 3.12.1, Fedora 38, GNOME 44.6

@terop terop force-pushed the remove_looseversion branch from d1eadd3 to 55b6cff Compare June 25, 2024 17:21
@terop
Copy link
Contributor Author

terop commented Jun 25, 2024

PEP 632 reference added.

@deltragon
Copy link
Collaborator

Should the new dependency also be added to the debian/control file? (Adding a dependency on https://packages.debian.org/bookworm/python3-packaging there)

@terop
Copy link
Contributor Author

terop commented Jun 26, 2024

@deltragon That's a good question which I cannot adequately answer as I have never done Debian packaging. If you think it should be added I can certainly add it.

@archisman-panigrahi
Copy link
Collaborator

archisman-panigrahi commented Jun 26, 2024

Should the new dependency also be added to the debian/control file? (Adding a dependency on https://packages.debian.org/bookworm/python3-packaging there)

Yes, we need to add python3-packaging to the build-depends field in debian/control. https://github.com/slgobinath/SafeEyes/blob/master/debian/control#L5

(I have been doing deb packaging for several years)

@terop terop force-pushed the remove_looseversion branch from 55b6cff to 1844649 Compare June 26, 2024 17:10
@terop
Copy link
Contributor Author

terop commented Jun 26, 2024

Thanks for your input @archisman-panigrahi. I added python3-packaging per your suggestion.

@archisman-panigrahi
Copy link
Collaborator

In that case, we should bump python version requirement to 3.12+ in debian/control, and also remove python3-setuptools from the build-depends.

@terop terop force-pushed the remove_looseversion branch from 1844649 to 2d2748f Compare June 26, 2024 17:43
@terop
Copy link
Contributor Author

terop commented Jun 26, 2024

Changes done. @archisman-panigrahi can you kindly check if the latest changes look correct?

@archisman-panigrahi
Copy link
Collaborator

I verified that the deb package builds with your PR in Ubuntu 24.04 and it runs fine.
However, also please update python3 version requirement in this line.

Otherwise, I approve merging

As of Python 3.12 setuptools is no longer installed by default and
if setuptools is not installed distutils is not found and SafeEyes
fails to start.

Furthermore distutils is deprecated and should thus not be used
anymore. The packaging module provides a replacement for LooseVersion
and it is also recommended by PEP 632 [1].

Also update required Python version for Debian to 3.12 or newer.

[1] https://peps.python.org/pep-0632/#migration-advice
@terop terop force-pushed the remove_looseversion branch from 2d2748f to 5c0884f Compare June 26, 2024 18:01
@archisman-panigrahi
Copy link
Collaborator

Looks good, and I approve merging.

@archisman-panigrahi archisman-panigrahi self-requested a review June 26, 2024 18:03
@archisman-panigrahi archisman-panigrahi merged commit 55693cb into slgobinath:master Jun 26, 2024
1 check passed
@archisman-panigrahi
Copy link
Collaborator

By the way, is this line still necessary, if setuptools is being discontinued? https://github.com/slgobinath/SafeEyes/blob/master/setup.py#L3

@terop terop deleted the remove_looseversion branch June 26, 2024 18:16
@terop
Copy link
Contributor Author

terop commented Jun 26, 2024

I suppose we are not discontinuing setuptools, at least not in this PR.

@archisman-panigrahi
Copy link
Collaborator

archisman-panigrahi commented Jul 10, 2024

@terop @deltragon Does anyone know if safeeyes would work with older versions of python after removing the use of distutils? Right now the minimum supported python version is set to be 3.12 in debian/control

However, if safe eyes works with older versions of python, I can backport it to older versions of Ubuntu in the PPA.

Edit: See next comment

@archisman-panigrahi
Copy link
Collaborator

archisman-panigrahi commented Jul 10, 2024

I tested in virtualbox, and it turns out that the current version of safeeyes does not work with python 3.8 in ubuntu 20.04 since this commit edb5b70 (that is fine, 20.04 has a working version of safeeyes in its official repositories, and we don't need to continue supporting very old versions of ubuntu).

So we can just support 22.04+ (python 3.10+).

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.

4 participants