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

Try explicit file name, and look system-wide for library #322

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

shithead
Copy link

Try explicit file name, if the general does not work (e.g. on nixos)
and if no local installed library sndfile, try system-wide

maybe additonal fix for #238, #258, #313,

Try explicit file name, if the general does not work (e.g. on nixos)
and if no local installed library sndfile, try system-wide
@bastibe
Copy link
Owner

bastibe commented Dec 1, 2021

Looks good to me. Is this ready to merge from your end?

@shithead
Copy link
Author

shithead commented Dec 4, 2021

GO, for it! 👍

@bastibe
Copy link
Owner

bastibe commented Dec 7, 2021

Sorry, @shithead, but another pull request recently changed the import behavior for M1 Macs, which broke your pull request.

I can amend your pull request for you if you'd like, but then it's my name in the commit history, not yours. If you'd like the attribution, you'll have to rebase your pull request onto the latest state.

Sorry for the inconvenience.

@shithead
Copy link
Author

shithead commented Dec 8, 2021

I try to solve it.

@shithead
Copy link
Author

shithead commented Dec 8, 2021

@panosl can look that this PR is possible solve your problem like in #311, too?

@shithead
Copy link
Author

shithead commented Dec 8, 2021

dlopen don't need an explicit path if LD_LIBRARY_PATH is right set. There is another solution possible with less complex code

import os as _os
_os.environ['LD_LIBRARY_PATH'] = ":".join([_os.path.join(_path,'_soundfile_data'),"/opt/homebrew/lib","/usr/local/lib",os.environ['LD_LIBRARY_PATH']])

set missing pathes to LD_LIBRARY_PATH environment.

remove machine architecture for darwin. Lib directories not influenced by machine architecture.
@bastibe
Copy link
Owner

bastibe commented Dec 10, 2021

If I remember correctly, LD_LIBRARY_PATH is meant to be customized by the user, not the application. For example, I could imagine a user setting a custom LD_LIBRARY_PATH to force an application to use a debugging library instead of a built-in one. So at the very least, we should append to the end of LD_LIBRARY_PATH in order to not break things, but it's probably prudent to leave it alone altogether.

Do you know of any resource on how to use LD_LIBRARY_PATH correctly, and whether it is permissible for us to modify it in the app?

You just added a commit that seems like it should solve the merge conflict, but it is still listed as conflicting on Github.

@shithead
Copy link
Author

Sry, I am not familiar with Resolving confilicts by webui. I thinks, it's ready now.

@bastibe
Copy link
Owner

bastibe commented Dec 22, 2021

This seems to change the original logic flow I think. Why did you code it this way?

soundfile.py Show resolved Hide resolved
soundfile.py Show resolved Hide resolved

# Homebrew on Apple M1 uses a `/opt/homebrew/lib` instead of
# `/usr/local/lib`. We are making sure we pick that up.
if _sys.platform == 'darwin':
Copy link
Owner

Choose a reason for hiding this comment

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

why did you drop and _machine() == 'arm64'?

@shithead
Copy link
Author

shithead commented Apr 16, 2022

#322 (comment)

Path specification is OS and programm specification not Machinearchitecture like arm64 or amd64. That's why I remove the machinearchitecture test.

Happy Eastern 🥚 .

@davidbernat
Copy link

dlopen don't need an explicit path if LD_LIBRARY_PATH is right set. There is another solution possible with less complex code

import os as _os
_os.environ['LD_LIBRARY_PATH'] = ":".join([_os.path.join(_path,'_soundfile_data'),"/opt/homebrew/lib","/usr/local/lib",os.environ['LD_LIBRARY_PATH']])

set missing pathes to LD_LIBRARY_PATH environment.

My solution required _os.environ['DYLD_LIBRARY_PATH'].

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

3 participants