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

Add setup.py for setuptools #230

Closed
wants to merge 1 commit into from
Closed

Add setup.py for setuptools #230

wants to merge 1 commit into from

Conversation

ObserverOfTime
Copy link
Contributor

This is related to issue #207.

I tried to keep changes to the codebase minimal but I had to edit syncplay.utils.findWorkingDir to find the path when installed via setuptools since resources/ is copied into syncplay/.
(You could probably use pkg_resources to load resources and avoid all the hassle.)

Once Syncplay is deployed to PyPI (which can be automated by Travis), installing it along with its optional dependencies will be as simple as pip3 install syncplay[gui,tls].

Environment info:

  • OS: Antergos Linux 5.0.3-arch1-1-ARCH
  • Python: Python 3.7.2
  • pip: pip 18.1
  • setuptools: setuptools 40.8.0

Amended syncplay.utils.findWorkingDir for the setuptools path
@albertosottile
Copy link
Member

Thank you for this PR, we really appreciate it. We are currently working on implementing setuptools for Syncplay here, but I see some differences between this PR and our approach that are definitely worthy of a discussion.

Firstly, you used requirements*.txt files in the setup.py script. That's something we could do, too.

Secondly, you found a way to install the scripts without restructuring the package, by subclassing the InstallScripts class. While we considered that approach, we think that using entry points is the most natural and future-proof choice, given that they are recommended. So, we restructured some of the code to use entry points, instead of install scripts, as you can see here.

Thirdly, you subclassed BuildPy instead of actually moving the resources folder inside the syncplay one. While I appreciate that this script does not require to relocate the folder, I am afraid for the future support of this solution and of the possible bugs that it could cause. As an example, this setup.py script does not currently copy the resources folder when running it with sdist. Of course, this and other future bugs can be fixed, but maybe it would just be better to move the resources folder now and migrate Syncplay to the recommended file structure for a python package. This is the approach that I took in the fork so far.

In summary, I think we will reject this PR in favor of opening a new one, based on the work done so far in the fork linked above. I sincerely apologize for the time you already invested in working on this. In any case, I strongly encourage to review and contribute to the upcoming PR, since you seem very experienced with setuptools.

@ObserverOfTime
Copy link
Contributor Author

ObserverOfTime commented Mar 26, 2019

Thank you for the response. That's all right, I just did it in an evening as a proof of concept mostly. I wasn't aware that you were already working on it in a fork. I'll be glad to offer my help, though I'm not really very experienced.

albertosottile added a commit to albertosottile/syncplay that referenced this pull request Mar 26, 2019
albertosottile added a commit to albertosottile/syncplay that referenced this pull request Mar 26, 2019
albertosottile added a commit to albertosottile/syncplay that referenced this pull request Mar 26, 2019
albertosottile added a commit to albertosottile/syncplay that referenced this pull request Mar 26, 2019
Et0h pushed a commit that referenced this pull request Apr 13, 2019
* setuptools: Initial commit

* setuptools: remove the .py extension from installed commands

* setuptools: restructure scripts to use entry_points in setup.py

* setuptools: include TLS dependencies and remove unneeded code

* setuptools: change resources path

* AppVeyor: upgrade Python and py2exe, embed TLS dependencies

* buildpy2exe: fix path for resources

* AppVeyor: upgrade py2exe and PySide2

* Amend setup.py according to the suggestions from PR #230

* Insert TLS dependencies in requirements

* AppVeyor: fix build for master

* AppVeyor: revert to PySide2 5.12.0
@Et0h
Copy link
Contributor

Et0h commented Apr 13, 2019

Closed as setuptools is now supported via #231

@Et0h Et0h closed this Apr 13, 2019
albertosottile added a commit to albertosottile/syncplay that referenced this pull request May 1, 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
Development

Successfully merging this pull request may close these issues.

3 participants