(Almost) complete CMake support added #97

krokoziabla · 2019-04-03T09:17:41Z

I examined the contents of configure.ac and Makefile.am's and *.vcxproj's and wrote a suite of CMakeLists.txt according to them. Everybody is welcome to try it out and suggest any comments or ideas on how to improve it.

Tested on Windows (MSVC, cygwin+gcc), on Mac (Xcode+clang, make+clang), on Linux (Ubuntu, Arch, Gentoo, make or ninja + clang or gcc), FreeBSD (clang+make)

Things to consider:

OGG support will function after PR Cmake updates ogg#40 will have been merged
XMMS plugin doern't seem to build even by autotools (it cannot find glib.h header out of the box) so I didn't try hard to make CMake build it either
I don't have a PPC machine so PPC features wasn't added/tested
Tests in /test folder could be converted to C/C++ in order to become platform independent and integrated to CMake's test framework. At the moment only /src/test_libFLAC and /src/test_libFLAC++ are there and can run on any flatform

Fixes #43

erikd · 2019-04-03T22:23:26Z

The XMMS plugin support is of very low priority.

PPC support is also low priority. I try to keep it working and if it gets broken I expect someone with a PPC machine to help fix it.

Tests in /test folder could be converted to C/C++ in order to become platform independent

I have looked at that, and unfortunately it looks like a huge task. Not sure how the tests are currently built and run on Windows with the VS project.

lvqcl · 2019-04-04T15:49:32Z

Not sure how the tests are currently built and run on Windows with the VS project.

built: VS builds the following .exe files (along with flac.exe and metaflac.exe, of course):
test_cuesheet.exe
test_libFLAC++.exe
test_libFLAC.exe
test_picture.exe
test_seeking.exe
test_streams.exe

run: they don't, AFAICS

krokoziabla · 2019-04-06T09:00:08Z

Well, the scripts in /test folder still can be used on Unix since CMake builds all the binaries which those scripts reference

erikd · 2019-04-06T23:09:53Z

Just ran (from the .travis.yaml file from libsndfile):

mkdir cmake-build && \
cd cmake-build && \
cmake .. -DCMAKE_BUILD_TYPE=Release -DCMAKE_C_FLAGS="-Wall -Wextra" &&\
make VERBOSE=1 && \
ctest -V

and got:

CMake Error at src/CMakeLists.txt:8 (find_package):
  By not providing "FindOGG.cmake" in CMAKE_MODULE_PATH this project has
  asked CMake to find a package configuration file provided by "OGG", but
  CMake did not find one.

  Could not find a package configuration file provided by "OGG" with any of
  the following names:

    OGGConfig.cmake
    ogg-config.cmake

  Add the installation prefix of "OGG" to CMAKE_PREFIX_PATH or set "OGG_DIR"
  to a directory containing one of the above files.  If "OGG" provides a
  separate development package or SDK, be sure it has been installed.

krokoziabla · 2019-04-07T05:29:30Z

@erikd You need either to add -DWITH_OGG=OFF flag or to build xiph/ogg#40 and install it somewhere and then add -DOGG_DIR= (-DOGG_DIR is needed only if OGG doesn't get into standard installation paths on UNIX)

BTW, -Wall -Wextra are enabled by default, as well as the others from configure.ac

erikd · 2019-04-07T06:25:17Z

@krokoziabla I have the ogg development libraries installed in the standard location. The Cmake build systems should be able to find them (like it does for libsndfile). The -DWITH_OGG parameter should only be needed to override the default or to provide a alternative location.

erikd · 2019-04-07T10:38:19Z

Ok, now it can find the Ogg libraries, but the full test suite is not being run. The full test suite takes about 30 minutes.

krokoziabla · 2019-04-07T14:31:23Z

Oh, sorry, I should have used another temporary branch so that you didn't see my temporary commits. I needed some time to make my fix stable on all the platforms I have. Now it is ready

but the full test suite is not being run. The full test suite takes about 30 minutes.

Not sure I get your point. make test now only runs test_libFLAC and test_libFLAC++ since successfulness of these tests is determined by their exit codes and that is what CTest needs.

The other tests still can be run but for that one needs to run something like this:
cd /path/to/flac/build/tests && for script in $(ls /path/to/flac/sources/tests/*.sh); do $script; done.
That is why I said about converting the tests to C/C++ so that one could just run make test

erikd · 2019-04-07T20:42:17Z

Ok, let me know when you have this sorted.

krokoziabla · 2019-04-08T01:23:14Z

Do you mean you suggest converting tests to C/C++ or what? FindOGG.cmake seems to work well now, I don't have anything else to do with it.

erikd · 2019-04-08T01:43:27Z

Do you mean you suggest converting tests to C/C++ or what?

I would not recommend that. Is there no other way to get the full test suite running?

krokoziabla · 2019-04-08T08:53:44Z

Done. I've made those scripts run by make test but that only works on UNIX

evpobr · 2019-04-12T09:09:21Z

@krokoziabla Why so many CMakeLists.txt files everywhere? One per target is enough, CMake is not Autotools.

krokoziabla · 2019-04-12T09:21:20Z

At least 31 CMakeLists.txt flies out of 43 contain a target. Most of the rest contain some install() and add_test() commands.

In general my intention was to avoid creating a GOD-like CMakeLists.txt

evpobr · 2019-04-12T09:56:55Z

Then you have another problem - too many targets. Visual Studio will not be happy to handle this.

evpobr · 2019-04-12T09:57:17Z

Also, for Windows linking STATIC libraries is OK, what about other OSes? At least Linux requires -fPIC flags for static library if it is linked to shared library.

evpobr · 2019-04-12T10:01:19Z

One more thing - FLAC uses math functions, so under Unix math library must be linked (-lm).

krokoziabla · 2019-04-12T10:05:55Z

Then you have another problem - too many targets.

I guess you're speaking about two different things at one time - too many CMakeLists.txt vs. too many targets. I believe the number of targets cannot be reduced.

Visual Studio will not be happy to handle this.

Too abstract... You can try it yourself. I didn't experience any problems with VS while building this

Also, for Windows linking STATIC libraries is OK, what about other OSes? At least Linux requires -fPIC
flags for static library if it is linked to shared library.

If you look into the CMakeLists.txt for libFLAC and libFLAC++ you'll see that there're two variants of libraries built - static and shared

One more thing - FLAC uses math functions, so under Unix math library must be linked (-lm).

... and it is

evpobr · 2019-04-12T10:07:39Z

~~One more thing - FLAC uses math functions, so under Unix math library must be linked (-lm).~~

Sorry, i found it.

evpobr · 2019-04-12T10:09:58Z

If you look into the CMakeLists.txt for libFLAC and libFLAC++ you'll see that there're two variants of libraries built - static and shared

And both are linked to the same STATIC utf8 library.

evpobr · 2019-04-12T10:15:20Z

I guess you're speaking about two different things at one time - too many CMakeLists.txt vs. too many targets. I believe the number of targets cannot be reduced.

No. I mean if you will remove bunch of useless STATIC libraries you will remove banch of useless CMake scripts. Simpler is better.

krokoziabla · 2019-04-12T10:20:40Z

And both are linked to the same STATIC utf8 library.

No. Only executables link to utf8, not shared libFLAC or libFLAC++

No. I mean if you will remove bunch of useless STATIC libraries you will remove banch of useless CMake scripts. Simpler is better.

The structure of targets/libraries was inherited from exirting automake/*.vcxproj files. I think it is not my business to change this. Probably the product owner will reorganase the repository later if he or she finds it useful.

evpobr · 2019-04-12T10:25:32Z

No. Only binaries link to utf8, not shared libFLAC or libFLAC++

I se now. Then OK.

evpobr · 2019-04-12T10:37:37Z

No ABI version. This is critical for UNIX.

set_target_properties(FLAC PROPERTIES
        VERSION 8.3.11
        SOVERSION 8
)

krokoziabla · 2019-04-12T10:41:33Z

Why is it 8.3.11? I cannot find it anywhere in the repo.

erikd · 2019-04-29T23:45:31Z

There also seems to be a problem running the tests one machines where the Ogg libs are not installed. Specifically it seems to hang at:

1: opening Ogg FLAC file... OK
1: testing FLAC__stream_decoder_init_ogg_FILE()... OK

I suspect this is an issue with FLAC__HAS_OGG being incorrectly defined.

Another issue seems to be the test output. With the autotool test output the output is like:

8: sine16-18 (--channels=2 --bps=16 -5 -e --disable-verbatim-subframes --disable-constant-subframes): encode... decode... compare... OK

whereas the CMake build output is:

8: sine16-18 (--channels=2 --bps=16 -5 -e --disable-verbatim-subframes --disable-constant-subframes): encode...
8: decode...
8: compare...
8: OK

Would be nice to have the CMake output the same as the autotool output.

krokoziabla · 2019-04-30T09:50:22Z

So close!

I notice that the tests seemed to be compiled with DEBUG enabled. This results in some extra unneeded output being printed eg:
1: testing FLAC__stream_decoder_process_until_end_of_stream()... 0 ('This')... content... CPU info (x86-64):
1:   CMOV ....... Y
1:   MMX ........ Y
1:   SSE ........ Y
1:   SSE2 ....... Y
1:   SSE3 ....... Y
1:   SSSE3 ...... Y
1:   SSE41 ...... Y
1:   SSE42 ...... Y
1:   AVX ........ Y
1:   FMA ........ Y
1:   AVX2 ....... Y
1:   AVX OS sup . Y
1: OK
Without DEBUG enabled the CPU capabilities are not printed.

Are you sure you compiled a release build? If you add -DCMAKE_BUILD_TYPE=Release CMake will pass NDEBUG to gcc and this output will be suppressed. It works in my setup

krokoziabla · 2019-04-30T09:53:31Z

There also seems to be a problem running the tests one machines where the Ogg libs are not installed. Specifically it seems to hang at:
1: opening Ogg FLAC file... OK
1: testing FLAC__stream_decoder_init_ogg_FILE()... OK
I suspect this is an issue with FLAC__HAS_OGG being incorrectly defined.

Another issue seems to be the test output. With the autotool test output the output is like:
8: sine16-18 (--channels=2 --bps=16 -5 -e --disable-verbatim-subframes --disable-constant-subframes): encode... decode... compare... OK
whereas the CMake build output is:
8: sine16-18 (--channels=2 --bps=16 -5 -e --disable-verbatim-subframes --disable-constant-subframes): encode...
8: decode...
8: compare...
8: OK
Would be nice to have the CMake output the same as the autotool output.

I've checked now that if there is no OGG on my Mac then I don't see FLAC__stream_decoder_init_ogg_FILE messages. What is the value of OGG_FOUND macro in your generated config.h?

As to those newlines - fixed.

erikd · 2019-04-30T10:14:36Z

Are you sure you compiled a release build?

Is it possible to make the release build the default? That is the way it is with autotools.

krokoziabla · 2019-04-30T10:20:58Z

Are you sure you compiled a release build?

Is it possible to make the release build the default? That is the way it is with autotools.

Done

erikd · 2019-04-30T10:53:58Z

Looking good. Will merge this in the morning.

Enable components search in package config mode

krokoziabla · 2019-04-30T11:48:33Z

@cristianadam Thanks. Fixed

MarcusJohnson91 · 2019-04-30T15:34:35Z

@evpobr Amen about Cmake not needing a shit ton of CmakeLists.txt files laying around.

I personally just have a /Projects folder that contains a single CmakeLists.txt, and the source is in ../libX/

then I use git's submodules to incorporate various libraries together into a bigger project.

Quick question, is there a way to have the user set the library type (in a shared/static sense, not debug/release)?

add_library requires either STATIC or SHARED and I'm unsure if there's an official way to do that or not.

krokoziabla · 2019-04-30T19:10:55Z

@MarcusJohnson91 you can use set BUILD_SHARED_LIBS variable for that https://cmake.org/cmake/help/latest/command/add_library.html

erikd · 2019-04-30T20:49:20Z

As mentioned in #100 (comment) it would be nice to have something in the README explaining how to use CMake with Visual Studio.

krokoziabla · 2019-04-30T22:09:02Z

Done. But I apologise for my English. It's not very fluent. Probably you'll want to edit this part afterwards

evpobr · 2019-05-01T07:28:20Z

Done. But I apologise for my English. It's not very fluent. Probably you'll want to edit this part afterwards

@krokoziabla, I see OGG in documentation. It is not abbreviation, the correct name is Ogg. Just to be clear.

Chocobo1 · 2019-05-01T15:50:53Z

Just saw this, I think it is a must (at least for me) to have a cmake option to change MSVC runtime settings without manually patching CMakeLists.txt.

https://stackoverflow.com/questions/14172856/compile-with-mt-instead-of-md-using-cmake
https://docs.microsoft.com/en-us/cpp/build/reference/md-mt-ld-use-run-time-library?view=vs-2019

krokoziabla · 2019-05-01T16:08:36Z

@Chocobo1 If I'm not mistaken you can regulate that by passing /Mx flags to CMake in CMAKE_C_FLAGS_RELEASE or CMAKE_C_FLAGS_DEBUG variables. Does it work for you?

evpobr · 2019-05-01T16:27:19Z

if (ENABLE_STATIC_RUNTIME)
	if (MSVC)
		foreach (flag_var
			CMAKE_CXX_FLAGS CMAKE_CXX_FLAGS_DEBUG CMAKE_CXX_FLAGS_RELEASE
			CMAKE_CXX_FLAGS_MINSIZEREL CMAKE_CXX_FLAGS_RELWITHDEBINFO
			CMAKE_C_FLAGS CMAKE_C_FLAGS_DEBUG CMAKE_C_FLAGS_RELEASE
			CMAKE_C_FLAGS_MINSIZEREL CMAKE_C_FLAGS_RELWITHDEBINFO
			)
			if (${flag_var} MATCHES "/MD")
				string (REGEX REPLACE "/MD" "/MT" ${flag_var} "${${flag_var}}")
			endif ()
		endforeach (flag_var)
	endif (MSVC)
endif()

CMake 3.15 will have CMAKE_MSVC_RUNTIME_LIBRARY option to handle this.

Chocobo1 · 2019-05-01T16:39:55Z

@Chocobo1 If I'm not mistaken you can regulate that by passing /Mx flags to CMake in CMAKE_C_FLAGS_RELEASE or CMAKE_C_FLAGS_DEBUG variables. Does it work for you?

Yes, but I'm wishing for an explicit cmake configure option, or even better the snippet evpobr suggested.

CMake 3.15 will have CMAKE_MSVC_RUNTIME_LIBRARY option to handle this.

Glad to know they finally are going to put this nuisance to an end.
However that option still has to set CMP0091 or enable_language() to enable :\ well... better than nothing.

cristianadam · 2019-05-01T16:43:03Z

One can use a toolchain with CMake version 3.11+ to change the msvc runtime library without touching the CMake code.

Actually one shouldn't change CMAKE_C|XX_FLAGS in CMake code directly, you should use a toolchain.

See how it can be done here: https://cristianadam.eu/20190223/modifying-the-default-cmake-build-types/

erikd · 2019-05-02T01:09:30Z

Will someone please let me know when this is ready to be merged?

krokoziabla · 2019-05-02T06:20:02Z

Well, I don't have plans to add anything to it. I think it is ready to use in the sense that it provides basic functionality: it can build, test and install the lib. Forsure it always can be improved and extra features can be added but then we will never merge this PR if all of them go here.

I would propose to merge it now and let further extentions of functionality be proposed by other PRs, it there are any

erikd · 2019-05-02T06:31:01Z

@cristianadam @Chocobo1 @evpobr Anything further needed?

evpobr · 2019-05-02T07:27:28Z

@cristianadam @Chocobo1 @evpobr Anything further needed?

No.

erikd · 2019-05-04T01:56:40Z

Thank you all!

This has been merged to master and pushed to https://git.xiph.org/flac.git . It should be mirrored here within a couple of hours.

CMake support added

e348079

krokoziabla mentioned this pull request Apr 3, 2019

CMakeLists.txt for libFLAC been rewritten audacity/audacity#348

Closed

FindOGG.cmake module added

46b8441

/test/*.sh enabled for CTest in UNIX

be8676d

Cosmetic change: using $<COMPILE_LANGUAGE:>

63fe409

Added ECHO_C to CMake tests

3e5f411

CMAKE_BUILD_TYPE set to Release by default

eaf5015

Enable components search in package config mode

30f5a62

krokoziabla and others added 2 commits April 30, 2019 14:10

Merge pull request #4 from evpobr/cmake-components

f6d8caa

Enable components search in package config mode

CMAKE_BUILD_TYPE set to Release by default: improved

98c17ab

Added README description for CMake

b4d608a

erikd closed this May 4, 2019

krokoziabla deleted the krokoziabla/cmake branch May 6, 2019 09:23

This pull request was closed.

(Almost) complete CMake support added #97

(Almost) complete CMake support added #97

Conversation

krokoziabla commented Apr 3, 2019 • edited Loading

erikd commented Apr 3, 2019

lvqcl commented Apr 4, 2019

krokoziabla commented Apr 6, 2019 • edited Loading

erikd commented Apr 6, 2019

krokoziabla commented Apr 7, 2019 • edited Loading

erikd commented Apr 7, 2019

erikd commented Apr 7, 2019

krokoziabla commented Apr 7, 2019 • edited Loading

erikd commented Apr 7, 2019

krokoziabla commented Apr 8, 2019

erikd commented Apr 8, 2019

krokoziabla commented Apr 8, 2019

evpobr commented Apr 12, 2019

krokoziabla commented Apr 12, 2019

evpobr commented Apr 12, 2019

evpobr commented Apr 12, 2019

evpobr commented Apr 12, 2019

krokoziabla commented Apr 12, 2019

evpobr commented Apr 12, 2019

evpobr commented Apr 12, 2019

evpobr commented Apr 12, 2019

krokoziabla commented Apr 12, 2019 • edited Loading

evpobr commented Apr 12, 2019

evpobr commented Apr 12, 2019

krokoziabla commented Apr 12, 2019

erikd commented Apr 29, 2019

krokoziabla commented Apr 30, 2019

krokoziabla commented Apr 30, 2019

erikd commented Apr 30, 2019

krokoziabla commented Apr 30, 2019

erikd commented Apr 30, 2019

krokoziabla commented Apr 30, 2019

MarcusJohnson91 commented Apr 30, 2019 • edited Loading

krokoziabla commented Apr 30, 2019

erikd commented Apr 30, 2019

krokoziabla commented Apr 30, 2019

evpobr commented May 1, 2019

Chocobo1 commented May 1, 2019

krokoziabla commented May 1, 2019

evpobr commented May 1, 2019

Chocobo1 commented May 1, 2019

cristianadam commented May 1, 2019

erikd commented May 2, 2019

krokoziabla commented May 2, 2019

erikd commented May 2, 2019

evpobr commented May 2, 2019

erikd commented May 4, 2019

krokoziabla commented Apr 3, 2019 •

edited

Loading

krokoziabla commented Apr 6, 2019 •

edited

Loading

krokoziabla commented Apr 7, 2019 •

edited

Loading

krokoziabla commented Apr 7, 2019 •

edited

Loading

krokoziabla commented Apr 12, 2019 •

edited

Loading

MarcusJohnson91 commented Apr 30, 2019 •

edited

Loading