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

Feature/improve find and scan code #874

Merged
merged 19 commits into from Oct 20, 2014

Conversation

3 participants
@adamcik
Member

adamcik commented Oct 15, 2014

Part of fixing #856 and #858. I'm stilling missing the follow config option, but rest should be more or less done, or at least ready for review.

@adamcik

This comment has been minimized.

Member

adamcik commented Oct 15, 2014

Most of this is actually just improving the tests to support adding the follow feature to be honest.

@adamcik

This comment has been minimized.

Member

adamcik commented Oct 15, 2014

Oh, and longer term this should actually just become a single threaded generator, but that is first when #742 gets underway, as the threads are mostly to compensate for slow remote stats and with a "pipeline" this doesn't really matter any more.

elif stat.S_ISREG(st.st_mode):
results[path] = st
else:
errors[path] = 'Not a file or directory'
errors[path] = Exception('Not a file or directory')
except os.error as e:

This comment has been minimized.

@jodal

jodal Oct 16, 2014

Member

s/os.error/OSError/

@jodal

This comment has been minimized.

Member

jodal commented Oct 16, 2014

👍

# Could be replaced with passing in a predicate to the find function?
for name in file_mtimes.keys():
if b'/.' in name:
logger.debug('Skipping hidden file/directory: %r', name)

This comment has been minimized.

@kingosticks

kingosticks Oct 16, 2014

Member

Am I right in saying that path.find_mtimes() doesn't return directories?

This comment has been minimized.

@adamcik

adamcik Oct 16, 2014

Member

True, with the exception of directories with permission problems showing up in the errors. So yes the logging is a bit off. I think the more sane approach is to pass in a function which is given each path and then decides if we should traverse it.

Though ideally I should just make sure the pipeline handles things like .directory files etc nicely and then we wouldn't need to skip anything.

This comment has been minimized.

@kingosticks

kingosticks Oct 16, 2014

Member

The message as it is suggests you could skip scanning a particular
directory by making it hidden. Which you can't.
On 16 Oct 2014 13:07, "Thomas Adamcik" notifications@github.com wrote:

In mopidy/local/commands.py:

@@ -74,9 +74,23 @@ def run(self, args, config):
uris_to_update = set()
uris_to_remove = set()

  •    file_mtimes = path.find_mtimes(media_dir)
    
  •    file_mtimes, file_errors = path.find_mtimes(media_dir)
    
  •    # TODO: Not sure if we want to keep this, but for now lets filter these
    
  •    # Could be replaced with passing in a predicate to the find function?
    
  •    for name in file_mtimes.keys():
    
  •        if b'/.' in name:
    
  •            logger.debug('Skipping hidden file/directory: %r', name)
    

True, with the exception of directories with permission problems showing
up in the errors. So yes the logging is a bit off. I think the more sane
approach is to pass in a function which is given each path and then decides
if we should traverse it.

Though ideally I should just make sure the pipeline handles things like
.directory files etc nicely and then we wouldn't need to skip anything.


Reply to this email directly or view it on GitHub
https://github.com/mopidy/mopidy/pull/874/files#r18951227.

This comment has been minimized.

@adamcik

adamcik Oct 16, 2014

Member

Find will traverse it, but files in hidden directories will be ignored. But think I'll code up the "predicate" based idea for this to bring back the old behavior properly.

This comment has been minimized.

@kingosticks

kingosticks Oct 16, 2014

Member

Oh, you ignore non-hidden files in hidden directories? I didn't see that. If the behaviour matters then some tests for it would also be worthwhile.

This comment has been minimized.

@adamcik

adamcik Oct 16, 2014

Member

The check for /. is a sneaky way to exclude both hidden files and directories + their content. Main reason for it is just because that's what it used to do. And more importantly that I know the scanner isn't fond of a bunch of the hidden files on my own system.

This comment has been minimized.

@kingosticks

kingosticks Oct 16, 2014

Member

Doh! Sorry I now see how that works.
On 16 Oct 2014 17:08, "Thomas Adamcik" notifications@github.com wrote:

In mopidy/local/commands.py:

@@ -74,9 +74,23 @@ def run(self, args, config):
uris_to_update = set()
uris_to_remove = set()

  •    file_mtimes = path.find_mtimes(media_dir)
    
  •    file_mtimes, file_errors = path.find_mtimes(media_dir)
    
  •    # TODO: Not sure if we want to keep this, but for now lets filter these
    
  •    # Could be replaced with passing in a predicate to the find function?
    
  •    for name in file_mtimes.keys():
    
  •        if b'/.' in name:
    
  •            logger.debug('Skipping hidden file/directory: %r', name)
    

The check for /. is a sneaky way to exclude both hidden files and
directories + their content. Main reason for it is just because that's what
it used to do. And more importantly that I know the scanner isn't fond of a
bunch of the hidden files on my own system.


Reply to this email directly or view it on GitHub
https://github.com/mopidy/mopidy/pull/874/files#r18966209.

st = os.lstat(entry)
if (st.st_dev, st.st_ino) in parents:
errors[path] = Exception('Sym/hardlink loop found.')

This comment has been minimized.

@adamcik

adamcik Oct 17, 2014

Member

This and the other plain exceptions still need to be replaced with a FindError or something along those lines. We could perhaps have it contain an errno and message and then use that instead of OSError as well.

This comment has been minimized.

@adamcik

adamcik Oct 20, 2014

Member

@jodal guess you didn't see this open question before merging?

This comment has been minimized.

@jodal

jodal Oct 20, 2014

Member

No question marks!

This comment has been minimized.

@adamcik

adamcik Oct 20, 2014

Member

Oh well :-) So does having such FindError instead of OSError make sense? As using Exception here is horribly stupid and was just meant as a placeholder.

This comment has been minimized.

@jodal

jodal Oct 20, 2014

Member

Sounds good.

@@ -97,11 +105,13 @@ def run(self, args, config):
relpath = os.path.relpath(abspath, media_dir)
uri = translator.path_to_local_track_uri(relpath)
if relpath.lower().endswith(excluded_file_extensions):
# TODO: move these to a "predicate" check in the finder?

This comment has been minimized.

@adamcik

adamcik Oct 17, 2014

Member

I'm postponing cleaning up where and how this gets filtered given the other changes I have planed wrt. scanning.

jodal added a commit that referenced this pull request Oct 20, 2014

@jodal jodal merged commit c84ed73 into mopidy:develop Oct 20, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@jodal

This comment has been minimized.

Member

jodal commented Oct 20, 2014

@adamcik Merged. Can you update the changelog?

@jodal jodal added this to the v0.20 - Audio cleanup milestone Oct 20, 2014

@jodal jodal added A-audio A-local and removed A-audio labels Oct 20, 2014

@jodal jodal self-assigned this Oct 20, 2014

@jodal

This comment has been minimized.

Member

jodal commented Nov 15, 2014

@adamcik Bump! Need a changelog update for this. I had some memory of this, and went to the changelog to give a link to a user now, but didn't find any trace of this.

@adamcik adamcik deleted the adamcik:feature/improve-find-and-scan-code branch Mar 2, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment