Skip to content

Intermediate solution for handling the packaged DLL files#98

Closed
mgeier wants to merge 2 commits intomasterfrom
dll-handling
Closed

Intermediate solution for handling the packaged DLL files#98
mgeier wants to merge 2 commits intomasterfrom
dll-handling

Conversation

@mgeier
Copy link
Contributor

@mgeier mgeier commented Jan 10, 2015

WARNING: I didn't test this under Windows!

With this PR I tried to find a middle ground between the current state and the suggestions from #47 and #88.

  • the DLLs are not renamed during installation
  • the DLL from the official libsndfile installer are used, if available

This still assumes that we keep the DLLs in the repository and provide binary Windows installers for the upcoming 0.6.0 release.
At a later point, once a proper installer for libsndfile is available (see libsndfile/libsndfile#89), we can completely remove the DLLs from the repository.

Differences to #88:

  • the DLLs are not included in distribution files (sdist, bdist*) if built on non-Windows.
  • sndfile.dll is not supported anymore
  • the module is not turned into a package

Would this be a meaningful solution for the upcoming release?

Currently, I have no Windows system available, so I couldn't test if this actually works under Windows!

- DLL files are not renamed during installation
- 32bit/64bit DLLs are explicitly selected when importing PySoundFile
- DLLs from the official libsndfile installer are preferred, if available
@mgeier mgeier added this to the 0.6.0 milestone Jan 10, 2015
@bastibe
Copy link
Owner

bastibe commented Jan 12, 2015

If I understand this correctly, all this does is to shift the "renaming" from distribution time to run time. Why would we want to do this?

As far as I know, the correct name for sndfile on Windows is libsndfile.dll. It is important that we keep it that way so that the user can install his own libsndfile.dll if he so desires. In the future, this will hopefully be done by the sndfile installer.

@mgeier
Copy link
Contributor Author

mgeier commented Jan 12, 2015

Right, now that you mention it, I also see that this doesn't really solve anything.
I just wanted to extract the "essence" from #88, where @nils-werner suggested to get rid of the copying.
But I guess this isn't really a problem worth solving, so we can just leave it as is (and completely remove the DLLs at a later time).

But the question of DLL names is still open ...

As far as I know, the correct name for sndfile on Windows is libsndfile.dll.

I don't know if there is such a thing as a correct name on Windows.
The name sounds reasonable, but it's not the "official" name, since the libsndfile installer uses libsndfile-1.dll.

Is there another source which uses the name libsndfile.dll?

It is important that we keep it that way so that the user can install his own libsndfile.dll if he so desires.

But currently this doesn't work!
Currently, only sndfile.dll is used.

Shall we change this from sndfile.dll to libsndfile.dll?

I think it would be good to already support the DLL from the libsndfile installer, so we should probably try these three strings in ffi.dlopen(): ["sndfile", "libsndfile-1", "libsndfile"].

The idea behind this order: First, try the Unix-like name (where lib is prefixed automatically), then try the official installer, finally fall back to the DLL we provide with PySoundFile.
We could do this without checking for the operating system.

How should the files in our repository be named?
I suggest libsndfile32bit.dll and libsndfile64bit.dll.

Since we want to remove the DLLs at some point anyway, we can also just drop this PR and fix the situation later, once a proper libsndfile installer is available.

@bastibe
Copy link
Owner

bastibe commented Jan 13, 2015

If I remember correctly, cffi.dlopen will try all combinations of lib, sndfile and .dll if given sndfile. I can't find a reference for the lib part, though.

Looking through Windows' own libraries in C:\Windows\System32 (64 bit) and C:\Windows\SysWOW64 (32 bit), it generally uses no lib prefix, and no 32/64 suffix. So I guess, sndfile.dll is indeed the correct name.

@mgeier
Copy link
Contributor Author

mgeier commented Jan 13, 2015

If I remember correctly, cffi.dlopen will try all combinations of lib, sndfile and .dll if given sndfile.

It didn't when I tried it.
I had a look at the CFFI sources and it seems that the library is searched with ctypes.util.find_library().

This does add the proper extension, but it doesn't add lib on Windows.

Citing from the documentation: "On Windows, [...], but since there is no predefined naming scheme a call like find_library("c") will fail and return None."

I tried it on a Windows 7 system and ffi.dlopen("sndfile") only finds sndfile.dll, it does not find libsndfile.dll nor libsndfile-1.dll. It doesn't find sndfile-1.dll, either.

So I guess, sndfile.dll is indeed the correct name.

I still wouldn't use the word correct, since apparently there is no naming scheme at all and the official installer uses something completely different.
I think both sndfile.dll and libsndfile.dll would be reasonable names.

Given that, I think for now we should keep the code as is (using only ffi.dlopen("sndfile") and nothing else).

Once an updated libsndfile installer is available (see libsndfile/libsndfile#89), we should check for ["sndfile", "libsndfile-1"] (except Erik decides to rename the DLL to something else) and remove the DLLs from our repository.

I suggest to reject both #88 and this PR (#98), but to cherry-pick the unrelated commit 9bed1db into master.

@bastibe
Copy link
Owner

bastibe commented Jan 14, 2015

I agree.

@mgeier
Copy link
Contributor Author

mgeier commented Jan 15, 2015

OK, I'm closing this.

@mgeier mgeier closed this Jan 15, 2015
@mgeier mgeier deleted the dll-handling branch January 15, 2015 14:09
@nils-werner
Copy link
Contributor

What do you have in mind to fix problem of moving the DLLs around in setup.py?

@bastibe
Copy link
Owner

bastibe commented Jan 15, 2015

There's no solution, really. The Windows bdist installer requires two different DLLs depending on the bit-depth of the installed Python. There is no way to prevent this.

Ideally, I wouldn't want the DLL to be part of the installer at all. Hopefully, we can solve this with the help of the maintainer of sndfile. The only other way would be to somehow move the DLLs out of this repository and create the bdist installers outside of this repo.

I'm open for suggestions. But moving the code from setup.py to pysoundfile.py is not a solution.

@nils-werner
Copy link
Contributor

What I mean: you're copying the files outside of setuptools.

The files will be copied whether I do python setup.py install, python setup.py build_sphinx or python setup.py nonsense.

If you need to copy the files you should at least do so in the corresponding command (sdist, bdist, build etc)

@bastibe
Copy link
Owner

bastibe commented Jan 15, 2015

True. I hope I'll have time to look into that, soon.

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.

3 participants