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

torrent clients support (transmission-daemon) #2617

Closed
anarcat opened this issue Jun 28, 2017 · 15 comments
Closed

torrent clients support (transmission-daemon) #2617

anarcat opened this issue Jun 28, 2017 · 15 comments
Assignees
Labels
needinfo We need more details or follow-up from the filer before this can be tagged "bug" or "feature." stale

Comments

@anarcat
Copy link
Contributor

anarcat commented Jun 28, 2017

Problem

Some of the files managed by beets may be seeded on torrent files to share with your friends.

If beets modifies those files, this will break the checksums from the torrent files, and some torrent clients will obviously be confused by this. Some will just warn the user, others won't do anything. In general, one can assume clients will start re-downloading the affected chunks, overwriting the changes performed by beets.

Setup

N/A

Proposed solution

I envision a plugin that would either deny writes on torrent-managed files or remove files from the torrent client when beets modifies them. This behavior could obviously be customizable from the configuration file.

The same question as with #2616 exists here: I assume I would need to hook into before_item_moved and write to do this. But i'm not clear how or if i can deny moving items at all?

With transmission, my current client of choice, there's a commandline tool (transmission-remote) that can be used to manipulate torrent files (add/remove/list etc). This is backed by an RPC interface that's basically JSON over HTTP which should be fairly easy to use.

@sampsyo sampsyo added the needinfo We need more details or follow-up from the filer before this can be tagged "bug" or "feature." label Jun 29, 2017
@sampsyo
Copy link
Member

sampsyo commented Jun 29, 2017

Interesting idea! Currently, I think most people just disable write, move, and copy to leave all the files in place. Would that suffice for your case?

@anarcat
Copy link
Contributor Author

anarcat commented Jun 29, 2017 via email

@sampsyo
Copy link
Member

sampsyo commented Jun 29, 2017

Interesting. Maybe one strategy, then, would be to leave the write option off and explicitly use beet write after the fact to update non-torrenting files?

If you're interested in putting together a plugin, yes, the write event does seem like the right one to use. You should be able to (conditionally) undo any updates—taking a look at the zero plugin might be a good place to start.

@anarcat
Copy link
Contributor Author

anarcat commented Jun 30, 2017

does write cover the copy, move, etc updates as well? i was wondering if i could interrupt move operations with before_item_moved as well...

@sampsyo
Copy link
Member

sampsyo commented Jul 1, 2017

No, write is just about updating tags. You might indeed need to hook into other events if you want to prevent moving files too.

@anarcat
Copy link
Contributor Author

anarcat commented Jul 1, 2017

how do we prevent moving? is it the same mechanism as write?

@sampsyo
Copy link
Member

sampsyo commented Jul 1, 2017

I don't have any specific ideas about preventing moving—that's not something anyone's wanted to do before. But you can try hooking into the various moving-related plugin events, or dive into the code for the importer or the Item.move method to see what additional events might be needed.

@jhermann
Copy link
Contributor

jhermann commented Mar 10, 2018

This could be solved by my proposal in #2835, by adding a command that checks "loaded into client" and then skips items using before_item_moved and similar events. This would probably also need new before_* events (e.g. before_write, before_album_import).

@stale
Copy link

stale bot commented Jul 12, 2020

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@anarcat
Copy link
Contributor Author

anarcat commented Jan 13, 2022

i wonder if i can just keep this issue open to keep tracking this problem, self-assigning as i may eventually deal with this, but feel free to take it on if you have time!

@anarcat anarcat reopened this Jan 13, 2022
@anarcat anarcat self-assigned this Jan 13, 2022
@anarcat
Copy link
Contributor Author

anarcat commented Jan 13, 2022

This could be solved by my proposal in #2835, by adding a command that checks "loaded into client" and then skips items using before_item_moved and similar events. This would probably also need new before_* events (e.g. before_write, before_album_import).

and it does seem like such a "skip" hook would be the solution here, yeah.

@anarcat
Copy link
Contributor Author

anarcat commented Jan 13, 2022

looking again at the code, it looks like in library.py, i would need to add:

  • before_write hook to Item.write() the write hook already happens before the write(), so that's done
  • ~~before_item_removed hook to Item.remove()``` the item_removed` hook is already called before the removal
  • before_album_removed hook to Album.remove() (although maybe that would be redundant with before_item_removed because Album.remove calls Item.remove
  • move_art hook to Album.move_art
  • before_art_set hook to Album.set_art, or move the art_set hook before the file operation, for consistency with the other hooks?

there is already:

  • before_item_moved

i think that would cover all cases where existing files would get modified or removed. copied or hardlinked files we care less about here, because they do not matter for existing torrents, as clients typically ignore extra files.

we also need to somehow pass information from the plugin to the caller. right now the plugins.send callers mostly ignore the return value, at least in the above hooks (there are rare exceptions elsewhere in the code). i wonder if we should return a truth value or raise an exception to signal a problem.

maybe the safest would be to raise an exception from the plugin. older versions of beets would just fail to do the operation and crash (a possible acceptable failure mode, as in "fail closed"), newer versions could handle the exception and behave correctly (e.g. skip or abort, depending on the exception)?

right now it seems like any HumanReadableException raised in the program will cause basically an abort, but maybe we could have a similar exception that would cause a skip in file operations. in fact, the try_write wrapper handles FileOperationError exceptions neatly, to skip errors on write. then we could do the same with a try_move, try_remove, but then would also need to fix all callers...

makes sense?

@stale stale bot removed the stale label Jan 13, 2022
anarcat added a commit that referenced this issue Jan 13, 2022
This reflects the `try_write` approach will handles write failures
elegantly, by logging the error and continuing. We do the same with
`move`, `art_set` and `move_art`.

We also handle exceptions on `before_item_moved` and `art_set` plugins
hooks, the latter of which is moved *before* the file operations to
remain consistent with other hook configurations.

That might be a mistake and API-breaking change, another approach
would be to have a new `before_art_set` hook instead.

We also introduce a new hook (`move_art`) for those operations as
well.

The point of this patch is to make it possible for plugins to send a
signal (through the already FileOperationError exception) to callers
that it should skip a specific item or artwork.

This is essential to allow beets to better integrate with other
utilities like bittorrent clients which may rewrite those files. The
rationale here is that some music collections will have *parts* of
them managed by such clients in which case we should be careful not to
overwrite or move those files.

Operations like copy or hardlink are not handled by this, for that
reason. We may also want to do proper error handling for those as
well, that said, but that seems out of scope for this specific
issue (#2617).
@anarcat
Copy link
Contributor Author

anarcat commented Jan 13, 2022

a draft of this now lives in #4230.

@stale
Copy link

stale bot commented Mar 16, 2022

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Mar 16, 2022
@stale stale bot closed this as completed Mar 26, 2022
@anarcat
Copy link
Contributor Author

anarcat commented Nov 28, 2023

following up on my old self, i wrote a small script that lists all files under transmission:

https://gitlab.com/anarcat/scripts/-/blob/main/transmission-list-all-files?ref_type=heads

i think my next step is to beet import all files that are not in that list to avoid rewriting tags on files that are controlled by transmission... an alternative is to set aside all those files under transmission and keep them "forked" indefinitely.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needinfo We need more details or follow-up from the filer before this can be tagged "bug" or "feature." stale
Projects
None yet
Development

No branches or pull requests

3 participants