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

Update filemagic to new API to fix meme crash #124

Closed

Conversation

clintgibler
Copy link
Contributor

When running scripts/promnesia index on macOS, I received the following error:

TypeError: __init__() got an unexpected keyword argument 'mime'

From src/promnesia/sources/auto.py#L22:

@lru_cache(1)
def _magic():
    import magic # type: ignore
    return magic.Magic(mime=True)

Reviewing the filemagic docs, as linked by @flinge in #122, I was able to fix the error by changing the arguments to the magic.Magic constructor and use magic.id_filename() instead of magic.from_file().

Notes

I'm not familiar with the Promnesia code base nor filemagic, so honestly I'm not sure if this code behaves as it should. Would greatly appreciate your feedback 🙏

Also, from the filemagic docs,

To ensure that resources are correctly released by magic.Magic, it’s necessary to either explicitly call close() on instances, or use with statement.

Since _magic() returns an instance of magic.Magic without a close() or using with, the code may currently be leaking file descriptors, though I'm not sure.

It would probably be good to use a with statement unless there's some huge performance or other impact I'm not aware of. This PR does not use with because I was trying to make the diff as small as possible.

@karlicoss
Copy link
Owner

Thanks! Appreciate the fix, but let me know if using python-magic solves this for you #122 (comment)

But well spotted about closing -- it seems that python-magic doesn't implement a context manager, even though in principle it probably should (it relies on __del__ at the moment https://github.com/ahupp/python-magic/blob/master/magic.py#L118-L131). I guess this is not super critical though -- even if it's leaking some resources, they will be released when indexing is finished along with the whole process. Perhaps this will matter more later if realtime indexing is added.

@karlicoss karlicoss mentioned this pull request Jul 5, 2020
@clintgibler
Copy link
Contributor Author

It does! I commented as well: #122 (comment). Closing this PR.

I see you beat me to adding a link to this on #20 :)

@clintgibler clintgibler closed this Jul 5, 2020
@clintgibler clintgibler deleted the fix-libmagic-meme-error branch July 5, 2020 20:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants