python-soundfile icon indicating copy to clipboard operation
python-soundfile copied to clipboard

Sloooowly remove SoundFile.__len__()

Open mgeier opened this issue 7 years ago • 22 comments

See #199.

This will be a breaking change and should therefore be very gentle, with long breaks between those action points:

  • [x] Deprecate .__len__() and introduce .frames as replacement (#206); released in v0.10.1 (2018-03-02)
  • [ ] Generate a deprecation warning when len() is used
  • [ ] Remove .__len__() and remove .__bool__()/.__nonzero__() which becomes obsolete

mgeier avatar Aug 23 '17 19:08 mgeier

I agree! This seems to be the right course of action. Thank you for taking charge!

bastibe avatar Aug 23 '17 21:08 bastibe

Great! So what about a new release to get things going?

mgeier avatar Sep 02 '17 08:09 mgeier

Do you think this should be a 0.9.1 or a 0.10.0?

Sorry for the slow rate of updates. Life is busy right now.

bastibe avatar Oct 16 '17 14:10 bastibe

I don't have a strong opinion on that as long as we are still below 1.0, either option seems fine to me.

We should probably try to make releases not such a big deal, e.g. by using a script such as https://github.com/bastibe/PySoundCard/blob/master/build_wheels.sh or https://github.com/spatialaudio/python-sounddevice/blob/master/make_dist.sh to speed up the process.

Shall I try to create such a script?

It probably also helps to make a little checklist, like I did at the end of this file: https://github.com/sfstoolbox/sfs-python/blob/master/CONTRIBUTING.rst?

Is there anything else I can do to help?

mgeier avatar Oct 17 '17 10:10 mgeier

That script is exactly what I am still using (once a year). I would be grateful for any assistance in this regard.

A CONTRIBUTING.rst would be a great idea, too--although its contents should probably be discussed in a pull request. In particular, I would like to include something like

However, please be aware that this is a hobby project of mine that I am developing for free, and in my spare time. While I try to be as accomodating as possible, I can not guarantee a timely response to issues. Publishing Open Source Software on Github does not imply an obligation to fix your problem right now. Please be civil.

(this is a quote from one of my other projects and would of course have to be changed).

Just a heads-up, though: I'm expecting a baby in the next few days, so my response time might be even longer than usual.

bastibe avatar Oct 17 '17 11:10 bastibe

Wow, I wish you a lot of fun in your first days of fatherhood!

We already have a CONTRIBUTING.rst (since #66), feel free to add your text there. TBH, I don't really like the text because it sounds a bit condescending, but it's up to you.

If you use one of those scripts, we should probably copy it to the repo?

mgeier avatar Oct 19 '17 16:10 mgeier

We already have a CONTRIBUTING.rst (since #66), feel free to add your text there.

Right, sorry about that. I didn't look into this much.

TBH, I don't really like the text because it sound a bit condescending, but it's up to you.

As I said, I wouldn't want to add that text verbatim, just something to a similar effect. I do get a bit of flak on social media whenever we push updates, and it is very annoying. Ever since I added a text like this to my other repos, responses have been more civil. I guess people expect commercial-grade support if they see commercial-grade software and documentation.

If you use one of those scripts, we should probably copy it to the repo?

I thought I used the script you linked to, but apparently I was wrong (apparently I was wrong a lot in that last post). Here's the actual script I use:

#!/usr/bin/env fish

rm dist/*.whl

rm -r build
set -xg PYSOUNDFILE_PLATFORM darwin
python setup.py bdist_wheel upload

rm -r build
set -xg PYSOUNDFILE_PLATFORM win32
set -xg PYSOUNDFILE_ARCHITECTURE 32bit
python setup.py bdist_wheel upload

rm -r build
set -xg PYSOUNDFILE_PLATFORM win32
set -xg PYSOUNDFILE_ARCHITECTURE 64bit
python setup.py bdist_wheel upload

rm -r build
set -xg PYSOUNDFILE_PLATFORM noplatform
set -xg PYSOUNDFILE_ARCHITECTURE noarch
python setup.py bdist_wheel upload
python setup.py sdist upload

I then run this script twice, while renaming "PySoundFile" to "SoundFile". But it's an ugly hack. I don't like how this script doesn't use twine, and how it doesn't automate the package renaming process, and how it's specific to my shell (fish). That's why I haven't uploaded it to the repo.

Speaking of which, do you have an idea how to deprecate "PySoundFile" without too much breakage?

bastibe avatar Oct 20 '17 06:10 bastibe

what about using cibuildwheel for releases?

tgarc avatar Oct 25 '17 15:10 tgarc

Wow, that sounds cool! This would be awesome!

I wouldn't want to automate the PyPi upload, though.

bastibe avatar Oct 26 '17 06:10 bastibe

Ah, I forgot you guys are bundling the libsndfile library in which makes things trickier...Anyway, I don't foresee having time for this in the near future so if you could hobble along with the current script to make the next release that would be much appreciated :D

tgarc avatar Nov 01 '17 15:11 tgarc

OK, the new release should be online now. Could you check if it works right?

Edit:

Something seems to be wrong, at least on macOS if I don't have libsndfile installed:

>>> import soundfile
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/bb/Projects/pysoundfile/soundfile.py", line 162, in <module>
    __libsndfile_version__ = _ffi.string(_snd.sf_version_string()).decode('utf-8', 'replace')
ffi.error: symbol 'sf_version_string' not found in library '<None>': dlsym(RTLD_DEFAULT, sf_version_string): symbol not found

It seems to work fine on Windows.

I have pulled the release from PyPI for that reason.

Find attached the wheels in question: Wheels.zip

I'd be grateful for help. Maybe the libsndfile binaries are outdated? Sorry but my time is extremely limited at the moment, so I might not be able to respond quickly.

bastibe avatar Nov 12 '17 09:11 bastibe

unfortunately I don't have a mac for testing...

It's confusing that cffi is reporting not found in library '<None>'. None usually means stdlib in cffi land.

It looks like sf_version_string was added in ee663d which was before the 1.0.20 release (May 14 2009) so it shouldn't be an issue with the binaries. Is it possible that there is a very old libsndfile installed on the test system which is getting loaded instead?

tgarc avatar Nov 14 '17 01:11 tgarc

I don't have a Mac either, but I have a possible lead:

I tried ctypes.util.find_library() on Linux with some nonsense string that doesn't exist as a library and it returns None instead of raising an exception!

Probably on macOS the behavior is the same? And probably on Windows it does throw and therefore everything works as expected?

mgeier avatar Nov 14 '17 10:11 mgeier

It turns out that find_library() is actually supposed to return None: https://docs.python.org/3/library/ctypes.html#finding-shared-libraries

I guess the difference is that ffi.dlopen(None) throws on Windows and doesn't on macOS?

mgeier avatar Nov 14 '17 10:11 mgeier

Okay I didn't know that about ctypes find_library. Thanks @mgeier. So soundfile should probably throw an exception itself if it gets None as a return value. The fact that the library can't be found though is maybe an issue with the test system? Either way, I think the exception raising should be added before making the next release. Ill make a PR.

On Nov 14, 2017 4:28 AM, "Matthias Geier" [email protected] wrote:

It turns out that find_library() is actually supposed to return None: https://docs.python.org/3/library/ctypes.html#finding-shared-libraries

I guess the difference is that ffi.dlopen(None) throws on Windows and doesn't on macOS?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/bastibe/SoundFile/issues/207#issuecomment-344214212, or mute the thread https://github.com/notifications/unsubscribe-auth/AAaVHPEZFFYfC6gzaJL9p_AD-_ahSC7Sks5s2WtfgaJpZM4PAcsX .

tgarc avatar Nov 14 '17 15:11 tgarc

@tgarc you can have a look at a possible solution of the same problem in the sounddevice module: https://github.com/spatialaudio/python-sounddevice/pull/111

Good point about the test system, but I think we are fine, because libsndfile has to be properly installed on the (Linux) test system anyway. If we ever get a CI service using macOS (or even Windows), we'll have to make sure to clone the submodule or install libsndfile on the system. Either way should work though.

But the automatic docs build might be a problem! We'll probably have to create a fake version of find_library() for that?

mgeier avatar Nov 14 '17 19:11 mgeier

I've updated https://github.com/spatialaudio/python-sounddevice/pull/111 with a possible solution to the docs problem: https://github.com/spatialaudio/python-sounddevice/pull/111/commits/387351a9768684022862b0c2152d94eb44a68c3b

If you have a better idea, please let me know!

mgeier avatar Nov 14 '17 19:11 mgeier

see #215 for find_library fixes. These are a duplicate of the patches @mgeier used for sounddevice.

tgarc avatar Nov 17 '17 19:11 tgarc

@bastibe have you tested running on MacOS again after #215 ? Is there something else you're waiting on?

tgarc avatar Dec 05 '17 05:12 tgarc

@tgarc sorry for my slow pace. I currently have a newborn and my in-laws at home, and only very rarely enough time to actually delve into something for more than a few minutes (which leads to problems like the previous non-release). I hope that there will be time before Christmas.

bastibe avatar Dec 05 '17 08:12 bastibe

hehe that's not a problem @bastibe, sounds like you have more important things to worry about :) just wanted to know if you were waiting for anything from me.

On Tue, Dec 5, 2017 at 2:32 AM, Bastian Bechtold [email protected] wrote:

@tgarc https://github.com/tgarc sorry for my slow pace. I currently have a newborn and my in-laws at home, and only very rarely enough time to actually delve into something for more than a few minutes (which leads to problems like the previous non-release). I hope that there will be time before Christmas.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/bastibe/SoundFile/issues/207#issuecomment-349232512, or mute the thread https://github.com/notifications/unsubscribe-auth/AAaVHOZv0xorCqMRbQYPLKwPaliDcVutks5s9P-3gaJpZM4PAcsX .

tgarc avatar Dec 05 '17 15:12 tgarc

FYI, my schedule is clearing up a bit, and I hope to resume work on SoundFile in the next few days.

bastibe avatar Feb 28 '18 11:02 bastibe