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

Some polishing for the cmake ReSVG discovery #187

Merged
merged 4 commits into from
Jan 24, 2019

Conversation

ferdnyc
Copy link
Contributor

@ferdnyc ferdnyc commented Jan 22, 2019

find_package_handle_standard_args is supposed to set the _FOUND variable automatically (as the comment right above it says), as well as handling things like REQUIRED, QUIETLY, etc. It should always be run at the end of a module, for this reason.

This change removes the conditionals around the call, lets it handle what it's meant to handle, and defines a custom failure message for discovery that replaces the manual one in src/CMakeList.txt.

In addition, the REQUIRED is removed from tests/CMakeLists.txt, since it's supposed to mark the module as required (which ReSVG is not), and was only working due to the aforementioned improper
conditional wrapping of the module's cleanup.

With these changes, on my system libopenshot still builds and make test still runs successfully both with and without ReSVG being discovered.

Additionally, if only one of the two parts is missing, cmake will automatically indicate that in the failure message:

$ cd /tmp/resvg/target/release/
$ ln -s ../../capi/include .
$ ls
build  examples  incremental  libresvg.d        libresvg.rlib  libusvg.rlib
deps   include   libresvg.a   libresvg_qt.rlib  libresvg.so    native
$ rm libresvg.so libresvg.a # (To my great surprise, libresvg.a would be discovered and used!)
$ cd /tmp/libopenshot/
$ rm CMakeCache.txt
$ RESVGDIR=/tmp/resvg/target/release /usr/bin/cmake -DCMAKE_INSTALL_PREFIX:PATH=/usr
[...]
-- Could NOT find RESVG, using Qt SVG parsing instead (missing: RESVG_LIBRARY) 

The message correctly indicates that only RESVG_LIBRARY was not found, because RESVG_INCLUDE_DIR was still successfully discovered as /tmp/resvg/target/release/include/.

Also:

  • $ENV{RESVGDIR} is added to the search list for find_path( RESVG_LIBRARY..., so that copying/linking the include dir into target/release/ and running RESVGDIR=/path/to/resvg/target/release/ cmake ... will successfully discover the library
  • The comment in FindRESVG.cmake claiming it sets RESVG_DEFINITIONS is removed, because it doesn't

This means that RESVGDIR can be pointed to the `target/release` dir 
where resvg was built, and both `libresvg.so` and `include/resvg.h` will 
be found.
`find_package_handle_standard_args` is supposed to set the `_FOUND` 
variable automatically (as the comment right above it says), as well as 
handling things like REQUIRED, QUIETLY, etc. It should always be run at 
the end of a module, for this reason.

This change removes the conditionals around the call, lets it handle 
what it's meant to handle, and defines a custom failure message for 
discovery that replaces the one in `src/CMakeList.txt`. 

In addition, the `REQRUIRED` is removed from `tests/CMakeLists.txt`, 
since it's _supposed_ to mark the module as required (which ReSVG is 
not), and was only working due to the aforementioned improper 
conditional wrapping of the module's cleanup.
Remove the comment that claims it does.
@jonoomph
Copy link
Member

This looks great @ferdnyc! I'm gonna test this on my system real quick

@jonoomph jonoomph merged commit c4321f3 into OpenShot:develop Jan 24, 2019
@jonoomph
Copy link
Member

Worked great on my system! Merged and hopefully all the build servers will like it also. 😄

@ferdnyc ferdnyc deleted the find-resvg branch January 26, 2019 18:12
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.

2 participants