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

Move GUI dependencies to setup.py extra #416

Closed
wants to merge 2 commits into from

Conversation

FallenWarrior2k
Copy link

I wanted to install Syncplay with pip to make use of the syncplay-server entry point. However, when I did, I noticed that setup.py pulls in PySide2 as install_requires unconditionally, making that avenue of install undesirable when I only want to run the server in that specific environment. Therefore, I have moved PySide2 to a new gui extra in setup.py, so Syncplay can be install by pip without pulling it in.

I have updated the AppImage build script to account for this change, but I haven't been able to find if the installation as a package is used anywhere else. While I did verify that the AppImage still works with the updated build script, I was unable to test the other platforms as I don't have machines for those available right now.

This allows installing Syncplay to use the syncplay-server entry point
without pulling in PySide2 where not needed.
@daniel-123
Copy link
Contributor

Pretty important preface: we don't really support pip as installation method. So there wasn't that much thought put into user experience when installing Syncplay through pip. Requirements files we have were made to facilitate pulling in dependencies with pip to aid in our CI processes and nothing more really.

The default assumption we take is that we should make install process as friction-less as possible for GUI client and that server/cli installation can be a bit more involved. If we end up on pypi in official capacity we would still prefer to see our default installation to be a client with GUI (unless that explicitly violates established good practices on pypi).

When it comes to no-gui install there are other options we do support - like our make install script or server package for Debian. Or just running Syncplay directly from downloaded repository tarball. We don't really have any plans to add pip as supported installation method, though I'm not strictly opposed to having it work if it doesn't break anything else.

Is it possible to have the requirements so that default would be to install everything needed for GUI and an option that allows for omitting those?

@FallenWarrior2k
Copy link
Author

True. I can definitely understand wanting to make the GUI installation as frictionless as possible. It's just that, for me, it felt easier and cleaner to do pip install git+https://github.com/syncplay/syncplay.git@v1.6.7#egg=syncplay, instead of having to download an archive, unpack it somewhere (meaning it's not tracked by any package manager; I'm not using Debian/Ubuntu atm, so the deb packages weren't an option), and then having to keep that directory around.

I have to admit that I'm not too intimately familiar with Python packaging. With Cargo (first example that came to mind), this could've easily been done with a default feature, but it seems extras in Python do not support such default semantics (cf. pypa/setuptools#1139).

The method described here could have worked, e.g. by specifying an empty extra server and adding a condition extra != 'server' to the pyside2 dependency. However, as this comment states, that method/workaround no longer works.

With things as they are, I am not aware of a method to enable this, other than splitting the GUI into a distinct package. Then, in theory, if you want to continue going down the frictionless route, the GUI could be syncplay, which would depend on e.g. syncplay-common/-server/-cli/whatever and the GUI dependencies. That would obviously be a quite heavy-handed approach, so I can understand if you're opposed.

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.

3 participants