modernised python packaging #493
Conversation
|
AppImage seems to work just fine on my Debian and this doesn't touch any of the deb packages, so from my side that seems to be covered. One thing that isn't is that your new method assumes that end user will always want the GUI. Which is reasonably fair assumption on a client, but not so much in case of headless servers. Do you have some ideas how to have an optional set of requirements? Preferably, but not necessarily while keeping the GUI libraries as default. See the #416 for broader discussion. |
|
The changes introduced in #416 is what i came up with basically (those changes can easily be translated to the setup.cfg file). But that results in the following setup: i noticed that @FallenWarrior2k head searched for a way to make the GUI default and couldn't find one. I am inclined to believe them. But I will look around a bit more to see if it possible without any workarounds. |
|
I believe the extras-based approach is blocked on pypa/setuptools#1139. |
|
Also to add some information, I tried to see if i could use a venv with pip to run the syncplay code on both a fresh ubuntu and Fedora VM. Fedora: The reason i am pointing out my experience is to show that getting gui working with pip based install method probably wont be straighforward. |
|
The fact that pyside2 installed from pip doesn't bring with itself whole Qt seems obvious in hindsight and largely invalidates my original argument behind focusing on gui. Without that in the way, the dependency chain of PySide2 is nowhere near as huge and wouldn't really have much of a practical impact. |
|
But I do agree with @FallenWarrior2k in that having at least the headless server package easily available on pypi would be awfully convenient. Maybe we can make just the server package available through PyPI? |
|
@daniel-123 I also want to point out that with this commit, all i have done is translate the old setup.py file to the new format. The behaviour is the same, as in the old setup.py file also assumed that the end user will always want the GUI client. What I want to point out is that making syncplay avaialable on PyPI is not the primary objective of this pull request, but instead to make it easier to follow packaging guidelines in fedora for projects developed in python. I will make a copr repo by tomorrow for 1.6.9 and link it here |
|
Before staring the review, just FYI, PySide2 wheels contain their own copy of Qt. So, in principle, there should be no need to resort to This is the case for Ubuntu 20.04.3 and PySide2 5.15.2, in which the library missing is In short, I would not consider impossible to install Syncplay with GUI from PyPi, provided that all the dependencies behave as they should. Nevertheless, I do not think that PyPI should be a supported distribution channel for this project, as we already have quite a few. We actually invest a lot of effort for providing binaries specifically prepared to avoid having to use the sources. In addition, I do not see the benefits of PyPi over plain |
| [options] | ||
| include_package_data = True | ||
| packages = find: | ||
| install_requires = |
There was a problem hiding this comment.
I am not a fan of duplicated information, and switching to PEP 621 without touching the requirements*.txt files does exactly that.
I would be in favor of deleting the requirement files, but currently the Windows build system relies on them. This could be circumvented by using pip-compile e.g. via
pip-compile --output-file requirements.txt setup.cfg
in the CI workflows before the file is actually needed.
On the other hand, we could retain the requirements files and start using them in the proper way: to pin dependencies for CI/release builds. We actually need to pin dependencies on macOS but, for the moment I did that in the workflow code (very bad, but I did not have a proper alternative up to now).
@Et0h @daniel-123 what is your opinion here?
There was a problem hiding this comment.
@Et0h @daniel-123 what is your opinion here?
As someone who is a Windows user I don't have any strong opinions on packaging beyond the general desire for things to work and be low-maintenance.
There was a problem hiding this comment.
Personally I'd prefer to avoid pinning dependencies to specific version unless strictly as rollback resulting from a bug in latest version of given dependency. At least as far as AppImage goes.
For OS packages - they just use what's on the OS, so pinning isn't really a useful concept in first place.
There was a problem hiding this comment.
Hi @daniel-123 @albertosottile
I just wanted to remind you of this!
There was a problem hiding this comment.
@batmanfeynman Sorry if I neglected this. The gist of this issue is that in both Windows and macOS workflows we currently have:
pip3 install -r requirements.txt
pip3 install -r requirements_gui.txt
and I do not see a clear way to reproduce the same behavior after this PR is merged UNLESS the information about dependencies is duplicated in setup.cfg and in the two requirements file. Am I missing something here? Is there a command to just install the dependencies of a package?
|
Link to copr repo as promised: I also want to suggest 3 modification to the syncplay.pl website:
Edit: PS: I have no issue with it being mentioned that my rpm is unofficial :). |
|
@batmanfeynman can you please add support for fedora 36 in your repo? |
|
@soredake done! |
|
@batmanfeynman Would it possible to get fedora 37 added to your repo? Thank you for working on this! |
|
@corbmr done. |
|
General comment: the issue of having the GUI dependencies separate is not solved. I believe the vast majority of people that install from sources do that precisely because they do not want to have the GUI. Therefore, I do not think we can just merge this PR in a way that does not cover for this use case. Unless I am missing something, I think we need to keep the two separate requirements file, for the moment. |
Fixed.
I've added it to https://copr.fedorainfracloud.org/coprs/batmanfeynman/syncplay/ and #574
Done. |
|
@Et0h @batmanfeynman Any idea if you could update the COPR Fedora 38 (and possibly build for Rawhide) and update the version to the latest release? |
I have made a pyproject.toml file as well as a setup.cfg file which is compliant with the latest python packaging recommendations.
These changes help me in creating an rpm for fedora as well. I will make an rpm package for v1.6.9 by including a patch file, but i think this modernized python packaging will help in possible future installation from pip.
Edit: I have not checked whether this interferes with the appimage creation script. But this does work well with
python -m build.