Watcher3 icon indicating copy to clipboard operation
Watcher3 copied to clipboard

Make Watcher3 pip installable and unvendor libs.

Open labrys opened this issue 2 years ago • 13 comments

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 avatar Jan 07 '23 21:01 labrys

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

labrys avatar Jan 07 '23 22:01 labrys

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.

labrys avatar Jan 07 '23 22:01 labrys

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 avatar Jan 09 '23 15:01 scambra

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

scambra avatar Jan 09 '23 15:01 scambra

@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 avatar Jan 09 '23 21:01 labrys

@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.

labrys avatar Jan 09 '23 21:01 labrys

@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.

scambra avatar Jan 09 '23 22:01 scambra

@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.

barbequesauce avatar Feb 05 '23 21:02 barbequesauce

@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.

labrys avatar Feb 07 '23 01:02 labrys

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...

barbequesauce avatar Feb 07 '23 03:02 barbequesauce

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

scambra avatar Feb 07 '23 09:02 scambra

@scambra take a look at #211 - thoughts?

barbequesauce avatar May 31 '23 14:05 barbequesauce

@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

scambra avatar Jun 02 '23 11:06 scambra