Skip to content

Conversation

@nils-werner
Copy link
Contributor

Sorry for flooding you guys with PRs again :-)

The situation is currently as follows:

  • setup.py contains unsafe and untested executable code (can and does error on some OSes)
  • DLLs are not included in sdist distributions (Windows cannot pip install)
  • DLLs may or may not be included in your installation
  • DLLs shipped may or may not hide system-wide lib installations due to name clash

This PR combines ideas and changes from #88, #98 and #110:

  • Does not move DLLs in setup.py
  • Includes all DLLs in sdist distributions, even if not needed
  • Includes all DLLs during installation, even if not needed
  • Differentiates DLLs during runtime, if needed

This dramatically reduces the surface of scenarios on which errors may appear:

  • setup.py remains simple and declarative, working exactly the same on all OSes
  • pip install works exactly the same on all OSes
  • Installation structure looks exactly the same on all OSes
  • Users may install their own libsndfile, as default names are assumed first; fallback to shipped DLLs comes last
  • Windows does not require a custom made binary installer

And thusly fixes the following errors:

  • pip install pysoundfile does not work on Windows, pip install -e . does
  • On Windows, python setup.py help moves files about, even when not intended to

@bastibe
Copy link
Owner

bastibe commented Mar 3, 2015

Very interesting. I am slowly coming around to the idea.

Are there any other pip packages out there that ship compiled code? This still seems troublesome to me.

If we do go down this route, we should ship OSX binaries as well. Linux users would probably prefer to use their package manager instead.

@nils-werner
Copy link
Contributor Author

Are there any other pip packages out there that ship compiled code? This still seems troublesome to me.

Not that I am aware of. But simply shipping it as if it was some random binary file (which plenty of pip packages are doing) seems like a neatly simple solution to me: Other OSes just don't care and it potentially makes the installation on Windows less cumbersome. All that without requiring any special building and distributing on your side.

If we do go down this route, we should ship OSX binaries as well. Linux users would probably prefer to use their package manager instead.

Thankfully there is Homebrew so there is no need for this. Both OSX and Linux are capable of installing the lib themselves (and it is second nature to devs on them to do so).

@bastibe
Copy link
Owner

bastibe commented Mar 3, 2015

Thankfully there is Homebrew

As much as I would like to agree with you there, few Mac users actually know how to use homebrew. I think that including a Mac binary would help a lot of people.

@nils-werner
Copy link
Contributor Author

Oh boy, I hope that isn't true :-) I don't think it is your responsibility though to cater to some people, just because they're lazy. I would mention Homebrew in the readme and be done with it, honestly.

@mgeier
Copy link
Contributor

mgeier commented Mar 4, 2015

I think there are at least two - rather disjoint - groups of potential users:

  1. Python programmers
  2. Scientific Python users

Note that I didn't call the second group "programmers", that's because I think most of them aren't.
For the first group it should be enough to say "get the libsndfile library" and it would probably be a luxury to add "you can use homebrew for that".
The second group, however, has probably never heard of "homebrew", neither "pip", nor has ever knowingly used a compiler and probably doesn't even know what the usual file extension of a dynamic library is on the operating system they're using. They will know what operating system they're using, though.

We should make the installation process as easy as possible for the second group, the first group will manage anyway.
We should document the installation process in a very detailed way, because it may be the first Python package our users have ever installed.

Having said that, I don't think it's a good idea to add a few megabytes of binary garbage to a source package that has a size of 32k.

That's what binary packages are for!
We should probably try to create Wheel packages. I don't know how that works and I don't know if an external library can be included. But probably this isn't even necessary (see below).

It's probably more important to get PySoundFile into Python distributions like Anaconda (if it's possible to include libsndfile in the package) and into more generic distributions like homebrew/MacPorts/Debian/...., which already contain a package for libsndfile.

