Skip to content

Remove libmagic dependency#58

Merged
znation merged 6 commits into
xetdata:mainfrom
znation:zn/pr-remove-libmagic
Jul 20, 2023
Merged

Remove libmagic dependency#58
znation merged 6 commits into
xetdata:mainfrom
znation:zn/pr-remove-libmagic

Conversation

@znation
Copy link
Copy Markdown
Contributor

@znation znation commented Jul 19, 2023

Reasons for removing libmagic are numerous, but the main ones are:

  • It's very slow
  • It lies about file types
  • It doesn't work on Windows

This PR replaces file type detection using libmagic with a file-extension based lookup (see file_types.rs). The lookup table is from file extension to mime type and friendly name, and uses phf for efficient compile-time construction of the hash table.

@znation znation requested review from hoytak, seanses and ylow July 19, 2023 09:31
Comment thread rust/libmagic/src/file_types.rs Outdated
@hoytak
Copy link
Copy Markdown

hoytak commented Jul 19, 2023

I like it, +100.

@seanses
Copy link
Copy Markdown
Contributor

seanses commented Jul 19, 2023

I like it, +100.

There are extension collisions in that wiki list. Definitely need some manual screening there.

Copy link
Copy Markdown
Contributor

@jgodlew jgodlew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! :shipit:

@znation
Copy link
Copy Markdown
Contributor Author

znation commented Jul 19, 2023

@hoytak, @seanses: how about we merge this PR, then open separate issues for (1) renaming libmagic, and (2) updating the file type list using the Wikipedia data (with some manual effort)?

@hoytak
Copy link
Copy Markdown

hoytak commented Jul 19, 2023

Sounds good to me!

@znation znation merged commit 5b4996e into xetdata:main Jul 20, 2023
@znation znation deleted the zn/pr-remove-libmagic branch July 20, 2023 04:31
hoytak pushed a commit that referenced this pull request Jul 20, 2023
Caused by #58 -- did not run tests before merging.
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.

4 participants