opusfile icon indicating copy to clipboard operation
opusfile copied to clipboard

cmake issues

Open sezero opened this issue 2 years ago • 5 comments

In SDL_mixer, we submodule'd opus and opusfile through our forks and attempted cmake build relying on the submodules. Issues we had:

  • The git tree, obviously, is missing a package_version file. We added to both opus and opusfile. However:

  • If everything fails opusfile cmake'ry sets OPUSFILE_PROJECT_VERSION to 0, as in integer: https://github.com/xiph/opusfile/blob/master/cmake/OpusFilePackageVersion.cmake#L63 ... But the caller expects a version instead, and fails: https://github.com/xiph/opusfile/blob/master/CMakeLists.txt#L6-L9 Should you change the fallback OPUSFILE_PROJECT_VERSION to 0.0 instead? (Same is true in Opus project.)

  • Opusfile cmake'ry doesn't look for package_version file if it finds git, because the package_version case is an elseif case which seems wrong: https://github.com/xiph/opusfile/blob/master/cmake/OpusFilePackageVersion.cmake#L31 Finalizing the git case with an endif changing package_version case to an if fixes things for us.

  • Opus: Going from v1.3.1 to git master, the issue described above is present in opus too: https://github.com/xiph/opus/blob/master/cmake/OpusPackageVersion.cmake#L31 The 1.3.1 release was good: https://github.com/xiph/opus/blob/e85ed7726db5d677c9c0677298ea0cb9c65bdd23/opus_functions.cmake#L45 (Note that get_package_version used to return only if git case succeeded.) So the same suggestion as in the opusfile case above.

sezero avatar May 23 '22 18:05 sezero

Any comments please?

sezero avatar May 28 '22 09:05 sezero

PING - anyone?

sezero avatar Jun 18 '22 17:06 sezero

@sezero Hi, I am not so good with all the version stuff. But can you check if this change would fix the issues you have for opus?

https://github.com/xnorpx/opus/commit/093b8dab25d5dc0ac25d7e4a47d03c6353e6cf79

xnorpx avatar Jul 06 '22 18:07 xnorpx

@madebr: cmake is your language, can you have a look? Reference issue: https://github.com/libsdl-org/SDL_mixer/pull/386#issuecomment-1133771043 Related commits: https://github.com/libsdl-org/opusfile/commit/264fc0df35a12569554d2a716d674d59ff3c86f1 https://github.com/libsdl-org/opusfile/commit/9ba5cc3dc920fb98f1b4068888d3c4e8617ef9b6

sezero avatar Jul 06 '22 19:07 sezero

I like @sezero's idea of using "if" instead of "else if". That way if decoding the package_version fails, it tries again from git. Like this:

if(NOT OPUS_PACKAGE_VERSION){ try getting it from package_version file }
if(NOT OPUS_PACKAGE_VERSION){ try getting it from git }
if(NOT OPUS_PACKAGE_VERSION){ print warning and set to 0.0 }

Side note, I think it would be useful to keep a basic "version" file maintained in-repo so we don't need to fallback to 0.0. (e.g. ogg keeps the version in the configure.ac file in the repo, but opus/opusfile currently do not.) In that case, we'd still need to get the commit id from git if available (and only if the git tag matches the version file). @mark4o thoughts?

daddesio avatar Jul 06 '22 20:07 daddesio