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

CMake: Add Features for docs, unit tests #400

Merged
merged 4 commits into from
Mar 2, 2020

Conversation

ferdnyc
Copy link
Contributor

@ferdnyc ferdnyc commented Dec 29, 2019

  • A new CMake option, ENABLE_TESTS, is created and defaults ON; pass -DENABLE_TESTS=0 to disable. (Legacy -DDISABLE_TESTS=1 will also override)
  • Target info for docs and tests is shown in the FeatureSummary
  • UnitTest++ is now RECOMMENDED, not REQUIRED, since libopenshot can be built without running unit tests

I also briefly broke FindUnitTest++.cmake, at one point completely changed the name to FindUnitTestCpp.cmake, and then ended up restoring the original name.

(There's a bug in CMake that prevents certain features of FindPackageHandleStandardArgs() from being used with module names that contain regular expression special characters (like +) — I actually submitted a change to CMake that fixes that issue, which was merged so in the future it won't be a concern. But since we support older releases, simplest solution is to just avoid making use of the feature (FOUND_VARIABLE) in question, which is what the new code does.)

By way of penance, when I restored the original module name I also added pkg-config support, so if UnitTest++ is installed in a standard system location (or if its pkgconfig directory location is added to the list of paths in the PKG_CONFIG_PATH environment variable) then FindUnitTest++.cmake will pick up a UnitTest++.pc file and import that info into the build environment.

No IMPORTED targets yet, though that's still on my goals list for this PR before merging. If it seems like it's just not gonna happen, I'll merge without and do that some time in the future.

The same old (environment,cache) variables will still work to set its path, but the PREFERRED way of specifying the location of UnitTest++ is by adding a -DUnitTest++_ROOT=/path/to/unittest++ argument to the CMake command line. pkgname_ROOT variables are handled with extra intelligence by CMake 3.12+, and the Find module (most, if not all, of our find modules now) now has forward-compatibility code that explicitly looks for those variables, so they'll work on older versions as well.

@ferdnyc ferdnyc added the build Issues related to compiling or installing libopenshot and its dependencies label Dec 29, 2019
@ferdnyc
Copy link
Contributor Author

ferdnyc commented Dec 29, 2019

CMake 3.16: cmake .. in build directory...

(Some output omitted)


-- Displaying feature summary

Build configuration:
-- The following features have been enabled:

 * Documentation, Build API documentation with 'make doc'
 * Testrunner, Run unit tests with 'make test'
 * Unit tests, Compile unit tests for library functions

-- The following OPTIONAL packages have been found:

 * cppzmq
 * RESVG
 * PythonInterp (required version >= 3)
 * PythonLibs (required version >= 3)
 * Ruby
 * Doxygen
 * PkgConfig
 * ImageMagick

-- The following RECOMMENDED packages have been found:

 * UnitTestCpp, aka UnitTest++
   Unit testing framework

-- The following REQUIRED packages have been found:

 * JsonCpp
 * Qt5Widgets
 * Qt5Core
 * Qt5Gui
 * Qt5Network (required version >= 5.13.2)
 * Qt5Multimedia
 * Qt5MultimediaWidgets
 * Qt5
 * FFmpeg
 * Threads
 * OpenMP
 * ZeroMQ
 * SWIG (required version >= 3.0)
 * OpenShotAudio (required version >= 0.1.9)

-- The following features have been disabled:

 * Coverage, analyze test coverage and generate report
 * IWYU (include-what-you-use), Scan all source files with 'iwyu'

-- Configuring done
CMake 3.2: cmake32 .. in build directory...

(Some output omitted)


-- Displaying feature summary

Build configuration:
-- The following features have been enabled:

 * Documentation , Build API documentation with 'make doc'
 * Testrunner , Run unit tests with 'make os_test'
 * Unit tests , Compile unit tests for library functions

-- The following OPTIONAL packages have been found:

 * cppzmq
 * PythonInterp (required version >= 3)
 * PythonLibs (required version >= 3)
 * Ruby
 * Doxygen
 * PkgConfig

-- The following REQUIRED packages have been found:

 * JsonCpp
 * Qt5Widgets
 * Qt5Core
 * Qt5Gui
 * Qt5Network (required version >= 5.12.5)
 * Qt5Multimedia
 * Qt5MultimediaWidgets
 * Qt5
 * FFmpeg
 * Threads
 * OpenMP
 * ZeroMQ
 * SWIG (required version >= 3.0)
 * UnitTest++
 * OpenShotAudio (required version >= 0.1.8)

