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 improvements #168

Open
wants to merge 16 commits into
base: master
from

Conversation

@piponazo
Copy link

piponazo commented Oct 18, 2019

In this PR I am proposing the addition of some improvements in the CMake code. The motivation of my work is to create a conan package (it will be hosted here) so that we can consume libheif in exiv2 (see the related PR here).

In case you do not know about conan, it is C++ Package Manager which makes very easy to create packages of libraries for different compilers, operative systems, compilation modes, etc. My plan is to create a conan package for libheif which could be used by any other project to consume libheif in a straightforward way.

These are the main changes I did in the CMake code (ordered by relevance):

  1. Addition of CMake installation commands. With this addition, we can run make install , ninja install or similar install commands for Visual Studio, XCode, etc.
  2. Export of Package Config file so that the project can be consumed easily by other CMake projects by doing find_package(heif).
  3. Usage of the variables PROJECT_NAME and PROJECT_VERSION which are defined by the CMake command project(). I think it might be a bit controversial one of the changes I did here ... I notice that you were using different library names depending on the platform ... which add lot of complexity when trying to consume libheif from other cross-platform projects.
  4. Modernisation of some CMake code. Updated required CMake version from 2.8 to 3.3 (which was released in July 23, 2015). This is the version we use in Exiv2, and most of the linux distributions have that version or higher.
  5. Generate heif_version.h in the build directory to not pollute the source directory.

Anyways, I tried to separate all the individual changes in separated commits to make the review process easier.

libheif/CMakeLists.txt Outdated Show resolved Hide resolved
@farindk

This comment has been minimized.

Copy link
Contributor

farindk commented Oct 18, 2019

Thanks for this work. It's much appreciated.

@piponazo

This comment has been minimized.

Copy link
Author

piponazo commented Oct 18, 2019

Heads up: sorry if I keep pushing some minor commits. I am testing the changes in several platforms now with the conan package that I am creating:

https://github.com/piponazo/conan-libheifh

I will notify you when I have the package working for gcc, clang, msvc on different platforms (linux, mac, windows).

@cgilles

This comment has been minimized.

Copy link

cgilles commented Oct 19, 2019

Note: libde265 needs a similar CMake improvement patch. As libheif team maintain libde265, please considerate to backport this PR too into libde265. Thanks in advance...

@piponazo

This comment has been minimized.

Copy link
Author

piponazo commented Oct 20, 2019

Note: libde265 needs a similar CMake improvement patch. As libheif team maintain libde265, please considerate to backport this PR too into libde265. Thanks in advance...

Sure, I'll take a look there too 😉

@piponazo piponazo mentioned this pull request Oct 20, 2019
@piponazo

This comment has been minimized.

Copy link
Author

piponazo commented Oct 20, 2019

I finally managed to have a conan recipe which is building for the classical combination of compilers and OSs used in conan:

https://travis-ci.org/piponazo/conan-libheifh/builds/600339942
https://ci.appveyor.com/project/piponazo/conan-libheifh

Once this PR gets merged, I will point the conan recipe to the upstream libheif repository and provide binary packages in bintray.

@D4N

This comment has been minimized.

Copy link

D4N commented Oct 22, 2019

The travis error appears to be a fluke: @farindk can you restart the one failing job?

Copy link
Member

fancycode left a comment

Thanks a lot for the progress so far. I left you some comments with feedback.

@@ -61,12 +64,6 @@ endif()

include_directories ("${PROJECT_SOURCE_DIR}")

if(UNIX)
set(LIBHEIF_LIBRARY_NAME heif)

This comment has been minimized.

Copy link
@fancycode

fancycode Oct 22, 2019

Member

This is necessary so the resulting file on Linux is named libheif.so. Without it, the name is liblibheif.so:
https://travis-ci.org/strukturag/libheif/jobs/599715352#L664

[ 43%] Linking CXX shared library liblibheif.so

This comment has been minimized.

Copy link
@piponazo

piponazo Oct 22, 2019

Author

This is due to the internal CMake variable CMAKE_SHARED_LIBRARY_PREFIX (whose default value for UNIX environments is lib:

https://cmake.org/cmake/help/latest/variable/CMAKE_SHARED_LIBRARY_PREFIX.html

Would it be fine for you to change the clear the value of that variable on UNIX so that we end up having libheif.so on UNIX and libheif.lib on Windows ?

My objective was to remove LIBHEIF_LIBRARY_NAME because we already have one variable (PROJECT_NAME) containing the information we need.

This comment has been minimized.

Copy link
@fancycode

fancycode Oct 22, 2019

Member

I'm fine with whatever works 😄 However if you clear CMAKE_SHARED_LIBRARY_PREFIX, make sure the libpixbufloader-heif.so from folder gdk-pixbuf still has the lib prefix.

This comment has been minimized.

Copy link
@piponazo

piponazo Oct 22, 2019

Author

Ha, I did not take into account that target. I think I will revert then this change and come back to the original implementation.

This comment has been minimized.

Copy link
@piponazo

piponazo Oct 22, 2019

Author

I take back what I just said. If we have different names for the target for different platforms, it is going to be a mess to consume libheif from other multi-platform projects.

I will think about this and come with a proposal.

This comment has been minimized.

Copy link
@piponazo

piponazo Oct 23, 2019

Author

I just made a change related to that. I renamed the project() to heif so that it will produce:

  • Unix: libheif.so
  • Windows: heif.lib & heif.dll

Then I hardcoded the include directory to libheif so that consumers of the library does not need to change anything to find the header files. It will only change for windows consumers, which will need to update the library name from libheif to heif.

From my point of view, it is legit to make this change because anyways, any CMake consumer of libheif should now use the CMake Config file to find the dependency, and that config file is in charge of indicating the library name and include directories. Note that it is also the default behaviour in CMake to have libXXX.so for UNIX and XXX.lib on Windows. Experiments trying to modify the default behavior end up in additional code for the maintained project, and additional difficulties for the consumers of the library.

Since this is a "breaking change" for windows users, I propose to create a new release of the library if this PR gets finally merged.

libheif/heif_version.h.in Show resolved Hide resolved
libheif/CMakeLists.txt Show resolved Hide resolved
libheif/CMakeLists.txt Show resolved Hide resolved
libheif/CMakeLists.txt Show resolved Hide resolved
libheif/CMakeLists.txt Show resolved Hide resolved
@fancycode

This comment has been minimized.

Copy link
Member

fancycode commented Oct 22, 2019

The travis error appears to be a fluke: @farindk can you restart the one failing job?

Just restarted the failing jobs.

@piponazo

This comment has been minimized.

Copy link
Author

piponazo commented Oct 22, 2019

The travis error appears to be a fluke: @farindk can you restart the one failing job?

Just restarted the failing jobs.

I noticed that the failing travis-ci jobs are also failing in other branches. It seems that the failures are not related to the changes I am doing here. Am I right?

@fancycode

This comment has been minimized.

Copy link
Member

fancycode commented Oct 22, 2019

The travis error appears to be a fluke: @farindk can you restart the one failing job?

Just restarted the failing jobs.

I noticed that the failing travis-ci jobs are also failing in other branches. It seems that the failures are not related to the changes I am doing here. Am I right?

Yes, this looks like a Travis hiccup. I can try to restart the jobs later.

@piponazo

This comment has been minimized.

Copy link
Author

piponazo commented Oct 23, 2019

Yes, this looks like a Travis hiccup. I can try to restart the jobs later.

Note that in master, those failures are already there:
https://travis-ci.org/strukturag/libheif/builds/597687977

@piponazo piponazo force-pushed the piponazo:cmakeImprovements branch from 00b9a08 to bbf8afa Nov 14, 2019
@piponazo

This comment has been minimized.

Copy link
Author

piponazo commented Nov 18, 2019

Hi, do I need to do something else about this PR? I would like to see this contribution included into the project soon to unblock some work we are doing in Exiv2 (Exiv2/exiv2#1044) .

The failures in TravisCI seems to be due to things unrelated to the changes I did here.

libheif/CMakeLists.txt Outdated Show resolved Hide resolved
libheif/CMakeLists.txt Outdated Show resolved Hide resolved
libheif/CMakeLists.txt Outdated Show resolved Hide resolved
libheif/CMakeLists.txt Outdated Show resolved Hide resolved
@@ -68,12 +68,14 @@ endif()
add_definitions(-DHAVE_VISIBILITY)
add_definitions(-DLIBHEIF_EXPORTS)

add_library(${LIBHEIF_LIBRARY_NAME} SHARED ${libheif_sources})
add_library(${PROJECT_NAME} SHARED ${libheif_sources})

This comment has been minimized.

Copy link
@cryptomilk

cryptomilk Nov 20, 2019

I think this is wrong, as the PROJECT_NAME is libheif which would result in liblibheif. In the meantime you do:

add_library(heif ${libheif_sources})
target_link_libraries(heif ...)

add_library(heif::heif ALIAS heif)

If you want to link against heif, use the alias heif::heif. This way you can also modify it.

instead of add_library(... SHARED ...) you remove the shared and add:

option(BUILD_SHARED_LIBS "Build shared libraries" ON)

Then you can easily build as a static lib too. This is what the documentation for BUILD_SHARED_LIBS suggest.

This comment has been minimized.

Copy link
@piponazo

piponazo Nov 20, 2019

Author

Did you read the comment I posted here?
https://github.com/strukturag/libheif/pull/168/files#r337861735

The original name of the project was libheif and I wanted to change as little as possible to avoid having a bigger impact in codebases consuming libheif. I also think it makes sense to use ${PROJECT_NAME} as the library target name to avoid hardcoded values.

I think I did not understand you completely about your point with the line

add_library(heif::heif ALIAS heif)

In case we would want to add a namespace for the library target, should not we use the NAMESPACE option in the install(EXPORT ...) command?
https://cmake.org/cmake/help/v3.13/command/install.html#installing-exports

Finally, I completely agree with the BUILD_SHARED_LIBS suggestion. It is also something I normally use in my projects. I will add it.

This comment has been minimized.

Copy link
@piponazo

piponazo Nov 20, 2019

Author

I just pushed to additional commit adding the support for BUILD_SHARED_LIBS and other one changing the name of the project to heif (more information in the commit message).

@fancycode

This comment has been minimized.

Copy link
Member

fancycode commented Nov 20, 2019

The failures in TravisCI seems to be due to things unrelated to the changes I did here.

@piponazo The CI issues have been fixed on master, please rebase your branch to get the latest changes.

piponazo added 10 commits Oct 18, 2019
This is to simplify the headers installation process later
In order to use the write_basic_package_version_file we need to provide the project version in the project() command
…t in the source_directory

It is a bad practice to pollute the source directory with things generated for the build.
I also reused the PROJECT_VERSION variable and removed the PACKAGE_VERSION one.
The objectives of this commit are:
- to reduce the usage of the conditionals LIBDE265_FOUND and X265_FOUND
- Have less local variables
@piponazo piponazo force-pushed the piponazo:cmakeImprovements branch from bf3a229 to 81e9204 Nov 20, 2019
piponazo added 2 commits Nov 20, 2019
This change is done so that it will produce:

Unix: libheif.so
Windows: heif.lib & heif.dll

The include directory is hardcoded to libheif so that consumers of the library does not need to
change anything to find the header files. It will only change for windows consumers, which will need
to update the library name from libheif to heif.

From my point of view, it is legit to make this change because anyways, any CMake consumer of libheif
should now use the CMake Config file to find the dependency, and that config file is in charge of
indicating the library name and include directories. Note that it is also the default behaviour in
CMake to have libXXX.so for UNIX and XXX.lib on Windows. Experiments trying to modify the default
behavior end up in additional code for the maintained project, and additional difficulties for the
consumers of the library.
@piponazo piponazo requested a review from fancycode Nov 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.