Skip to content
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

Adding CMake build support (without unuseful commits) #37

Closed
wants to merge 3 commits into from

Conversation

sardylan
Copy link

@sardylan sardylan commented Mar 7, 2017

I added CMake support for building opus library keeping all the options and flags used on Autotools build system.
Current it support the generation of libopus.so (shared library) and libopusstatic.a (static library).
Some features (like document generation, demos and tests) are still to be implemented.
Tested only on GNU/Linux OS, using Kubuntu 16.10 64bit distribution.

@jmvalin
Copy link
Member

jmvalin commented Mar 11, 2017

Any particular reason you want CMake support? If we're going to maintain another build system there needs to be clear benefits. Also, it would have to be able to use the *.mk files rather than duplicate the list of files.

@cristianadam
Copy link

CMake is the de facto project generator in the C++ world. Even Visual Studio has support for it. This way instead of upgrading every two years the win32 Visual Studio solution, one could simply have a CMakeLists.txt file.

Unfortunately the style of the CMakeLists.txt is a bit outdated. Instead of add_definitions it is preferable to use target_compile_definitions, not to pollute the compile flags for other targets.

Instead of setting all those variable in the CMake cache one should use the option function.

Since I mentioned Visual Studio, this needs to work also on Windows. One can download a free Windows virtual machine from Microsoft Edge Tools department.

A good resource about Modern CMake and CMake resources in general can be found at Awesome CMake.

@sardylan
Copy link
Author

I started using opus as a component for a C application based on CMake as build system. Since this application had to work both in Linux and Windows (using mingw as cross-compiler), adding CMake support in Opus was the simple and clean solution for me, at the time.
After few month I needed Opus for an Android app, and CMake was used for embedded opus as native dependency.
I spent some times to create CMake build files and I think someone else may consider it useful, so I decided to create a pull request. If my CMake files became part of the official code, I'm sure that someone that know CMake better than me could manage a better integration.
CMake give a simple way to integrate the library in existing projects and (imo) it's a clean build system than autotools.
Besides, as cristianadam says, CMake became the standard C/C++ build tool for different platforms (this one alone may worth the time to convert Opus to CMake).

As for using *.mk files, I think that is not possible to directly integrate pieces of makefiles into the building system because CMake generates makefiles from scratch using its own style, but I may be wrong.
Perhaps there is a better way to have a single file list is converting *.mk files in a way which can be imported and used by both the building system.

@cristianadam thanks for the link about CMake resources.

@jmvalin
Copy link
Member

jmvalin commented Mar 14, 2017

The .mk issue is really important. Requiring a separate list is pretty much a guarantee that the CMake files will be broken more often than not.

@sardylan
Copy link
Author

sardylan commented Mar 20, 2017

The .mk issue is really important. Requiring a separate list is pretty much a guarantee that the CMake files will be broken more often than not.

You're right... I'll try to find a simple way to have a single list for both compilation systems.

@tusharpm
Copy link

I'm hoping this PR is still open for discussion. I'd like to use CMake bindings in opus.
I wrote a simple CMake module to read a .mk file and interpret each statement as CMake assignments. It has its limitations (like it understands only the simplest assignment statements; no ?=, +=, etc), but I checked this would be sufficient for the .mk files here.
@sardylan please consider trying it out once.

@sardylan
Copy link
Author

Hi tusharpm,
no problem... This PR is still open...
Thank you for your module... I'm very busy in this period, but I'll give it a try asap...

@Timmmm
Copy link

Timmmm commented Jun 21, 2017

Is there any reason to keep the Autotools build system once CMake works?

@rillian
Copy link
Contributor

rillian commented Jun 21, 2017

Afaict CMake still doesn't have an equivalent of autotools' make distcheck. Until that's available I think it's important to keep the autotools build.

If you don't like autotools, do you know about make -f Makefile.unix?

@Timmmm
Copy link

Timmmm commented Jun 22, 2017

Doesn't work on Windows does it?

You can implement make distcheck in CMake (example), but I'm not exactly sure why you'd need it.

@evpobr
Copy link
Contributor

evpobr commented Oct 21, 2017

Any particular reason you want CMake support? If we're going to maintain another build system there needs to be clear benefits. Also, it would have to be able to use the *.mk files rather than duplicate the list of files.

Portability. Not that portability across UNIX and family (~5%). Real portability. At least Linux, Windows and MacOS. MUCH more users, more bug reportings, more PRs, more help.

Usability. No restriction to use MinGW or Cygwin to build for Windows. Use what use want. Build scripts if you want scripts. Solution if you work with Visual Studio or with XCode.

Speed. CMake is more intelligent then Autotools. Better dependency tracking. With ninja build tool it is even faster.

You can implement make distcheck in CMake (example), but I'm not exactly sure why you'd need it.

It is not portable so forget about it.

Afaict CMake still doesn't have an equivalent of autotools' make distcheck.

If you mean one rule to do the same work it doesn't. But CMake does it in different way:

According to documentation about distcheck:

tries to do a VPATH build (see VPATH Builds), with the srcdir and all its content made read-only;

mkdir cmake-build
cd cmake-build
cmake ..
cmake --build .

CMake'a approach is to use out-of-source builds, when all generated files are in separate directory. I think it is the same as VPATH Build.

runs the test suite (with make check) on this fresh build;

ctest -V

installs the package in a temporary directory (with make install), and tries runs the test suite on the resulting installation (with make installcheck);

cpack -G"ZIP;TGZ;DEB;RPM;whatever"

checks that the package can be correctly uninstalled (by make uninstall) and cleaned (by make distclean);

No uninstall target. Never use install\uninstall, use packages for your plafrom. install is supported only to make cpack work.

distclean is rm cmake-build.

finally, makes another tarball to ensure the distribution is self-contained.

Compress folder and that's all if you use CMake.

More lines, yes, but you don't make distcheck every build i guess. You can do script if you really want.

Since this code is obsolete and stopped, i can write CMake build script for Opus. With @tusharpm module to parse mk file.

@berkus
Copy link

berkus commented Nov 24, 2017

Just use CMake and throw the makefile garbage out, easy solution.

Anyone could generate makefile, ninja, VS project or anything else if they need from cmake files.

@Timmmm
Copy link

Timmmm commented Nov 24, 2017

Have to agree with berkus. Here are the pros and cons I can see:

Stick with Autotools

Pros

  • Lazy option :-)
  • Works on obscure Unixes that nobody uses anymore
  • Supports make distcheck

Cons

  • Terrible support on Windows
  • Very slow and archaic
  • Difficult to understand build files
  • Doesn't integrate well with IDEs.

Ditch Autotools for CMake

Pros

  • Quite a lot more sane build files, though CMake still has "quirks".
  • Supports export to lots of different builders - make, ninja, Visual Studio, XCode, etc.
  • Much faster than Autotools
  • Decent built-in packaging and testing support (CPack, CTest)
  • Standard C/C++ build tool these days (or as close as we will get to one).

Cons

  • Still a bit insane
  • Not as fast as some build systems (e.g. QBS)
  • Doesn't support make distcheck natively.

There's also Meson, which is a CMake-like system that tries to fix some of its insanities:

Meson

Pros

  • Similar to CMake so easy transition / easy to learn if you know CMake
  • Fixes CMake's language weirdnessees
  • Does support make distcheck

Cons

  • New and less popular than CMake at the moment - worse IDE integration and more bugs (even ones in the parser).
  • Not Turing-complete. Could be a pro depending on your views.
  • Written in Python, with all the issues that brings.

Overall I think it is a clear win for CMake. The cons are minimal compared to those of autotools.

@berkus
Copy link

berkus commented Nov 24, 2017

I maintain a fork of opus repo with CMake support added, just because it makes it much easier to INTEGRATE into my projects.

I'd wish to update it to support opus-config.cmake (new style cmake infra), so it would be even easier to package it in something like conan.io (good luck setting that up with Makefiles).

@evpobr
Copy link
Contributor

evpobr commented Nov 24, 2017

