Remove the version restriction on mutagen #619
Conversation
|
Looks like we found the "python tests" that were failing that mutagen restriction was aimed at. So now I get to look into the tests and try to figure that out. |
hairmare
changed the title
|
Ok, so I did more investigation for this, basically all that changed was the bitrate that mutagen determines. If you comment out the bitrate assertion it passes. I suspect it is because of this the change in 1.39 and made in this PR. My suggestion is that we change the assertions to the bitrate that mutagen now reports and merge this so that we can use the newest version that has the fixes since we are installing it from pip vs. the one included in the distro. If for some reason we want to support the older version or suspect that this would happen again we could change it to check and see if the bitrate is within a reasonable range vs. an exact number and/or we could just not worry about the tests regarding the bitrate. |
…n results from different mutagen versions
Robbt
changed the title
|
I decided testing it on the range of bitrate made the most sense in terms of dealing with this particular problem that different mutagen versions show different bitrates for mp3s. I also don't think the bitrate test was really testing anything with analyzer itself. |
|
Plot twist: Just out of interest, what would the numbers have look if you just fixed the tests for the new mutagen version rather than allowing a range? Maybe they add up to something smart like 128 kbit/s or 64 kbit/s (the latter being rather weird in itself but not impossible). Could it make sense to trust mutagen on this and not assert a range? |
|
Yeah, I don't know here is what the new version provides, which are 2 bytes shy of 64000 and 128000 on all of them so they're probably more accurate than the previous one. For ogg there are round numbers. |
|
I'm happy to rewrite the test to match the new output but it seems like the bitrate is unrelated to anything actually being tested on the test and so the test will fail if they have the older version of mutagen but with a range it'll work with either version but fail if something gets out of wack completely with the bit rate. Changing it to the new version probably makes sense as every other test passed with mutagen 1.41 |
|
The mutagen changelog has lots of notes about improving bitrate accuracy for MP3 files so I'd say it's safe to assume the new values are sane... It could be an interesting challenge to figure out where exactly those two byte are going missing, we really would expect to see 64000 and 128000 tbh. |
|
merging this one would unblock #639 which would then (i think) let @ned-kelly use the main libretime repo for his multicontainer docker repo. i however am unqualified to merge this. @hairmare ? gracias! |
|
@paddatrapper do this one and #639 look ok to you? |
|
So @paddatrapper I just added you as a maintainer, our intention was to ask you if you would like to be on the maintainer team before adding you but I thought that already happened and so I jumped the gun. So if your willing you should be able to merge this now. |
|
Heh thanks. Merged |
|
i just did a fresh install from master at a subsequent commit in ubuntu 16, noticed that it looked like mutagen 1.31 was still being installed, ran can anyone else reproduce this? |
|
Ubuntu seems to have a patched 1.31. Are you sure that pip isn't just showing the version of the distro package? I didn't check but I kind of remember pip also knowing about site-packages installed by the distro. |
|
Pip does know about distro-provided python packages (they're installed in similar paths under the python install). This skidoo means pip should be able to update a distro-provided package. Do we install python-mutagen via apt or mutagen via pip in the installer? |
It looks like we let setuptools do it's thing and it installs mutagen through pip. In my CentOS packages I install the distro package which leads to setuptools not trying to reinstall/update mutagen. |
|
So it is definitely installing it via pip but for some reason it thinks that 1.31 is the best version, perhaps there is a preferred version somewhere set by the distribution. I included a commit to require version 1.41 or better in the airtime-celery non-mp3 fix PR in #639. It is working. I don't know enough about how Python determines what version to use and the documentation is somewhat obtuse but this appears to work so hopefully that can be considered to resolve this. |
This fixes #610 and possibly other issues regarding mutagen. In theory it could introduce new bugs but I suspect the version restriction wasn't necessary and was geared towards simply avoiding the need to rewrite tests. We can respond to new issues if we are able to find them but this should fix a number of potential issues based upon the number of bug fixes from 1.31 to 1.41.