On the long run, we should delete the DLLs from our repository (and not add dylibs or anything) and, when installed with pip, the library installed on the system should be used.
The next release of libsndfile will have an installer for Windows that will install the DLLs in a way that they can be found by CFFI, see #47.
Installing libsndfile with homebrew/macports/whatever should also work.

In the end, there should be two ways of installing PySoundFile:

  1. install a package (for Anaconda/Debian/homebrew/...) and all dependencies will be automatically taken care of and it will simply work
  2. install libsndfile separately (with the Windows installer/homebrew/compiling it yourself/...) and afterwards install PySoundFile with pip or python setup.py ... or by copying pysoundfile.py around. After those two steps it should also just work.

I don't know if binary packages (wheels or something) are necessary at all, but I also don't have any experience with them.

@nils-werner
Copy link
Contributor Author

I absolutely agree, the binaries shouldn't be here at all.

  1. The Python-side of PySoundFile should not care about anything that is beyond its control (libsndfile installation), the installation should just assume everything is in place
  2. If there will be a installer for conda, apt-get, yum, homebrew, those should not ship binaries but only declare the dependencies
  3. For Windows, there should be a readme entry pointing to the new libsndfile-installer, once that thing is done. At that point, the win/ directory should be removed from PySoundFile.

This PR only tries to fix the current situation: PySoundFile cannot be installed on Windows using pip.

@mgeier
Copy link
Contributor

mgeier commented Mar 4, 2015

OK, I didn't know that this PR is supposed to be a temporary solution!

I thought the current temporary solution was to provide Windows installers for PySoundFile (see https://github.com/bastibe/PySoundFile/releases).

What's wrong with that?

This doesn't pollute the source package, and once the new libsndfile installer is available, we won't have to provide our installers anymore.

@nils-werner
Copy link
Contributor Author

You cannot install them using pip (and thus, no setup.py or requirements.txt may ever reference it when you have colleagues working on Windows).

@mgeier
Copy link
Contributor

mgeier commented Mar 4, 2015

I see.
But isn't this a general (not Windows-specific) problem?

I don't really see a solution for that except raising a RuntimeError if dlopen() fails, stating that the library wasn't found and the user should install it (probably giving a link to the documentation).
Once the user has installed it (e.g. with our installers), it will work again.
No need to have a Windows-specific special case there.

The Windows-specific problem here is that the copying in setup.py raises an error if the DLLs aren't there, so this should probably be fixed (by ignoring errors during copy).

@nils-werner
Copy link
Contributor Author

Yes. But I wonder why the copy statements exist in there if they are expected to always fail?

Do you need them to create the .exe installers (how are they created btw, bdist_wininst)?

@mgeier
Copy link
Contributor

mgeier commented Mar 4, 2015

Yes, as far as I know, that's the only reason (except of course it also works if you install directly from the Git checkout).
But @bastibe creates the installers, I don't know the exact process.

Anyway, this is "dead code walking", since it will be completely removed once the new libsndfile installer comes out.

@bastibe
Copy link
Owner

bastibe commented Mar 4, 2015

I am indeed using bdist_wininst, and only include the correct dll in the installer. The dll has to have the correct name though, otherwise dlopen won't find it; That's why there needs to be that renaming step. This is clearly not an optimal solution, though.

The thing is: Since the binary installers are required on Windows anyway, as there is no practical way of installing sndfile otherwise, it never occurred to me that pip install would fail there. This clearly needs to be fixed, though.

@bastibe bastibe modified the milestone: 0.7.0 Mar 4, 2015
@bastibe bastibe self-assigned this Mar 4, 2015
@bastibe
Copy link
Owner

bastibe commented Mar 5, 2015

We decided to publish the next version as pip wheel and deprecate the binary Windows installers. This will hopefully solve this issue, and #110 as well.

@nils-werner
Copy link
Contributor Author

I see! Well, in that light this PR makes little sense.

Wheels definitely sound good. You need to find a way that keeps the "we're only doing this for Windows" stuff out of the sdist though :-/

@nils-werner nils-werner closed this Mar 6, 2015
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