-
Notifications
You must be signed in to change notification settings - Fork 22
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
SymlinkView: Fix symlink removal (fixes #47) #48
SymlinkView: Fix symlink removal (fixes #47) #48
Conversation
0ed4a84
to
77e0ec3
Compare
I just tried it out. I imported new music, made the symlinks with
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 EDIT: Actually, the conflict is probably not with the old link, but with some other file. How did you populate |
Well, |
Then what was the effect of
and
? Sorry, I'm not quite following which commands you ran to lead to this error. |
Yes, more exactly the
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. |
The effect of the commands: In my config, when I import, I don't move right away. If you want I can give you my config. |
Ok, so you didn't start with a fresh library/newly imported music in each testrun, but only imported the new music from
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.
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.
Yes and no. It does delete obsolete softlinks that it actually knows about (otherwise the testsuite would fail).
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 :-). |
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). |
No worries :)
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. |
There was no crash and no prompt. However, after checking for broken symlinks (with |
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.
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).
I don't think any further testing regarding the current issue is really necessary, but thanks :) |
Hi! I processed my whole library (I just ripped 200 CDs in confinement) and got this error:
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? |
Sorry, my mistake, I pushed an incomplete changeset. Fixed now, thanks for hanging on! |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking this on!
beetsplug/alternatives.py
Outdated
if p1 == p2: | ||
return True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are the circumstances under which p1 != p2
but they have the same lstat
? Isn’t it enough to fully resolve p1
and p2
and assert that the are equal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure, need to reconsider. I just copied over _samefile
from beets.util
and swapped stat
for lstat
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. I think this logic is present in beets.util.samefile
because we want to check that the target of the link is the same. In our case it should be enough to check that the resolved (i.e. absolute) paths are the same. No filesystem access required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, so I should be able to replace the _samelink(path, dest)
invocation with just path == dest
, right? If I'm not missing anything, all these paths ultimately come from Item.destination
, which returns absolute paths normalized through os.path.normpath
.
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. |
Hi! One little thing:
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? |
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. |
04ad8dd
to
ffa7af8
Compare
I finally adressed your feedback @geigerzaehler. If you don't see an issue with #48 (comment), this should be ready. Sorry for the delay, @dosoe! |
What happened to Travis? It apparently ran a (succesful) build for the latest force-push, but github fails to report that... |
See #54 |
Thanks for your work on addressing this! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✨ Thanks for the contribution @wisp3rwind and @dosoe and sorry for keeping you waiting.
This needs to wait for #56 to make the tests pass and then this should be good to merge and release.
it might be interesting to actually symlink the artwork here, but running the embedding on the symlinked tracks really shouldn't be done
The `soft` kwarg will make util.remove silently skip the deletion of symlinks (os.remove doesn't appear to follow symlinks, but os.path.exists does)
previously, symlinks were followed by os.path.isfile
and targets First, this also a file-system only operation, i.e. cheap, and secondly, the mtime code follows symlinks, so it is not really quite obscure whether and why it does the right thing for symlinks
8e355a7
to
84941a7
Compare
The first commit implements a test for the scenario from #47. The others fix it:
util.remove(path, soft=True)
is always a no-op for symlinks...External
/ExternalConvert
collections andSymlinkView
s since they have somewhat different semantics.Todo: add unit tests for
_remove
and_samelink
, update changelog.Closes #47