search

geigerzaehler / beets-alternatives

Beets plugin to manage external files
MIT License
91 stars 21 forks source link

SymlinkView: Fix symlink removal (fixes #47) #48

Closed wisp3rwind closed 4 years ago

wisp3rwind commented 4 years ago

The first commit implements a test for the scenario from #47. The others fix it:

Todo: add unit tests for _remove and _samelink, update changelog.

Closes #47

dosoe commented 4 years ago

I just tried it out. I imported new music, made the symlinks with beet alt update by-work then moved them and tried to recreate the symlinks with the same command. so to summarize:

  1. beet import Personnel/music_temp
  2. beet alt update by-work
  3. beet move Personnel/music_temp
  4. beet alt update by-work Output: 
    
(base) soergeld@ist-156-28:~$ beet -vv alt update by-work
user configuration: /home/soergeld/.config/beets/config.yaml
data directory: /home/soergeld/.config/beets
plugin paths: /home/soergeld/beets-alternatives/beetsplug
Sending event: pluginload
inline: adding item field pw
inline: adding item field aa
inline: adding item field aaa
inline: adding item field pc
inline: adding item field tr
inline: adding item field work_prefix
inline: adding item field type
inline: adding item field dupe
library database: /media/soergeld/My Passport/Musicdata_sync.db
library directory: /media/soergeld/My Passport
Sending event: library_opened
+/media/soergeld/My Passport/by-work/Classique/[anonymous]/Divers/Ensemble Organum, Peres, Marcel/Chants de la Cathedrale de Benevento/1-1 Antienne_ Otin to stauron.flac
Traceback (most recent call last):
File "/usr/local/lib/python3.6/dist-packages/beets/util/__init__.py", line 510, in link
os.symlink(syspath(path), syspath(dest))
FileExistsError: [Errno 17] File exists: b'/media/soergeld/My Passport/by-album/Ensemble Organum, Peres, Marcel/Chants de la Cathedrale de Benevento/01-01 Antienne_ Otin to stauron.flac' -> b'/media/soergeld/My Passport/by-work/Classique/[anonymous]/Divers/Ensemble Organum, Peres, Marcel/Chants de la Cathedrale de Benevento/1-1 Antienne_ Otin to stauron.flac'

Error: File exists during link of paths /media/soergeld/My Passport/by-album/Ensemble Organum, Peres, Marcel/Chants de la Cathedrale de Benevento/01-01 Antienne Otin to stauron.flac, /media/soergeld/My Passport/by-work/Classique/[anonymous]/Divers/Ensemble Organum, Peres, Marcel/Chants de la Cathedrale de Benevento/1-1 Antienne Otin to stauron.flac


It still tries to create a new link and fails because the old link is still there. Maybe check beforehand that the present links are fine before creating new ones?
wisp3rwind commented 4 years ago

It still tries to create a new link and fails because the old link is still there. Maybe check beforehand that the present links are fine before creating new ones?

That's what it should do, however, for some reason, it fails to properly detect this situation. I'm stuck figuring out what might be going on here and can't seem to reproduce this. Could you try this again (I've added a commit to this PR to print some more information) and attach verbose logs of all the commands you ran, in particular the beet move? Thanks!

EDIT: Actually, the conflict is probably not with the old link, but with some other file. How did you populate Personnel/music_temp and /media/soergeld/My Passport before this test?

dosoe commented 4 years ago

Well, Personnel/music_temp was not populated, but /media/soergeld/My Passport is my music library, so it is populated. However, the file that causes the problem (/media/soergeld/My Passport/by-work/Classique/[anonymous]/Divers/Ensemble Organum, Peres, Marcel/Chants de la Cathedrale de Benevento/1-1 Antienne_ Otin to stauron.flac) was created by the first beet alt update by-work and stopped the second one.

wisp3rwind commented 4 years ago

Then what was the effect of

1. `beet import Personnel/music_temp`

and

  1. beet move Personnel/music_temp ? Sorry, I'm not quite following which commands you ran to lead to this error.
dosoe commented 4 years ago

Yes, more exactly the beet move. I use beet move Personnel/music_temp because that's where the music I imported is, I don't want him to update the paths of the whole collection, that takes too much time (50 000 files), just move the new ones in place. I import music with beets, then I create softlinks (with beet alt update by-work). Then I move the music, and I want to update the softlinks (again with beet alt update by-work), but the old softlinks are still in place so the new ones can't be created. I cloned and installed again, as you suggested. Here are the logs: 

(base) soergeld@ist-156-28:~$ beet -vv alt update by-work
user configuration: /home/soergeld/.config/beets/config.yaml
data directory: /home/soergeld/.config/beets
plugin paths: /home/soergeld/beets-alternatives/beetsplug
Sending event: pluginload
inline: adding item field pw
inline: adding item field aa
inline: adding item field aaa
inline: adding item field pc
inline: adding item field tr
inline: adding item field work_prefix
inline: adding item field type
inline: adding item field dupe
library database: /media/soergeld/My Passport/Musicdata_sync.db
library directory: /media/soergeld/My Passport
Sending event: library_opened
+/media/soergeld/My Passport/by-work/Classique/[anonymous]/Divers/Ensemble Organum, Peres, Marcel/Chants de la Cathedrale de Benevento/1-1 Antienne_ Otin to stauron.flac
Traceback (most recent call last):
  File "/usr/local/lib/python3.6/dist-packages/beets/util/__init__.py", line 510, in link
    os.symlink(syspath(path), syspath(dest))
FileExistsError: [Errno 17] File exists: b'/media/soergeld/My Passport/by-album/Ensemble Organum, Peres, Marcel/Chants de la Cathedrale de Benevento/01-01 Antienne_ Otin to stauron.flac' -> b'/media/soergeld/My Passport/by-work/Classique/[anonymous]/Divers/Ensemble Organum, Peres, Marcel/Chants de la Cathedrale de Benevento/1-1 Antienne_ Otin to stauron.flac'

Error: File exists during link of paths /media/soergeld/My Passport/by-album/Ensemble Organum, Peres, Marcel/Chants de la Cathedrale de Benevento/01-01 Antienne_ Otin to stauron.flac, /media/soergeld/My Passport/by-work/Classique/[anonymous]/Divers/Ensemble Organum, Peres, Marcel/Chants de la Cathedrale de Benevento/1-1 Antienne_ Otin to stauron.flac

I checked, this recording is not a dupe (He managed to create the softlinks before the move after all). It really feels like he isn't deleting the obsolete softlink and therefore gets into conflict. As for the initial problem (the one for which I submitted a bug), it is the opposite: the file didn't move, but the metadata that is used to create the softlink changed, so the softlink changed. It created the new softlink, but didn't delete the old one. So there seems to be a problem with deleting obsolete softlinks.

dosoe commented 4 years ago

The effect of the commands: I ripped some CDs, stored in Personnel/music_temp I import them for them with beet import Personnel/music_temp to be in my beets library, situated in /media/soergeld/My Passport/by-album. Then I create softlinks (in /media/soergeld/My Passport/by-work), then I move the files (with beet move Personnel/music_temp because I only want to move the files I just imported) according to my beets config file (path variable), then I try to create the softlinks again (in the hope that it will update the softlinks).

In my config, when I import, I don't move right away. If you want I can give you my config.

wisp3rwind commented 4 years ago

Yes, more exactly the beet move. I use beet move Personnel/music_temp because that's where the music I imported is, I don't want him to update the paths of the whole collection, that takes too much time (50 000 files), just move the new ones in place.

Ok, so you didn't start with a fresh library/newly imported music in each testrun, but only imported the new music from music_temp once?

I import music with beets, then I create softlinks (with beet alt update by-work). Then I move the music, and I want to update the softlinks (again with beet alt update by-work), but the old softlinks are still in place so the new ones can't be created. I cloned and installed again, as you suggested. Here are the logs: [...]

Are you sure that this was really with the latest commits from this PR? There's none of the additional output I would've expected. In any case, I pushed another change to the debug print's, which should give a clear indication in the log that the latest commit is being run.

I checked, this recording is not a dupe (He managed to create the softlinks before the move after all). It really feels like he isn't deleting the obsolete softlink and therefore gets into conflict. As for the initial problem (the one for which I submitted a bug), it is the opposite: the file didn't move, but the metadata that is used to create the softlink changed, so the softlink changed. It created the new softlink, but didn't delete the old one.

Yes, and that's what this PR originally attempted to fix. In the process, I made a few changes to how exactly it detects out-of-date symlinks. However, I don't see how these changes can lead to the crash you're observing now.

So there seems to be a problem with deleting obsolete softlinks.

Yes and no. It does delete obsolete softlinks that it actually knows about (otherwise the testsuite would fail). beets-alternatives does not appear to think the problematic link was created by itself, hence does not attempt to replace it. Here's my latest guess at what is going on: The crashes in earlier runs of beet alt update resulted in a state where the symlinks were already created in by-work, but it crashed before it could track them in the database. Hence, the plugin doesn't realize it is safe (and in fact the intended action) to replace them. If you pull the latest commit from this PR and try to beet alt update by-work again, it should prompt you whether to overwrite such links, which would fixup this inconsistent state of the database (Please do test this!)

In my config, when I import, I don't move right away. If you want I can give you my config.

I figured as much from the logs (and you did post your config in the other beets issue, so no need to repeat it right now :-).

dosoe commented 4 years ago

Are you sure that this was really with the latest commits from this PR?

Ok, I double-checked, and I was in the wrong branch (sorry, I'm not fluent in git). It now works, but gives great amounts of output, so I can't quite follow what's happening. Once it's over (in an hour maybe) I will check if there are some broken links or an error. He doesn't ask me anything though, I don't get any prompt (an he already passed the link that was problematic before).

wisp3rwind commented 4 years ago

Ok, I double-checked, and I was in the wrong branch (sorry, I'm not fluent in git).

No worries :)

It now works, but gives great amounts of output, so I can't quite follow what's happening. Once it's over (in an hour maybe) I will check if there are some broken links or an error. He doesn't ask me anything though, I don't get any prompt (an he already passed the link that was problematic before).

Oh, sorry, I didn't have in mind the extent of your library when adding the additional output. It's going to print a few lines for each track in in your library... which indeed renders it rather useless. But unless there's another crash, I guess it doesn't matter.

dosoe commented 4 years ago

There was no crash and no prompt. However, after checking for broken symlinks (with find /media/soergeld/My\ Passport/by-work/ -xtype l -print ) I do find some, more specifically these links correspond to an album that was already in the library that I re-ripped and reimported. The two versions were in different file formats (flac and mp3) so there was no conflict when creating the links, but then beet dup -d removed the mp3 recordings. beet alt update by-work saw that the recordings were still in the collection and kept the links, even if I now have each recording with a link in flac and a (broken) link in mp3. I hope I'm clear, else please tell me. Is there a way to do beet alt up by-work on only part of the library (only the files where something has changed)? Then the verbose output would be usable and useful. Else I can try to create a new library for testing purposes with fewer recordings.

wisp3rwind commented 4 years ago

There was no crash and no prompt. However, after checking for broken symlinks (with find /media/soergeld/My\ Passport/by-work/ -xtype l -print ) I do find some, more specifically these links correspond to an album that was already in the library that I re-ripped and reimported. The two versions were in different file formats (flac and mp3) so there was no conflict when creating the links, but then beet dup -d removed the mp3 recordings. beet alt update by-work saw that the recordings were still in the collection and kept the links, even if I now have each recording with a link in flac and a (broken) link in mp3. I hope I'm clear, else please tell me.

This is probably unrelated to #47/#48: I'm pretty sure that beets-alternatives always leaks items that are removed from the beets library, removing them is simply not implemented (and never was). I'm gonna open a new issue to track this.

Is there a way to do beet alt up by-work on only part of the library (only the files where something has changed)? Then the verbose output would be usable and useful.

No, there isn't. How long this run (roughly)? If it takes an unreasonable amount of time, we should rather see how to optimize this, it should be possible to check 50000 tracks rather quickly (I suspect that the bottleneck here are beets' flexattrs, as always when beets is slow. However, since we only need to access one column, and optimization should be relatively straighforward).

Else I can try to create a new library for testing purposes with fewer recordings.

I don't think any further testing regarding the current issue is really necessary, but thanks :)

dosoe commented 4 years ago

Hi! I processed my whole library (I just ripped 200 CDs in confinement) and got this error: 

get_path(item): 
b"/media/soergeld/My Passport/by-work/Medieval/Abadia de Montserrat, Coro de monjes de la, Maria Einsiedeln, Choralschola des Klosters, Choralschola der Benediktinerabtei Munsterschwarzach, Choeur des moines de l'Abbaye Notre-Dame de Fontgombault,/Die Tradition des Gregorianischen Chorals/4-106 Ambrosianischer Choral_ Cantus Officiorum_ Psallenda_ Pax in caelo.flac"
lstat(get_path(item)): os.stat_result(st_mode=41471, st_ino=420968, st_dev=2065, st_nlink=1, st_uid=1001, st_gid=1001, st_size=358, st_atime=1583432979, st_mtime=1583432979, st_ctime=1583432979)
get_path(item): 
b"/media/soergeld/My Passport/by-work/Medieval/Abadia de Montserrat, Coro de monjes de la, Maria Einsiedeln, Choralschola des Klosters, Choralschola der Benediktinerabtei Munsterschwarzach, Choeur des moines de l'Abbaye Notre-Dame de Fontgombault,/Die Tradition des Gregorianischen Chorals/4-107 Ambrosianischer Choral_ Cantus Officiorum_ Psallenda_ Videntes stellam magi.flac"
lstat(get_path(item)): os.stat_result(st_mode=41471, st_ino=420976, st_dev=2065, st_nlink=1, st_uid=1001, st_gid=1001, st_size=367, st_atime=1583432979, st_mtime=1583432979, st_ctime=1583432979)
get_path(item): 
None
+/media/soergeld/My Passport/by-work/Classique/[anonymous]/Divers/chant 1450/Du fond de ma pensee/1-31 Du fond de ma pensee.flac
Traceback (most recent call last):
  File "/usr/local/bin/beet", line 11, in <module>
    load_entry_point('beets==1.5.0', 'console_scripts', 'beet')()
  File "/usr/local/lib/python3.6/dist-packages/beets/ui/__init__.py", line 1267, in main
    _raw_main(args)
  File "/usr/local/lib/python3.6/dist-packages/beets/ui/__init__.py", line 1254, in _raw_main
    subcommand.func(lib, suboptions, subargs)
  File "/home/soergeld/beets-alternatives/beetsplug/alternatives.py", line 148, in func
    opts.func(lib, opts)
  File "/home/soergeld/beets-alternatives/beetsplug/alternatives.py", line 76, in update
    alt.update(create=options.create)
  File "/home/soergeld/beets-alternatives/beetsplug/alternatives.py", line 459, in update
    self.create_symlink(item, replace)
TypeError: create_symlink() takes 2 positional arguments but 3 were given

I've never seen this error before and it is is one of the new files, the first one alphabetically (Edit: It is actually a new file). Do you know what the problem could be?

wisp3rwind commented 4 years ago

Sorry, my mistake, I pushed an incomplete changeset. Fixed now, thanks for hanging on!

dosoe commented 4 years ago

Ok, it seems to do fine now. However, I could do with a little less output :D . It's running right now (I guess the output slows it down as well), he offers me some choice with a prompt, I'll let you know if there is any further problem.

wisp3rwind commented 4 years ago

Thanks for the review, @geigerzaehler! I guess I need to split this PR into more sensible chunks. Right now, it is rather convoluted due to how it evolved when I tried to debug @dosoe's issues.

dosoe commented 4 years ago

Hi! One little thing: 

(base) soergeld@ist-156-28:~$ beet absubmit 
** error loading plugin alternatives:
Traceback (most recent call last):
  File "/usr/local/lib/python2.7/dist-packages/beets/plugins.py", line 275, in load_plugins
    namespace = __import__(modname, None, None)
  File "/home/soergeld/beets-alternatives/beetsplug/alternatives.py", line 141
SyntaxError: Non-ASCII character '\xe2' in file /home/soergeld/beets-alternatives/beetsplug/alternatives.py on line 141, but no encoding declared; see http://python.org/dev/peps/pep-0263/ for details

It's probably related to python2-python3 stuff, I never know in which python I install beets when I do it manually.

wisp3rwind commented 4 years ago

Hi! One little thing:

(base) soergeld@ist-156-28:~$ beet absubmit 
** error loading plugin alternatives:
Traceback (most recent call last):
  File "/usr/local/lib/python2.7/dist-packages/beets/plugins.py", line 275, in load_plugins
    namespace = __import__(modname, None, None)
  File "/home/soergeld/beets-alternatives/beetsplug/alternatives.py", line 141
SyntaxError: Non-ASCII character '\xe2' in file /home/soergeld/beets-alternatives/beetsplug/alternatives.py on line 141, but no encoding declared; see http://python.org/dev/peps/pep-0263/ for details

It's probably related to python2-python3 stuff, I never know in which python I install beets when I do it manually.

Yeah, on Python 3 the default encoding is utf-8, and this is also unrelated to the current PR, but rather #46. The real question here is why this didn't fail travis on Python 2 before. I presume the tests load plugins differently than a 'real' run of beets?

geigerzaehler commented 4 years ago

I guess I need to split this PR into more sensible chunks.

I’m totally fine with the scope of this PR and you can keep it like this. I’d just prefer to discuss adding the user interaction on existing links separately.

wisp3rwind commented 4 years ago

I finally adressed your feedback @geigerzaehler. If you don't see an issue with https://github.com/geigerzaehler/beets-alternatives/pull/48#discussion_r421013799, this should be ready. Sorry for the delay, @dosoe!

wisp3rwind commented 4 years ago

What happened to Travis? It apparently ran a (succesful) build for the latest force-push, but github fails to report that...

wisp3rwind commented 4 years ago

Hi! One little thing:

(base) soergeld@ist-156-28:~$ beet absubmit 
** error loading plugin alternatives:
Traceback (most recent call last):
  File "/usr/local/lib/python2.7/dist-packages/beets/plugins.py", line 275, in load_plugins
    namespace = __import__(modname, None, None)
  File "/home/soergeld/beets-alternatives/beetsplug/alternatives.py", line 141
SyntaxError: Non-ASCII character '\xe2' in file /home/soergeld/beets-alternatives/beetsplug/alternatives.py on line 141, but no encoding declared; see http://python.org/dev/peps/pep-0263/ for details

It's probably related to python2-python3 stuff, I never know in which python I install beets when I do it manually.

Yeah, on Python 3 the default encoding is utf-8, and this is also unrelated to the current PR, but rather #46. The real question here is why this didn't fail travis on Python 2 before. I presume the tests load plugins differently than a 'real' run of beets?

See #54

dosoe commented 4 years ago

Thanks for your work on addressing this!