-- The following features have been disabled:

 * IWYU (include-what-you-use) , Scan all source files with 'iwyu'

-- The following OPTIONAL packages have not been found:

 * ImageMagick
 * RESVG

If -DDISABLE_TESTS=1 or -DENABLE_TESTS=0 is passed on the command line, the disabled features output changes to:

-- The following features have been disabled:

 * IWYU (include-what-you-use), Scan all source files with 'iwyu'
 * Unit tests, Compile unit tests for library functions

And the 'Testrunner' target info is not shown at all.

@codecov-io
Copy link

codecov-io commented Jan 12, 2020

Codecov Report

Merging #400 into develop will increase coverage by 0.84%.
The diff coverage is 28.8%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #400      +/-   ##
===========================================
+ Coverage    42.37%   43.22%   +0.84%     
===========================================
  Files          129      130       +1     
  Lines        13276    12932     -344     
===========================================
- Hits          5626     5590      -36     
+ Misses        7650     7342     -308
Impacted Files Coverage Δ
include/effects/Shift.h 0% <ø> (ø) ⬆️
include/ImageReader.h 25% <ø> (ø) ⬆️
include/DummyReader.h 0% <ø> (ø) ⬆️
include/WriterBase.h 100% <ø> (ø) ⬆️
include/effects/Saturation.h 0% <ø> (ø) ⬆️
include/effects/Brightness.h 0% <ø> (ø) ⬆️
include/effects/Mask.h 0% <ø> (ø) ⬆️
include/QtImageReader.h 33.33% <ø> (ø) ⬆️
include/effects/Pixelate.h 0% <ø> (ø) ⬆️
include/effects/Negate.h 100% <ø> (ø) ⬆️
... and 71 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7ab18fd...600e884. Read the comment docs.

- A new CMake option, ENABLE_TESTS, is created and defaults ON
  (Legacy -DDISABLE_TESTS=1 on the command line will override)
- Target info for docs and tests is shown in the FeatureSummary
The same old variables will be respected for setting the path, but the
PREFERRED method is defining `-DUnitTest++_ROOT=/path/to` on the
CMake command line. _ROOT variables are handled with extra intelligence
by CMake.

The find module will also attempt to locate the pkg-config file
UnitTest++.pc, and if found will import its data.
@ferdnyc
Copy link
Contributor Author

ferdnyc commented Feb 26, 2020

Oh, I forgot to mention: if pkg-config is used to configure UnitTest++, it'll also pick up and report the version during CMake's generation process:


cmake -DCMAKE_BUILD_TYPE=Release -DOpenShotAudio_ROOT=../../libopenshot-audio/build/dist/ -DRESVG_ROOT=/home/ferd/rpmbuild/REPOS/resvg/ ..
-----------------------------------------------------------------
          Welcome to the OpenShot Build System!

CMake will now check libopenshot's build dependencies and inform
you of any missing files or other issues.

For more information, please visit .
-----------------------------------------------------------------
-- The C compiler identification is GNU 9.2.1
-- The CXX compiler identification is GNU 9.2.1
-- Check for working C compiler: /usr/lib64/ccache/cc
-- Check for working C compiler: /usr/lib64/ccache/cc -- works
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Detecting C compile features
-- Detecting C compile features - done
-- Check for working CXX compiler: /usr/lib64/ccache/c++
-- Check for working CXX compiler: /usr/lib64/ccache/c++ -- works
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Detecting CXX compile features
-- Detecting CXX compile features - done

Generating build files for OpenShot with CMake 3.16.4
  Building libopenshot (version 0.2.4)
  SO/API/ABI Version: 18

-- Found ImageMagick: /usr/lib64/libMagick++-6.Q16.so (found version "6.9.10-86") 
-- Found OpenShotAudio: /home/ferd/rpmbuild/REPOS/libopenshot-audio/build/dist/lib64/
libopenshot-audio.so (found suitable version "0.1.9-dev1", minimum required is "0.1.9") 
-- Looking for system JsonCpp
-- Found PkgConfig: /usr/bin/pkg-config (found version "1.6.3") 
-- Found jsoncpp: /usr/include  
[...]
-- Found Doxygen: /usr/bin/doxygen (found version "1.8.15") found components: doxygen dot 
-- Found DOXYFILE_IN: /home/ferd/rpmbuild/REPOS/libopenshot/Doxyfile.in  
-- Doxygen found, documentation target enabled
-- Found UnitTest++: /usr/lib64/libUnitTest++.so (found version "2.0.0")
-- Tests enabled, test executable will be built as tests/openshot-test
-- Cmake 3.11+ detected, enabling 'test' target
-- Displaying feature summary

Build configuration:
-- The following features have been enabled:

 * Documentation, Build API documentation with 'make doc'
 * Testrunner, Run unit tests with 'make test'
 * Unit tests, Compile unit tests for library functions

-- The following OPTIONAL packages have been found:

 * cppzmq
 * RESVG
 * PythonInterp (required version >= 3)
 * PythonLibs (required version >= 3)
 * Ruby
 * Doxygen
 * PkgConfig
 * ImageMagick

-- The following RECOMMENDED packages have been found:

 * UnitTest++
   Unit testing framework

-- The following REQUIRED packages have been found:

 * JsonCpp
 * Qt5Widgets
 * Qt5Core
 * Qt5Gui
 * Qt5Network (required version >= 5.13.2)
 * Qt5Multimedia
 * Qt5MultimediaWidgets
 * Qt5
 * FFmpeg
 * Threads
 * OpenMP
 * ZeroMQ
 * SWIG (required version >= 3.0)
 * OpenShotAudio (required version >= 0.1.9)

-- The following features have been disabled:

 * Coverage, analyze test coverage and generate report
 * IWYU (include-what-you-use), Scan all source files with 'iwyu'

-- Configuring done
-- Generating done
-- Build files have been written to: /home/ferd/rpmbuild/REPOS/libopenshot/build

@jonoomph
Copy link
Member

@ferdnyc Any risk of this breaking on build servers? Should we test on a dedicated branch, or has that already been done? I'm okay merging if you feel it's safe though.

@ferdnyc
Copy link
Contributor Author

ferdnyc commented Feb 27, 2020

@jonoomph No significant danger. I tested the configs locally with CMake 3.2, as well as Fedora's current 3.16.

@ferdnyc
Copy link
Contributor Author

ferdnyc commented Feb 27, 2020

I SHOULD check whether there are any changes to .gitlab-ci.yml that should go with this. I'll do that this evening, then merge.

Edit: (Well, that didn't happen, but only because I fell asleep at 7pm and just slept for 12 hours. I'll get to it; there's no urgency as this is purely build-housekeeping. #SoTired)

@ferdnyc
Copy link
Contributor Author

ferdnyc commented Mar 2, 2020

I'm actually going to merge this, because I just experimentally discovered that on Windows, trying to build without this change will fail if UnitTest++ is not installed. With these changes, it's correctly skipped. (The unit tests are simply disabled, as intended.)

Any changes to the Gitlab builder configs should be optional, not required, so those can happen separately.

@ferdnyc ferdnyc merged commit b724f2e into OpenShot:develop Mar 2, 2020
@ferdnyc
Copy link
Contributor Author

ferdnyc commented Mar 2, 2020

(I will of course check on the Gitlab builders now...)

@ferdnyc
Copy link
Contributor Author

ferdnyc commented Mar 2, 2020

Everything configured fine. Amusingly, the presence of a version number in the discovery logs indicates that the macOS builder used pkg-config to discover libunittest++.dylib:

-- Found UnitTest++: /usr/local/lib/libUnitTest++.dylib (found version "2.0.0") 

but the Linux builder DIDN'T:

-- Found UnitTest++: /usr/lib/libunittest++.a  

...I don't even know what to make of that.

(Maybe it's because Linux has (only?) the static-lib version of UnitTest++, not the shared lib. Or it could be that the name is different — on my Fedora system, the shared lib is named libUnitTest++.so.2, so the Find module looks for a pkg-config file UnitTest++.pc. Maybe it was named unittest++.pc on ancient Ubuntu. 🤷‍♂️ )

Regardless, doesn't affect anything with the build.

@ferdnyc ferdnyc deleted the add-features branch March 2, 2020 01:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues related to compiling or installing libopenshot and its dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants