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

Add support for file's builtin magic module #120

Merged
merged 1 commit into from Jul 27, 2020

Conversation

felixonmars
Copy link
Contributor

file's builtin magic module has a somewhat different API, and it's not
co-installable with pypi:python-magic as both has the same name. Adding
a fallback logic here makes mocket work with either one.

All related tests are passing here.

@mindflayer
Copy link
Owner

Hi @felixonmars, my main concern is about how to test the two solutions, since they have the same namespace import magic. Fixing the issue you opened (#119) does not seem the most difficult part of the change.

@mindflayer
Copy link
Owner

mindflayer commented Jul 27, 2020

I was having a look at https://pypi.org/project/filemagic/ and I thought it was the module you are trying to support but I see it implements a slightly different API. Is that what you used?
If so I guess you wanted to use the flag magic.MAGIC_MIME_TYPE:

>>> magic.MAGIC_MIME_TYPE
16
>>> magic.MAGIC_MIME
1040

@felixonmars
Copy link
Contributor Author

felixonmars commented Jul 27, 2020

I was having a look at https://pypi.org/project/filemagic/ and I thought it was the module you are trying to support but I see it implements a slightly different API. Is that what you used?

Actually, it's yet another. The one I use is bundled inside upstream "file" project and unfortunately not available from PyPI: https://github.com/archlinux/svntogit-community/blob/packages/python-magic/trunk/PKGBUILD#L12

So projects like s3cmd has implemented a quite complex compatibility check for the three versions:
https://github.com/s3tools/s3cmd/blob/3d4a4caf91582c2a674deaa837a5e07d96510059/S3/S3.py#L55

@mindflayer
Copy link
Owner

mindflayer commented Jul 27, 2020

The one I use is bundled inside upstream "file" project and unfortunately not available from PyPI

So the question is: how to test it in a CI environment if not available in PyPI?

@felixonmars
Copy link
Contributor Author

So the question is: how to test it in a CI environment if not available in PyPI?

I see no easy answer either. Since the Arch repository version is the "file" one, and I always run tests during packaging, would you feel okay if I report back in a timely manner if the tests are not passing?

@mindflayer
Copy link
Owner

mindflayer commented Jul 27, 2020

In that case I will have to exclude that block from mocket's test coverage. Of course I'll be more than happy to accept PRs or issues from you.
Could you try to see if the other flag I suggested is available in the library you are testing? I am curious to see if that's the reason for the two different results.

file's builtin magic module has a somewhat different API, and it's not
co-installable with pypi:python-magic as both has the same name. Adding
a fallback logic here makes mocket work with either one.

All related tests are passing here.
@felixonmars
Copy link
Contributor Author

@mindflayer Indeed, that's the problem. I have updated this PR to use MAGIC_MIME_TYPE instead and reverted the test change, all tests are now passing.

@mindflayer
Copy link
Owner

OK, let me finish what I am doing for my job and I'll release a version of mocket with the changes you need for make it work on Arch.
By the way, I used to be an Arch user, so I am really happy to help.

@felixonmars
Copy link
Contributor Author

Thanks a lot :)

@mindflayer mindflayer merged commit 27ae893 into mindflayer:master Jul 27, 2020
mindflayer added a commit that referenced this pull request Jul 27, 2020
@mindflayer
Copy link
Owner

@felixonmars felixonmars deleted the file-magic branch July 27, 2020 17:04
@felixonmars
Copy link
Contributor Author

Thanks!

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