ahupp / python-magic

A python wrapper for libmagic
Other
2.59k stars 280 forks source link

Allow environment variable to specify full path to libmagic shared lib #315

Closed bbenne10 closed 2 months ago

bbenne10 commented 4 months ago

As it currently sits, python-magic does not allow the user to customize the full path to magic.

This is kind of okay on a linux host, as you can set LD_LIBRARY_PATH and ctypes.util.find_library will respect this. On MacOS though, this is not the case.

This makes using python-magic on a macOS host with a nix-managed development tree quite hard. One needs to either symlink some files around (and know that nix might cleanup the path at any arbitrary garbage collection) or install magic via homebrew (which I personally do not use otherwise) because there's a fixed set of places to look in specified by hand.

It seems to me that something like the following might help (at the top of loader.py:

def _lib_candidates():
  user_set_magic_path = os.environ.get("PYTHON_MAGIC_SO_PATH")  # Maybe someone has a better name?
  if user_set_magic_path:
    yield find_lib(user_set_magic_path)

  yield find_library('magic')
  # ... rest of function body

I'm happy to put a PR together for this if someone can give me the word on what the environment variable should be called.

ddelange commented 4 months ago

about your example implementation: if you set that env var and that path doesnt exist (or has a typo), it will be silently skipped and the next yielded paths will be tried. see also https://github.com/ahupp/python-magic/pull/279

regarding naming: the only name that's already taken is the MAGIC env var which may point to a magic.mgc mime database (as per file man page).

also, brew-built libmagic will be bundled (and your problem will probably be solved) when https://github.com/ahupp/python-magic/pull/294 merges

bbenne10 commented 4 months ago

about your example implementation: if you set that env var and that path doesnt exist (or has a typo), it will be silently skipped and the next yielded paths will be tried. see also https://github.com/ahupp/python-magic/pull/279

Yes - That's fine (for my use at least). It might be nice to know that it is the env variable that is broken, but it is pretty apparent that something's wrong when the final error gets thrown.

regarding naming: the only name that's already taken is the MAGIC env var which may point to a magic.mgc mime database (as per file man page).

I didn't know that - thanks for the information. That doesn't REALLY relate here though (other than defining what we cannot call it - though I wouldn't have picked MAGIC anyway)

also, brew-built libmagic will be bundled (and your problem will probably be solved) when https://github.com/ahupp/python-magic/pull/294 merges

That's right - though I can imagine a scenario where users may want to override the found path anyway. I'd be happy with either scenario - bundle the .so or allow me to say where it is. We are in the worst case "semi-working" scenario right now, in my experience.

That being said: your PR has been open for ~8 months and has not been merged. I'm not knocking the quality at all - but maybe a smaller change set has a chance of going through faster? 🤷🏼

ahupp commented 3 months ago

Does DYLD_LIBRARY_PATH work here? I think that's the macos equivalent to LD_LIBRARY_PATH.

bbenne10 commented 3 months ago

I will check on Tueaday when I am back at my Mac. ctypes.find_library is not documented to work this way, so I didnt think to check when I opened this PR.

bbenne10 commented 3 months ago

I will say though that it looks promising. ctypes.util.find_library is conditionally defined and then calls into ctypes.macholib.dylib, which seems to respect DYLD_LIBRARY_PATH.

bbenne10 commented 2 months ago

...i have obviously forgotten to do this. Ill check when I get to the office today

bbenne10 commented 2 months ago

I tested this but TOTALLY forgot to come here to give an update. DYLD_LIBRARY_PATH works! It's just way under-documented. This might be worth a documentation update?

ahupp commented 2 months ago

thanks for the update, I've updated the readme to mention this.