Add CMakeLists.txt #25

h3ndrk · 2021-05-28T17:01:12Z

This PR adds CMake support for opusfile and opusurl. autoconf is untouched.

Fixes #15

h3ndrk · 2021-05-29T16:50:39Z

This PR is ready for review. Some questions to the authors:

Currently the version is hard-coded in the CMakeLists.txt. How is your normal workflow for embedding it into the library and distribution files etc.? I only had a brief look at the many scripts that somehow mutate and propagate the version. It seems very complex tooling for the job... Maybe there is a simple way that I don't see currently?
Do we want to also build the documentation with CMake? There is also another problem with it, see Documentation #21

Thanks!

rillian · 2021-05-29T18:20:16Z

I only had a brief look at the many scripts that somehow mutate and propagate the version. It seems very complex tooling for the job...

Yes, this is pretty complicated. The requirements are:

When building from a repository, the software version should be derived from git describe --tags for clarity and correctness.
When building from a release source package, where no revision history is available, the version should be set statically.
Version info needs to be propagated to:
- config.h for use in the library
- pkg-config files describing the library
- source package naming
- documentation

That way we get something like 0.12 for a release but 0.12-6-g641bf7e the current development tree.

There's a unix shell script update_version which queries git and writes the result to a file package_version which is read by various build systems. This file isn't tracked by git, but is included in release source packages. Together that covers the static and dynamic version requirements.

Now, opusfile library code doesn't currently use the PACKAGE_VERSION define, cmake doesn't create pkg-config files or source packages, so you'd only need this for documentation, if you add support for building that (which would be nice). However, opusfile might use the version string at some point in the future, so a cmake build should support it.

You can't call the unix shell script on Windows. I suggest copying cmake/OpusPackageVersion.cmake from the opus repo, which re-implements the script in cmake.

rillian

I'd like to see the version TODO addressed, and some ci converage before this lands. Building the documentation would be nice, but can also be a separate PR. Otherwise I looks fine to me.

@evpobr, @xnorpx do you have any comments?

xnorpx · 2021-05-29T21:39:37Z

I can take a look next week.

Do you need cmake_minimum_required(VERSION 3.16) ?

(people with old linux distros always complain with high cmake versions :) )

h3ndrk · 2021-05-31T15:33:17Z

Do you need cmake_minimum_required(VERSION 3.16) ?

(people with old linux distros always complain with high cmake versions :) )

I've only tested with CMake 3.16 (officially available on Ubuntu 20.04) and I know at least one feature (timestamping of Doxygen) which requires 3.16. But compatibility vs. modern features is always a hard question... In the book Professional CMake, the author says that it is preferred to not limit the project maintainers in functionality caused by old CMake versions but rather require users to download a more recent version of CMake themselves (which is very easy).

I'd like to see the version TODO addressed

Done, version is retrieved via a CMake module taken from opus.

Building the documentation would be nice

Done with changing behavior: The output paths change because I would like to use the official FindDoxygen.cmake module which makes integrating Doxygen very easy in CMake. The module generates a Doxyfile automatically and outputs the documentation into a subdirectory in the build directory which means the Doxygen.in file is obsolete and documentation is generated not in the doc/ directory.

some ci converage before this lands

Currently, there is no CMake find module for opus and ogg which means that the CMakeLists.txt of opusfile will only work with opus and ogg installed with CMake configs instead of legacy pkgconfig. Not installing CMake configs is quite common in most distributions sadly (the Travis CI also has this problem). I will try to come up with some find modules relying on CMake configs and pkgconfig as a fallback. I've no experience how opusfile is built on Windows and how it finds its dependencies there. I will add some CI steps after the find modules are done.

h3ndrk · 2021-05-31T19:14:21Z

Find modules added
Travis CI passing
The same commands have been added to GitLab CI

rillian · 2021-05-31T21:17:00Z

.gitlab-ci.yml

+      apt-get install -y libopus-dev libogg-dev libssl-dev
+        doxygen
+  script:
+    - wget https://cmake.org/files/v3.16/cmake-3.16.9-Linux-x86_64.tar.gz


Thanks for adding a CI config. That's nice to see, and these steps are green on our gitlab.

Pulling a binary like this is unfortunate. It makes the build description specific to x86_64-linux, so at the very least you need an arch-specific tag.

The gcc:9 container we use is based on debian:10 which has cmake 3.13.3, and is used by all our other gitlab ci jobs. Is that also too old? See, I'm already complaining about the aggressive cmake version. :)

There's a kitware/cmake container feed on docker hub (just as huge as the gcc images!) but it seems to just be their internal ci runners, rather than something for external use. At least, I couldn't make sense of the versioning for that.

I'm not sure what you mean with arch-specific tag. Could you elaborate on this?

I also found rikorose/gcc-cmake, but it is third-party and I'm not sure if you want to rely on that.

Regarding the CMake version, see my comment below.

rillian · 2021-05-31T21:27:19Z

.travis.yml

  homebrew:
    brewfile: true

 env: PKG_CONFIG_PATH=$PKG_CONFIG_PATH:/usr/local/opt/openssl/lib/pkgconfig

+before_script:
+  - if [ "$TRAVIS_OS_NAME" = "linux" ]; then
+      wget https://cmake.org/files/v3.16/cmake-3.16.9-Linux-x86_64.tar.gz;


You can use curl -O here to avoid having to install wget.

evpobr · 2021-06-01T04:46:12Z

CMakeLists.txt

@@ -52,6 +56,7 @@ target_link_libraries(opusfile
  PUBLIC
    Ogg::ogg
    Opus::opus
+    $<$<BOOL:OP_HAVE_LIBM>:m>


$<$<BOOL:${OP_HAVE_LIBM}>:m>

evpobr · 2021-06-01T04:57:11Z

I've only tested with CMake 3.16 (officially available on Ubuntu 20.04) and I know at least one feature (timestamping of Doxygen) which requires 3.16. But compatibility vs. modern features is always a hard question... In the book Professional CMake, the author says that it is preferred to not limit the project maintainers in functionality caused by old CMake versions but rather require users to download a more recent version of CMake themselves (which is very easy).

In theory it is correct. In practice version it is too high for some Linux distros. It's surprising to me that someone is using such old distros, but i have encountered similar reports on a regular basis.

I could recommend to use more or less safe version (e.g. 3.6) and enable features depending on CMAKE_VERSION checks. You can display message if some feature is disabled because CMake version is too low.

evpobr · 2021-06-01T05:04:26Z

CMakeLists.txt

+  include(CheckCSourceCompiles)
+  cmake_push_check_state(RESET)
+  if(WIN32)
+    check_include_file("winsock2.h" OP_HAVE_WINSOCK2_H)


winsock2 is from Win95 times, not sure it is required to check it today.

I tried to do a nearly exact port of the autoconf/autotools scripts. Since I don't have access to Windows please say more directly how we should proceed here. Should I just remove the check?

xnorpx · 2021-06-01T06:01:01Z

CMakeLists.txt

+    $<$<C_COMPILER_ID:MSVC>:/wd4267>
+    $<$<C_COMPILER_ID:MSVC>:/wd4244>
+    $<$<C_COMPILER_ID:MSVC>:/wd4090>
+    $<$<C_COMPILER_ID:Clang,GNU>:-std=c89>


use https://cmake.org/cmake/help/v3.1/prop_tgt/C_STANDARD.html ?

I'd propose to just omit that compiler flag because C_STANDARD does not seem to support any value for C89. Maybe that is already default?

h3ndrk · 2021-06-04T19:06:35Z

I could recommend to use more or less safe version (e.g. 3.6) and enable features depending on CMAKE_VERSION checks. You can display message if some feature is disabled because CMake version is too low.

Testing and back-porting the CMake scripts is a lot of effort because

... there are things that don't exist in older versions. I don't have much motivation to re-implement things that already exist in newer versions.
... back-porting can lead to nasty bugs in CMake without helpful error messages. See https://alexreinking.com/blog/how-to-use-cmake-without-the-agonizing-pain-part-1.html (section "Validating Your Minimum Version").
... manually figuring out what features are needed from newer versions and implementing reasonable fallbacks is complicated and error-prone.

These points are also argued by CMake experts, saying that developers of CMake build systems should not try to support very old versions but rather require the version that they have developed on.

Because of these reasons I would like to keep cmake_minimum_required(VERSION 3.16).

petterreinholdtsen · 2021-06-04T19:35:07Z

While I agree that it is good to have conservative build dependencies, to help those that do not have luxury of trying the latest and less tested versions of software, I thought it might be useful with a data point on cmake versions in Debian. From <URL: https://tracker.debian.org/pkg/cmake > it can be seen that oldstable (9) is the version of Debian with cmake < 3.16. Stable and newer have =< 3.16. Is it known which distributions and versions will be locked out by depending on 3.16?

…

-- Happy hacking Petter Reinholdtsen

h3ndrk · 2021-06-04T19:51:26Z

Maybe have a look at https://pkgs.org/download/cmake or https://gitlab.kitware.com/cmake/community/-/wikis/CMake-Versions-on-Linux-Distros.

evpobr · 2021-06-05T07:23:03Z

https://repology.org/project/cmake/versions

Be-ing · 2021-06-06T03:52:06Z

Thanks for working on this! It will make this library easier to use, especially on Windows.

h3ndrk · 2021-06-28T12:41:18Z

Ping

evpobr · 2021-06-28T15:23:19Z

CMakeLists.txt

+  LIBRARY DESTINATION "${CMAKE_INSTALL_LIBDIR}"
+  ARCHIVE DESTINATION "${CMAKE_INSTALL_LIBDIR}"
+  INCLUDES DESTINATION "${CMAKE_INSTALL_INCLUDEDIR}"
+  PUBLIC_HEADER DESTINATION "${CMAKE_INSTALL_INCLUDEDIR}"


Should it be ${CMAKE_INSTALL_INCLUDEDIR}/opus?

https://packages.debian.org/sid/amd64/libopusfile-dev/filelist

Is it correct that we install in /usr/include/opus instead of /usr/include/opusfile?

I guess so:

opusfile/Makefile.am

Line 7 in 5672864

opusincludedir = ${includedir}/opus

And public include directory should be ${CMAKE_INSTALL_INCLUDEDIR}/include/opus:

opusfile/examples/opusfile_example.c

Line 25 in 5672864

#include <opusfile.h>

Done. You wrote ${CMAKE_INSTALL_INCLUDEDIR}/include/opus, I'am putting it in ${CMAKE_INSTALL_INCLUDEDIR}/opus. I hope this is fine.

Yes, you're right.

Yohannfra · 2022-01-14T18:05:08Z

Hi, any news for this PR ?

rillian · 2022-01-15T19:14:35Z

Merged upstream in cf218fb. Thanks!

h3ndrk added 5 commits May 28, 2021 18:55

Add CMakeLists.txt

5f2fd24

More improvements in CMakeLists.txt

9a28612

Add build directory to .gitignore

03a7f77

Remove some TODOs from CMakeLists.txt

e4a85b9

Include targets from all configurations

112b79b

h3ndrk marked this pull request as ready for review May 29, 2021 16:44

rillian requested changes May 29, 2021

View reviewed changes

h3ndrk added 2 commits May 31, 2021 15:56

Dynamic versions

8ca407f

Build documentation with CMake

8ce01f9

h3ndrk force-pushed the cmake branch from e3ac6da to 8ce01f9 Compare May 31, 2021 14:57

h3ndrk added 9 commits May 31, 2021 19:49

Add find modules, minor fixes to include paths etc.

9f0518b

Add CMake building to Travis CI and GitLab CI

a57da31

Add math library (fixes static linking)

83d94dc

Remove verbose output of tar extracting

f1a0ee4

Use CMake 3.16 in CI, fix OSX CI

950b73c

Add semicoli

499b65a

Export current directory in PATH

4d5e04c

Make imported pkgconfig targets global

2847e20

Install doxygen in Brewfile

beab650

Add SOVERSION, fix target name

2d317b4

rillian reviewed May 31, 2021

View reviewed changes

evpobr reviewed Jun 1, 2021

View reviewed changes

xnorpx reviewed Jun 1, 2021

View reviewed changes

Make pkgconfig targets globally imported targets

3a10e81

h3ndrk mentioned this pull request Jun 4, 2021

Prepare Yocto for CMake HULKs/meta-hulks#1

Merged

h3ndrk added 2 commits June 4, 2021 20:33

Use curl -O instead of wget

b0903d7

Fix BOOL generator expression

6cceb89

Do not check for winsock2.h

b94ce6a

h3ndrk added 4 commits June 6, 2021 08:31

Generate config file

d1b2ee8

Move opusfileConfig.cmake.in

7b09bf5

Add find_dependency(OpenSSL) to config script

996f5ae

Change case of namespace

30c375f

evpobr suggested changes Jun 28, 2021

View reviewed changes

Put opusfile.h into subdirectory

85b5a29

h3ndrk mentioned this pull request Jul 4, 2021

CMake rewrite + feature completion using modern CMake best practices PortAudio/portaudio#461

Merged

evpobr approved these changes Jan 15, 2022

View reviewed changes

rillian approved these changes Jan 15, 2022

View reviewed changes

rillian closed this Jan 15, 2022

h3ndrk deleted the cmake branch January 15, 2022 19:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add CMakeLists.txt #25

Add CMakeLists.txt #25

h3ndrk commented May 28, 2021 •

edited

Loading

h3ndrk commented May 29, 2021

rillian commented May 29, 2021

rillian left a comment

xnorpx commented May 29, 2021

h3ndrk commented May 31, 2021

h3ndrk commented May 31, 2021 •

edited

Loading

rillian May 31, 2021

h3ndrk Jun 4, 2021

rillian May 31, 2021

h3ndrk Jun 4, 2021

evpobr Jun 1, 2021

h3ndrk Jun 4, 2021

evpobr commented Jun 1, 2021

evpobr Jun 1, 2021

h3ndrk Jun 4, 2021

evpobr Jun 5, 2021

h3ndrk Jun 5, 2021

xnorpx Jun 1, 2021

h3ndrk Jun 4, 2021

h3ndrk commented Jun 4, 2021

petterreinholdtsen commented Jun 4, 2021 via email

h3ndrk commented Jun 4, 2021 •

edited

Loading

evpobr commented Jun 5, 2021

Be-ing commented Jun 6, 2021

h3ndrk commented Jun 28, 2021

evpobr Jun 28, 2021

h3ndrk Jul 2, 2021

evpobr Jul 2, 2021

h3ndrk Jul 2, 2021 •

edited

Loading

evpobr Jul 2, 2021

Yohannfra commented Jan 14, 2022

rillian commented Jan 15, 2022

Add CMakeLists.txt #25

Add CMakeLists.txt #25

Conversation

h3ndrk commented May 28, 2021 • edited Loading

h3ndrk commented May 29, 2021

rillian commented May 29, 2021

rillian left a comment

Choose a reason for hiding this comment

xnorpx commented May 29, 2021

h3ndrk commented May 31, 2021

h3ndrk commented May 31, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evpobr commented Jun 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

h3ndrk commented Jun 4, 2021

petterreinholdtsen commented Jun 4, 2021 via email

h3ndrk commented Jun 4, 2021 • edited Loading

evpobr commented Jun 5, 2021

Be-ing commented Jun 6, 2021

h3ndrk commented Jun 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

h3ndrk Jul 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yohannfra commented Jan 14, 2022

rillian commented Jan 15, 2022

h3ndrk commented May 28, 2021 •

edited

Loading

h3ndrk commented May 31, 2021 •

edited

Loading

h3ndrk commented Jun 4, 2021 •

edited

Loading

h3ndrk Jul 2, 2021 •

edited

Loading