ping @jmvalin @berkus

I don't find clutches like this reasonable, but if @jmvalin likes it - good for him.

As far as I understand this will break change tracking for cmake, making it either always regenerate the build system (good luck if you use MSVC or Xcode - msvc just crashes when studio project is re-generated from underneath it on rebuild) or miss some changes in the file lists.

Even though file lists are not gonna change often, the former problem of MSVC crashing is much more severe.

Agree, not ideal but probably worth the tradeoff in this case.

I will try the scenario you describe in MSVC and if it is indeed crashing that should be fixed by that team.

@xnorpx , look here, i've implemented some options and configure tests.

@evpobr Thanks! once I get a initial like from @jmvalin I will add the options.

So this at least looks sane from my point of view at least (not knowing anything about cmake). I had a few questions though?

• Why does the patch rely on Makefile.unix?
• Is there a way to avoid explicitly naming all the SOURCES_SSE* sets of files?
• Any way to avoid having to name all the test_* programs (easy to forget to add one)?
  None of the issues I'm raising above is fatal, but it would reduce the maintenance burden (and likelihood of messing up) if they can be addressed.

In any case, this is looking like something I should be able to merge (assuming it works well and all, which I haven't tested yet).

As far as I understand this will break change tracking for cmake, making it either always regenerate the build system (good luck if you use MSVC or Xcode - msvc just crashes when studio project is re-generated from underneath it on rebuild) or miss some changes in the file lists.

@berkus I assume that by "break change tracking" you mean that things will break if someone changes the file list, as opposed to never properly rebuilding only the files that change, right? If that's the case, then I guess it's acceptable. If incremental builds is completely broken, then it's probably not acceptable.

"Why does the patch rely on Makefile.unix?"
Missed to remove it, will clean up.

"Is there a way to avoid explicitly naming all the SOURCES_SSE* sets of files?"
Essentially we need a "set" of files for each target, for the component tests it seems good enough. For the smaller tests you have spread out in the codebase I am not sure if they can be merged into 1 binary. But I didn't spend time on it as the test was mostly a way to validate the build for now. I will take a closer look on the test portion.

I will add @evpobr changes and see if I can clean up the test section without touching the make files, also make sure it builds on Mac

@jmvalin the unittests is not ideal for this situation. As they all have mains and need to built into separate binaries I cannot really think of a nice solution without refactor the structure of the test and merge them into one binary. Also create a test_sources.mk to keep the file lists there. But probably out of scope for this change.

I assume that by "break change tracking" you mean that things will break if someone changes the file list

@jvmalin as far as i know, cmake will take a conservative approach in case of using globs and will fully regenerate build system on each triggered build. While correctly generated cmake does this only when configuration changes.

That's an "acceptable" trade off here, as i understand.

@xnorpx it usually helps to build shared part of code as library and then link that into each test, which is essentially a main() in that case.

You can make an OBJECT library so it won't even make any real libraries in this case, just group files together.

JFYI http://lists.qt-project.org/pipermail/development/2018-October/034023.html

@jmvalin what is that requirement that doesn't allow you to drop Makefile.am and all this ancient crud? If it's some VMS system, then I'm sure there's a cmake build for it too.

Ah, found it, "keep what was working". Nvm.

I assume that by "break change tracking" you mean that things will break if someone changes the file list

@jvmalin as far as i know, cmake will take a conservative approach in case of using globs and will fully regenerate build system on each triggered build. While correctly generated cmake does this only when configuration changes.

That's an "acceptable" trade off here, as i understand.

Still trying to understand what you mean exactly here, so let me describe two scenarios:

1. Download Opus, build with cmake, then change one line in some source file, build again
2. Download Opus, build with cmake, add a new source file to one of the .mk files, build again

Will the "build again" step for 1) recompile everything, or just the file I changed? Will the build in 2) proceed properly? Will it recompile everything or just the file I added.

@jmvalin

1. It will do incremental build just fine.
2. CMake will not regenerate the solution automagically as it doesn't see any changes so the user that add/remove a file needs to do either:

• Delete solution and regenerate solution to pick up the new file or remove the file
• Make a change in CMakelists.txt to get CMake to pick the new changes

But this is just for the local development when using CMake, I see this as a small price to pay to get initial CMake support for Opus

@berkus VS2017 V.15.8.5 did not crash for me when git rm a file from the repo. The file only had a red mark in the solution and could of course not be opened.

@jmvalin

1. It will do incremental build just fine.

2. CMake will not regenerate the solution automagically as it doesn't see any changes so the user that add/remove a file needs to do either:


* Delete solution and regenerate solution to pick up the new file or remove the file

* Make a change in CMakelists.txt to get CMake to pick the new changes

But this is just for the local development when using CMake, I see this as a small price to pay to get initial CMake support for Opus

OK, sounds reasonable (just wanted to make sure that 1 works fine).

VS2017 V.15.8.5 did not crash for me when git rm a file from the repo. The file only had a red mark in the solution and could of course not be opened.

@xnorpx what usually happens is this:

• cmake sees that there's a glob used
• it then forces a regeneration of VS solution when you press Build
• build step re-generates the solution file from within VS
• VS goes crazy and crashes

If MS fixed this, then excellent and full-speed ahead.

And since you just parse a file, without globs, then the only dependency should be these makefile.am files changing - so try changing it in VS and press Build.

I'm having a hard time tracking down all the commits in this pull request (and honestly I don't actually use github much since this is just a mirror), so it would help to see this as a single commit. When it's ready, it would be good to post it on the list for review.

@jmvalin yeah sorry for the spam, let me switch to another dev branch for my last changes, once ready I squash, clean up, force update this branch and ping this thread. If you want to avoid having open pull request you can close it and I reopen a new one once ready.

Wait, how did this turn into Windows-only thing?? CMake is by definition cross-platform, adding it only for Windows makes no sense.

@jmvalin , not sure i understand the MAY_HAVE and PRESUME defines do, MAY_HAVE enables autodetection of SIMD sets? PRESUME builds for exact SIMD set?

@berkus I was thinking first iteration Windows only as that I can test easiest. Then add Mac and Linux on top of that.

@evpobr Yeah the SIMD implementation is not clear to me and is were I am now.

@jmvalin Regarding libs, should opus be shipped as one "opus.lib" or should it be split up with silk/opus/celt libraries?

@evpobr MAY_HAVE means "the CPU may have that instruction set, but may not have it, so check at run-time", whereas PRESUME means "the CPU definitely has that instruction set, and no need to check at run-time"

@xnorpx Opus should build as a single opus.lib library. No need to split into celt/silk (it used to be that way, but only for historical reasons)

No.

@xnorpx , please don't reformat. Damn hard to see important changes. It looks like all the files were changed.

@evpobr yes sorry about that, itchy finger and didn't realize that I didn't commit my previous changes. Won't do it until final rebase now.

ping @evpobr

I've got linker errors. Can you test clean build?

[build]    Creating library opus.lib and object opus.exp
[build] encode_frame_FIX.c.obj : error LNK2001: unresolved external symbol _SILK_VAD_GETSA_Q8_IMPL
[build] find_LPC_FIX.c.obj : error LNK2001: unresolved external symbol _SILK_BURG_MODIFIED_IMPL
[build] burg_modified_FIX.c.obj : error LNK2001: unresolved external symbol _SILK_INNER_PROD16_ALIGNED_64_IMPL
[build] burg_modified_FIX_sse4_1.c.obj : error LNK2001: unresolved external symbol _SILK_INNER_PROD16_ALIGNED_64_IMPL
[build] opus.dll : fatal error LNK1120: 3 unresolved externals
[build] ninja: build stopped: subcommand failed.
[build] Build finished with exit code 1

Many warnings in x86 mode:

[build] cl : Command line warning D9025 : overriding '/arch:SSE' with '/arch:SSE2'
[build] cl : Command line warning D9025 : overriding '/arch:SSE2' with '/arch:AVX'

I suspect MAY_HAVE and PRESUME must be both set, you set only one of them.

@evpobr I need to do some more testing, but regarding current state is it good to go?

@evpobr I need to do some more testing, but regarding current state is it good to go?

I think it is good.

What's up with merging it then?

Considering that these patches don't really change existing files (unless I overlooked something), I think it's fine to all collapse them into a single patch. Can you do that and then post the result on the mailing list for review? I'd rather have that review on the list (more people paying attention -- including me) than on github.

I managed to subscribe to the mailing list now, I guess spam filter killed the confirm email on work email. Once I verified builds I will rebase, give kudos to @evpobr in commit and create patch.

The patch is on the mailing list so should be good for merge in a couple of days unless any other feedback comes in.

The patch is on the mailing list so should be good for merge in a couple of days unless any other feedback comes in.

Great work @xnorpx. If we find bug later we will fix it.

@jmvalin Thanks! Consider adding a CMake build on any platform on any buildsystem to sanity check the CMake integration.

Yay, gratz! Welcome to 21st century, opus.

Here is pull request to fix Opus in VCPKG: microsoft/vcpkg#6007 which will fix MAC/Linux and uwp-arm builds.

Any thoughts about the VS2015 files in the opus/win32 directory? Are those eventually going to become useless in the longer term so they can be removed? I'm still leaving them for now, but it'd be nice if they could go away at some point.

@jmvalin I would remove it asap, VS2019 was release this week. Change the Appveyor CI step to use CMake and then remove the win32 folder.

Nice to have would be to have static/dynamic builds for Windows/Mac/Linux and Android running on pull requests.

Well, I'm not removing anything just yet -- at least not before people have had time to use the cmake setup and discover any issues it may have compared to the build files they've been using. The good news is that I'm planning on releasing 1.3.1 tomorrow or Friday, so we'll have the cmake stuff in the hands of users very soon. The only thing I'd like to fix at this point is defaulting to release builds (see email on the list).