build: replace deprecated setup.py with direct file installation#8711
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request removes the setup.py script and replaces the Python build and installation process with manual commands in Makefile.am. Feedback indicates that these changes inadvertently drop support for Python 2 bindings. It is recommended to restore the installation, uninstallation, and cleanup logic for Python 2 to maintain compatibility, following the new manual installation pattern established for Python 3.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request replaces the setuptools-based build and installation process for the SSSDConfig Python module with direct Makefile instructions. It removes src/config/setup.py, simplifies the install-exec-hook and uninstall-hook by using standard INSTALL_DATA and rm commands, and eliminates the need for Debian-specific layout options in the build configuration. I have no feedback to provide.
alexey-tikhonov
force-pushed
the
setup.py
branch
3 times, most recently
from
a05e132 to
4283ae4
Compare
|
@joakim-tjernlund, could you please test this patch? |
Builds OK with setuptools-scm-10.0.5 installed. Thanks |
justin-stephenson
left a comment
justin-stephenson
left a comment
There was a problem hiding this comment.
Does it make sense to follow steps in https://packaging.python.org/en/latest/guides/modernize-setup-py-project/#modernize-setup-py-project instead?
I personally would like to avoid introducing 'pip' dependency for a sake of a so trivial python "package"... |
| endif | ||
|
|
||
| install-exec-hook: installsssddirs | ||
| if BUILD_PYTHON2_BINDINGS |
There was a problem hiding this comment.
Can we remove BUILD_PYTHON2_BINDINGS ?
There was a problem hiding this comment.
Probably.
C10S doesn't support python2. Debian dropped its support around Debian 12.
Not sure about OpenSUSE (@scabrero, do you know?)
But I'd prefer if it would be a separate PR.
There was a problem hiding this comment.
But I'd prefer if it would be a separate PR.
Agreed.
There was a problem hiding this comment.
Probably. C10S doesn't support python2. Debian dropped its support around Debian 12. Not sure about OpenSUSE (@scabrero, do you know?)
But I'd prefer if it would be a separate PR.
Hi, python2 was dropped in Leap 15.4 so ok to remove.
sumit-bose
left a comment
sumit-bose
left a comment
There was a problem hiding this comment.
Hi,
thanks for the patch, the installed files are the same except the deprecated egg-info directory which is not installed anymore, ACK.
bye,
Sumit
setuptools has deprecated `setup.py install`. Since the SSSDConfig package is pure Python (3 files), replace setup.py build/install with direct $(INSTALL_DATA) calls in Makefile.am and remove the now-unnecessary setup.py.in template. Resolves: SSSD#8705 Assisted-By: Claude Code (Opus 4.6) Reviewed-by: Justin Stephenson <jstephen@redhat.com> Reviewed-by: Sumit Bose <sbose@redhat.com>
6 participants
setuptools has deprecated
setup.py install(see#8705). Since the SSSDConfig package is pure Python (3 files), replace setup.py build/install with direct $(INSTALL_DATA) calls in Makefile.am and remove the now-unnecessary setup.py.in template.
Resolves: #8705
Assisted-By: Claude Code (Opus 4.6)