-
Notifications
You must be signed in to change notification settings - Fork 594
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add CMake support #100
Add CMake support #100
Conversation
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. |
So this at least looks sane from my point of view at least (not knowing anything about cmake). I had a few questions though?
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). |
@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?" "Is there a way to avoid explicitly naming all the SOURCES_SSE* sets of files?" 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When/if you look at installation, it would be great if you have options to avoid building/installing any executables or samples (cross-compilation scenarios).
@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. |
@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. |
Still trying to understand what you mean exactly here, so let me describe two scenarios:
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. |
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. |
OK, sounds reasonable (just wanted to make sure that 1 works fine). |
@xnorpx what usually happens is this:
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 |
@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?
|
Many warnings in x86 mode:
|
I suspect |
@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. |
00075b4
to
a1d1806
Compare
Co-Authored-By: evpobr
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). |
Yet another proposal for cmake
Update
Included:
Known things missing:
How to run:
2 mkdir build
-- Install configuration: "Debug"
-- Installing: C:/Program Files (x86)/opus/lib/opus.lib
-- Installing: C:/Program Files (x86)/opus/lib/opus.dll
-- Installing: C:/Program Files (x86)/opus/lib/opus_static.lib
-- Up-to-date: C:/Program Files (x86)/opus/include/opus
-- Up-to-date: C:/Program Files (x86)/opus/include/opus/opus.h
-- Up-to-date: C:/Program Files (x86)/opus/include/opus/opus_custom.h
-- Up-to-date: C:/Program Files (x86)/opus/include/opus/opus_defines.h
-- Up-to-date: C:/Program Files (x86)/opus/include/opus/opus_multistream.h
-- Up-to-date: C:/Program Files (x86)/opus/include/opus/opus_projection.h
-- Up-to-date: C:/Program Files (x86)/opus/include/opus/opus_types.h
Install the project...
-- Install configuration: ""
-- Installing: /usr/local/lib/libopus.so
-- Installing: /usr/local/lib/libopus.a
-- Up-to-date: /usr/local/include/opus
-- Up-to-date: /usr/local/include/opus/opus.h
-- Up-to-date: /usr/local/include/opus/opus_custom.h
-- Up-to-date: /usr/local/include/opus/opus_defines.h
-- Up-to-date: /usr/local/include/opus/opus_multistream.h
-- Up-to-date: /usr/local/include/opus/opus_projection.h
-- Up-to-date: /usr/local/include/opus/opus_types.h