Skip to content
This repository has been archived by the owner on Aug 12, 2023. It is now read-only.

Make Watcher3 pip installable and unvendor libs. #206

Draft
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

labrys
Copy link

@labrys labrys commented Jan 7, 2023

This begins the work on making Watcher3 pip installable. It opts for setuptools instead of Poetry. Steps to install:

git clone https://github.com/barbequesauce/watcher3
cd watcher3
# optionally create a virtual environment and activate it here
pip install m2r  # required because cherrypyscheduler fails to install without it
pip install .  # or pip install -e . for an editable install for development

The above steps are currently functional. I'd like to continue development to make the project fully pip-installable through a command like pip install watcher3@git+https://github.com/barbequesauce/watcher3@master or if you create a pypi package simply pip install watcher3. This would completely remove the requirement to git clone the repo and would give the added benefit of being able to upgrade Watcher3 with pip install --upgrade watcher3.

@labrys labrys marked this pull request as draft January 7, 2023 21:21
@labrys
Copy link
Author

labrys commented Jan 7, 2023

@barbequesauce Let me know how you feel about this PR.

@labrys
Copy link
Author

labrys commented Jan 7, 2023

This PR beings the implementation of a system to meet the goal of #161 using setuptools instead. Once pip-installable you could use pipx install watcher3 to make an isolated build of Watcher3. Then to run it you would pipx run watcher3.

@scambra
Copy link
Collaborator

scambra commented Jan 9, 2023

I think you shouldn't unvendor some libs which were changed in Watcher3, check the changes:
https://github.com/barbequesauce/Watcher3/blob/master/lib/mods.txt

I think PTN got many changes, I would like to replace it with something else as it isn't maintained, so probably it should never be unvendored.

The changes on cheroot and transmissionrpc may not be needed if they are related to some bug which is fixed in a newer version, but it should be checked before unvendoring them

@scambra
Copy link
Collaborator

scambra commented Jan 9, 2023

About PTN, another way to unbundle it would be creating a fork and pushing our changes there.

@labrys
Copy link
Author

labrys commented Jan 9, 2023

@scambra like in #205 I'm not yet addressing the issues in mods.txt, mainly just to provide a functional proof of concept before it makes its way into master. Some of the changes may be obsolete. For example I'll be using my fork of transmission-rpc which uses requests instead of urllib. Also if a fix is simple enough to monkey-patch, I'd rather install the upstream version and monkey-patch locally. If the changes are significant enough then I think forking it with the changes is the best way to go. I'm generally against vendoring libs as they tend to get ignored and never get any TLC or QoL improvements or even security fixes. Also when people vendor libs their changes frequently never make their way upstream to improve the code base for others. If you're contributing upstream then that's more people working on those repositories as well, improving the whole ecosystem. The point is I do plan on addressing them before the code gets merged into master, but probably not before going to develop.

@labrys
Copy link
Author

labrys commented Jan 9, 2023

@scambra oh and I forgot to say, thanks for pointing out the mods.txt, while I had already seen it, its always great to see others looking out for potential issues, as we can't catch everything. I appreciate it.

@scambra
Copy link
Collaborator

scambra commented Jan 9, 2023

@labrys agree with you, I don't like vendoring, I had to upgrade some lib and it's a pain. Although I think vendoring PTN is ok as it's not maintained, and probably is better to replace it with guessit or something else later. Although forking it with our changes is probably better than vendoring.

If the changes on transmission or cheroot are not needed anymore, it's great, otherwise we should send a PR, and fork or monkey patch them until fix is merged into upstream.

@barbequesauce
Copy link
Owner

@scambra @labrys did either of you had further thoughts on PTN? Other than that this patch has been running nicely on my system since PR submission, I'm ok to merge it other than the PTN question... transmission changes dont seem to be needed (tx is my torrent client of choice) and cheroot hasn't shown any issues.

@labrys
Copy link
Author

labrys commented Feb 7, 2023

@barbequesauce In my opinion the easiest path forward is forking PTN. While replacing it in the future would probably advisable, there's no reason to delay this for a significant change like that.

@barbequesauce
Copy link
Owner

I'm thinking the same @labrys - I want to give @scambra a chance to weigh in as I'd say they know the code base better than me...

@scambra
Copy link
Collaborator

scambra commented Feb 7, 2023

I agree, I wanted to fork PTN and push the changes, but I had no time yesterday

@barbequesauce
Copy link
Owner

@scambra take a look at #211 - thoughts?

@scambra
Copy link
Collaborator

scambra commented Jun 2, 2023

@labrys Can you change parse-torrent-name with parse-torrent-title?
Then we should work in a branch to adapt our code to the new fields in PTN

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants