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

Edit folder structure and add setuptools support #231

Merged
merged 12 commits into from Apr 13, 2019

Conversation

@albertosottile
Copy link
Member

@albertosottile albertosottile commented Mar 26, 2019

As discussed in issue #207, our current folder structure is different from the typical one for a Python package. Specifically, the resources folder should be included in the syncplay folder, as all the relevant python script.

This PR includes that file restructuring and also adds a setup.py script that relies on it. Entry points are used for both client and server. Requirements are taken directly from the corresponding text files (thanks to PR #230).

Additionally, the packages needed for TLS are now included in the main requirements, as we do not really want the TLS support to be optional. Also, AppVeyor has been updated to the latest py2exe (link) to enable the embedding of TLS libraries. Incidentally, Windows binaries are also upgraded to Python 3.6.

Copy link
Contributor

@ObserverOfTime ObserverOfTime left a comment

It might be better to place requirements_gui.txt in extras_require since some users may not want the GUI (and PySide2 is several MBs large). You'll just need to inform end users that they'll need syncplay[gui] for the full package.

return f.read()

installRequirements = read('requirements.txt').splitlines() +\
read('requirements_gui.txt').splitlines()
Copy link
Contributor

@ObserverOfTime ObserverOfTime Mar 26, 2019

Suggested change
read('requirements_gui.txt').splitlines()

with open(fname, 'r') as f:
return f.read()

installRequirements = read('requirements.txt').splitlines() +\
Copy link
Contributor

@ObserverOfTime ObserverOfTime Mar 26, 2019

Suggested change
installRequirements = read('requirements.txt').splitlines() +\

url=projectURL,
download_url=projectURL + 'download/',
packages=setuptools.find_packages(),
install_requires=installRequirements,
Copy link
Contributor

@ObserverOfTime ObserverOfTime Mar 26, 2019

Suggested change
install_requires=installRequirements,
install_requires=read('requirements.txt').splitlines(),
extras_require={'gui': read('requirements_gui.txt').splitlines()},

@Et0h
Copy link
Contributor

@Et0h Et0h commented Apr 10, 2019

@ObserverOfTime

It might be better to place requirements_gui.txt in extras_require since some users may not want the GUI (and PySide2 is several MBs large). You'll just need to inform end users that they'll need syncplay[gui] for the full package.

Aside from saving a few megabytes of storage space and speeding up the installation time, are there any circumstances where automatically installing PySide2 would be disruptive, prohibitive or impossible?

In terms of the Syncplay client, the GUI is a standard part of the software rather than an optional extra. The CLI/console is a backup mode rather than a use case that we recommend (not least because various newer features are only available from the GUI).

@ObserverOfTime
Copy link
Contributor

@ObserverOfTime ObserverOfTime commented Apr 10, 2019

I think #229 qualifies as a use case where the GUI is not needed.

Moreover, if someone just wants to set up the server on a VPS, the storage space saved by not installing PySide2 can be significant.

@daniel-123
Copy link
Contributor

@daniel-123 daniel-123 commented Apr 10, 2019

Personally I think that the vast majority of users do use the GUI and would expect the "software_name" package to deliver that functionality. I would prefer not to even slightly inconvenience them by making them install package with extras. Even if there exist some use cases where such packaging would be more convenient.

What is possibly worth considering is that most cases where one would care about not including superfluous dependencies are headless server. So maybe that should be a completely separate, minimalist package?

I'm not entirely familiar with setuptools ecosystem, but at least with deb/rpm packaging such problem would be usually solved by existence of package syncplay-no-gui that's either required by full syncplay or is mutually exclusive with it.

@ObserverOfTime
Copy link
Contributor

@ObserverOfTime ObserverOfTime commented Apr 11, 2019

I don't think what you're proposing is feasible with setuptools.
Additionally, it's not possible to declare default or opt-out requirements.

How about this solution:

diff --git a/setup.py b/setup.py
index 91b2602..a083b15 100644
--- a/setup.py
+++ b/setup.py
@@ -3,14 +3,16 @@
 import distutils.command.install_scripts
 import setuptools
 
+from os import environ as env
 from syncplay import projectURL, version as syncplay_version
 
 def read(fname):
     with open(fname, 'r') as f:
         return f.read()
 
-installRequirements = read('requirements.txt').splitlines() +\
-                      read('requirements_gui.txt').splitlines()
+installRequirements = read('requirements.txt').splitlines()
+if env.get('SYNCPLAY_NO_GUI', '0') != '1':
+    installRequirements += read('requirements_gui.txt').splitlines()
 
 setuptools.setup(
     name="syncplay",

This way, regular users will not be affected and anyone who doesn't want the GUI can just set SYNCPLAY_NO_GUI=1 prior to the installation and update.

I'll update my proposed changes if everyone finds this satisfactory.

@albertosottile
Copy link
Member Author

@albertosottile albertosottile commented Apr 11, 2019

Will that work for wheels too? I wonder if the sake of not having PySide2 installed on a system is worth amending in this way the setup.py script, introducing a new environment variable, documenting it somewhere, maintaining the whole thing...

@Et0h
Copy link
Contributor

@Et0h Et0h commented Apr 13, 2019

@ObserverOfTime Thanks for your suggestion regarding how we could use environmental variables as a way of supporting a 'no GUI required' installation option without making it the default - we will look into it further if we ever decide to pursue PyPi support.

@Et0h Et0h merged commit 281d802 into Syncplay:master Apr 13, 2019
2 checks passed
@Et0h Et0h mentioned this pull request Apr 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants