1.1.0 h239cbf + mpg123 and lame

[bmcfee]

Checklist

• [x] Used a personal fork of the feedstock to propose changes
• [x] Bumped the build number (if the version is unchanged)
• [x] Reset the build number to 0 (if the version changed)
• [x] Re-rendered with the latest conda-smithy (Use the phrase code>@<space/conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
• [x] Ensured the license file is being packaged.

This PR adds lame and mpg123 to the dependencies, but is otherwise identical to the update generated by the autotick bot.

[conda-forge-linter]

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

[bmcfee]

I'll try to come back to this later on, but this isn't working yet because the mpg123 package on conda-forge needs to be updated to 1.25.10.

[wolfv]

@bmcfee thanks for this! Do you want to try your hand at a PR for mpg123 as well?

[bmcfee]

@bmcfee thanks for this! Do you want to try your hand at a PR for mpg123 as well?

I can, but I probably won't be of much use if it doesn't go entirely smoothly. BTW would protocol here be to bump to the minimum next version (1.25.10 for the purposes of libsndfile) or the latest release (1.29.3)?

[wolfv]

I think latest release sounds fine

[bmcfee]

I think latest release sounds fine

That was my thought as well after spending some time poring over the changelogs. I think the PR over there is good at this point, however it doesn't include a windows build, and that may complicate things for the libsndfile package. Two options I see:

1. Have slightly different builds of libsndfile: linux and osx include mp3 support, windows doesn't.
2. Fix the mpg123 (and lame) conda-forge packages to have windows builds so that the libsndfile build can have uniform functionality.

(1) is obviously easier, but (2) would be vastly preferable from both the user and downstream maintainer perspectives. Only trouble is that I have zero experience with windows builds (and honestly it's been decades since I spent any appreciable time with c/c++ builds on *n*x) so I don't think I'd be able to get that going.

[bmcfee]

Updated the recipe to skip lame/mpg123 on windows until https://github.com/conda-forge/lame-feedstock/pull/7 and https://github.com/conda-forge/mpg123-feedstock/issues/28 are resolved.

[bmcfee]

Also waiting on https://github.com/conda-forge/mpg123-feedstock/pull/30 to get feature parity on linux-aarch64/ppc64le builds here (currently failing) and windows.

[bmcfee]

Trying to resume this one now that lame shipped a windows build, and hitting a bit of a wall.

Apparently LAME does not ship a cmake config, so libsndfile has trouble locating it out of the box. I've set up a local windows virtualbox environment, got the build tools and a conda environment set up there, and adding -DLAME_INCLUDE_DIR:PATH="%LIBRARY_PREFIX%" to cmake does let it find the installed lame package. (It compiles on my machine, but does not link, probably due to having skipped some cross-platform packages in the vs build tools setup to save space.)

Adding this same switch to the bld.bat here does not work, however, and I'm not sure why.

[bmcfee]

It's possible this is related to https://github.com/libsndfile/libsndfile/issues/816

[bmcfee]

Very weird! It looks like the problem is that the tar.xz distribution of 1.1.0 lacks the cmake files, but the .zip distribution does. (This appears to have been a post-release fix.) This would explain why things build locally for me but not via the build recipe.

This would not have been caught on the linux / osx builds because those use autotools rather than cmake.

I'll try this again using the .zip distro for all platforms and see if that fixes things.

[bmcfee]

Using the .zip download does not work on all platforms, so I've temporarily hacked the meta.yaml to grab .zip on windows and .tar.xz on linux/osx. This seems to have done the trick, but surely it's not great practice going forward. I expect that when 1.2.0 is released, we'll be able to revert to a single download URL. @wolfv do you have any thoughts about this proposed (temporary) fix?

This PR is still blocked by mpg123, but once that merges I think this will be clear to go.

[bmcfee]

mpg123 updates are merged upstream, so I think this PR is unblocked. Once the dust settles (https://github.com/conda-forge/mpg123-feedstock/runs/8725787835 ) I'll finalize this recipe to include mpeg build on windows.

[bmcfee]

This is now failing on windows again. When mpg123 was missing, libsndfile would bypass the mpeg module entirely. Now that it's there, it's actually trying to build mp3 support, and hitting new snags with the lame distribution: https://dev.azure.com/conda-forge/feedstock-builds/_build/results?buildId=578509&view=logs&j=a70f640f-cc53-5cd3-6cdc-236a1aa90802&t=f5d15007-a01c-5ad8-c9ce-4d519d3b275f&l=820

...
FAILED: CMakeFiles/sndfile.dir/src/id3.c.obj 
C:\PROGRA~2\MICROS~2\2019\ENTERP~1\VC\Tools\MSVC\1429~1.301\bin\Hostx64\x64\cl.exe  /nologo -D_CRT_NONSTDC_NO_DEPRECATE -D_CRT_SECURE_NO_WARNINGS -Dsndfile_EXPORTS -I%SRC_DIR%\include -I%SRC_DIR%\build\include -I%SRC_DIR%\src -I%SRC_DIR%\build\src -external:I%PREFIX%\Library\include -external:I%PREFIX%\Library -external:W0 /DWIN32 /D_WINDOWS /W3 /MD /O2 /Ob2 /DNDEBUG /showIncludes /FoCMakeFiles\sndfile.dir\src\id3.c.obj /FdCMakeFiles\sndfile.dir\ /FS -c %SRC_DIR%\src\id3.c
%SRC_DIR%\src\id3.c(32): fatal error C1083: Cannot open include file: 'lame/lame.h': No such file or directory

This is after I forced the include path to add %LIBRARY_PREFIX% in the include directories. It appears that the lame build places lame.h in the top-level of Library\include rather than in a subfolder (Library\include\lame) as expected by libsndfile's cmake: https://github.com/libsndfile/libsndfile/blob/ea3ac90e98c6a98cd52cae39010446fba368a2e3/cmake/FindLame.cmake#L13 . This is despite the script reporting to have found the header file in the build log: https://dev.azure.com/conda-forge/feedstock-builds/_build/results?buildId=578509&view=logs&j=a70f640f-cc53-5cd3-6cdc-236a1aa90802&t=f5d15007-a01c-5ad8-c9ce-4d519d3b275f&l=457

I'll continue experimenting with this...

[bmcfee]

PR https://github.com/conda-forge/lame-feedstock/pull/8 should address the include path issue, this is blocked until that merges.

[bmcfee]

Alright, I think this is all ready for a final look-over. (Sorry for the seemingly endless barrage of comments and commits, this one has been a real doozy!)

The windows build is now working with lame and mpg123 (→mp3 support in general), up to parity with the other platforms. After modifying the upstream lame package to put headers in the right spot, I was able to remove most of the weirdness from the libsndfile cmake build script. At this point, it looks pretty stock.

The only things that stick out to me as non-standard at this point have to do with the meta.yml as mentioned above. I've left comments in meta.yml which should be revisited when the next release (1.2.0) comes out, and we will probably be able to simplify this a little then.

I've also added myself in as a maintainer - against my better judgment :laughing: - having now spent enough time inside the guts of this one to have a sense of what's going on.

[wolfv]

Looks great, thanks for your hard work on this! I am happy to merge, but it would be slightly better to remove the fn: ... before.

[bmcfee]

Thanks @wolfv ! I've made the suggested changes, so I think this is good to go once CI clears.

[github-actions[bot]]

Hi! This is the friendly conda-forge automerge bot!

I considered the following status checks when analyzing this PR:

• linter: passed
• travis: passed
• azure: passed

Thus the PR was passing and merged! Have a great day!