@berkus, i have working CMake script with all autoconf tests implemented. It also reads sources from makefiles but it is really stupid IMHO. So it is only few steps to implement working CMake build system if maintrainers deside to drop their Makefiles.

Also look at Vcpkg, it is really nice package system based on CMake for Windows.

@zamazan4ik
Copy link

@evpobr I recommend you look into Conan - cross-platform package manager, not only for Windows.

@xnorpx
Copy link
Contributor

xnorpx commented Jan 4, 2018

+1 for CMake, would allow for much easier consumption for us as well and keep up to date.

@atkawa7
Copy link

atkawa7 commented Sep 8, 2018

@jmvalin ping ...

@jmvalin
Copy link
Member

jmvalin commented Sep 8, 2018

My earlier comment still stands. We're not adding yet another file list that's going to be outdated as soon as we add/remove any file. Any CMake support would need to use the existing .mk files and not require much maintenance.

@evpobr
Copy link
Contributor

evpobr commented Sep 8, 2018

My earlier comment still stands

You just don't understand what CMake is.

Just look how i work with libsndfile.

My platfom is Windows. I clone libsndfile from repository and i know it has CMake project.

I open directory in Visual Studio Code IDE and that's all - after configuration i can work - build, test and debug. No pain.

Now i want to test my changes on Linux. I start VM with Linux, clone repo and open directory in the same cross-platform Visual Studio Code. Different platform, different compiler, but everything works the same. No pain.

Windows and Visual Studio 2017? No problem. Latest VS2017 supports CMake natively - i can open libsndfile directory as folder. No pain.

Windows and outdated Visual Studio 2015 without CMake support? No problems. Generate Visual Studio solution and open it. No pain, again.

Do you like command prompt? Ok - generate makefile. No pain.

Do you like fast Ninja build tool script instead of outdated makefile? Generate it.

So, just like @berkus daid:

Just use CMake and throw the makefile garbage out, easy solution.

Anyone could generate makefile, ninja, VS project or anything else if they need from cmake files.

@atkawa7
Copy link

atkawa7 commented Sep 8, 2018

@evpobr I second your comment. I currently contribute to vcpkg and would love to have cmake version for opus which works for all systems i.e (Win, Mac, Linux). However with the autoconf you have to support multiple configurations. It could be made easy using cmake

@Timmmm
Copy link

Timmmm commented Sep 8, 2018

Thirded. CMake is as much as a standard C++ build system as we have, and although it is quite insane it is still much less insane than your existing solution.

I'd say laziness is the only reason not to change (and it's a perfectly valid reason) but somebody else has already done most of the work.

Perhaps you could enumerate what you fear having to do if Opus switches to CMake and ditches autotools? Is there some CI system that would require updating?

@rillian
Copy link
Contributor

rillian commented Sep 8, 2018

It's not just autotools we'd be abandoning (or maintaining in parallel). A number of large projects integrate opus into their own build systems based on the source file lists.

CMake is perfectly capable of parsing the *_sources.mk files and constructing the build description from there. People have written libraries for such things in other projects. Please add that to the PR, like @jmvalin requested, along with a ci description so we know when it breaks.

@atkawa7
Copy link

atkawa7 commented Sep 8, 2018

@rillian Thanks. However @jmvalin previous comment We're not adding yet another file list that's going to be outdated as soon as we add/remove any file made it seem like opus members were not considering @sardylan contribution. Even autoconf gets outdated when you change a header or something which recent commits can show

@jmvalin
Copy link
Member

jmvalin commented Sep 8, 2018

Look, if the purpose of this PR is to drop autotools support, then we can close it right now (try posting to LKML demanding the kernel only uses cmake from now on and see how far that goes). Now, if the goal is to add CMake, then I've made my conditions clear: it needs to use the existing .mk files and be low maintenance. I'm not going to be maintaining it and since contributors come and go, anyone who uses CMake should be able to maintain it. If these conditions can be met, then I'll have a look at including a cmake build. If not, then no I'm not going to add maintenance issues down the road for that feature.

@berkus
Copy link

berkus commented Sep 9, 2018

Well, i guess then the only option is to use own fork with cmake support and sometimes track upstream changes.

@berkus
Copy link

berkus commented Sep 9, 2018

@jmvalin the only maintenance issue for cmake is to add or remove a file from a simple list of source files. That's entirely all maintenance burden that you will need to do, provided you ever add any new files to the repo.

@berkus
Copy link

berkus commented Sep 9, 2018

@evpobr
Copy link
Contributor

evpobr commented Sep 9, 2018

@berkus, they forgot to add new header to Visual Studio project. So it is hard.

@berkus
Copy link

berkus commented Sep 9, 2018

@evpobr classic for projects that don't use cmake - they forget to update one of several build systems they use instead of a single-source-of-truth.

@jmvalin
Copy link
Member

jmvalin commented Oct 31, 2018

Doesn't look like there's any progress on this, so I'm closing this for now. Feel free to reopen should there be a cmake solution that can actually coexist with the autotools buils system without duplicating the list of files.

@jmvalin jmvalin closed this Oct 31, 2018
@Timmmm
Copy link

Timmmm commented Oct 31, 2018

What kind of progress were you expecting? Looks to me like it has been implemented and everyone agrees that it should be merged except you, but it's a little hard to tell why.

I suspect the reason is that you have never used CMake and can't be bothered to learn it or don't want to spend limited free time learning it? Honestly that is a totally reasonable reason but I think you should be honest about that so we don't waste time trying to convince you of something that you don't need to be convince of.

(That probably sounds really aggressive but I don't mean it to.)

@berkus
Copy link

berkus commented Oct 31, 2018

@Timmmm just help keep one of the forks with cmake up-to-date, this is useless.

@jmvalin
Copy link
Member

jmvalin commented Oct 31, 2018

@Timmmm I case you missed it from the 5 other times I mentioned it, I said I'd gladly add CMake support if it can work from the existing .mk files. Some comments suggest it's possible, but nobody has volunteered to implement that. One of the general principles here is to not break what people are using. That means I'm not going to break autotools and pure-make support. It also means I'm not going to break cmake support if I ever add it. From there, it means the only option I have based on these patches is to maintain yet another list of files and I'm not willing to add the burden of doing that forever, especially if another solution (reusing the .mk files) likely exists. Calling me an idiot for not dumping all the existing build infrastructure and switching to CMake is not going to help in any way.

@evpobr
Copy link
Contributor

evpobr commented Oct 31, 2018

@jmvalin , as i know parsing source file lists or globbing is bad practice. CMake cannot track properly dependencies in this case, if something is added or removed, It is bad idea.

Anyway, reusing .mk files doen't help you too much right now. I'm talking about Visual Studio solution. You need to track two build systems anyway. Even worse, you need Windows and Visual Studio just to add new file to it. And you need Visual Studio 2015 exactly. Or maybe Visual Studio 2017 with 2015 toolchain.

So why not replace ONLY Visual Studio solution with CMake project? Much easier to keep it updated. And you will get Visual Studio solutions from 6.0 to 2017 versions.

So, the only problem in this case is two source sets. Actually, it is not a big problem. You don't add source files each commit, right? If you compare current master branch vs v1.0.x tag, you will see only 2 files added, .gitattributes and update_version, both are not source files.

@Timmmm
Copy link

Timmmm commented Oct 31, 2018

I case you missed it from the 5 other times I mentioned it, I said I'd gladly add CMake support if it can work from the existing .mk files

I did not miss that. I missed the reason for this requirement.

That means I'm not going to break autotools and pure-make support.

You have not mentioned backwards compatibility for users as a reason before. This is a much more understandable reason than just "we have to keep autotools" - you might have gotten a bit less push-back if this was clear from the start.

Calling me an idiot

I did not call you an idiot.

@jmvalin
Copy link
Member

jmvalin commented Oct 31, 2018

@evpobr Don't get me wrong, I absolutely hate that extra list that goes with the Visual Studio solution. That's why I'm not going to repeat that mess. But since the VS files are in and people are using them, they're not going away. FTR, they also do get broken regularly when things change.

@xnorpx
Copy link
Contributor

xnorpx commented Nov 1, 2018

Added #100

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet