CMake: Generate pkg-config files for all Windows builds #396
Conversation
fanc999
force-pushed
the
cmake-win32-pkgconfig
branch
from
7c09136
to
c679937
Compare
|
Thank you, looks neat, but could you please fix the following build error? I tested it on macOS, and it fails with the Ninja generator and the Release target, as follows: |
|
Any update on this? It will be very helpful! |
fanc999
force-pushed
the
cmake-win32-pkgconfig
branch
from
c679937
to
b3f0140
Compare
|
Hi @ctruta, Sorry for the delay, as I was away and this PR was out for some time... I rebased some items, so I think things should probably be remedied, hopefully, since I don't have a macOS devenv. |
CMakeLists.txt
Outdated
| else() | ||
| STRING (TOLOWER "${CMAKE_BUILD_TYPE}" config_lower) | ||
| if (config_lower STREQUAL "debug") | ||
| set(LIBS "${ZLIB_LIBRARY_DEBUG} ${M_LIBRARY}") |
There was a problem hiding this comment.
I'm not an expert at how CMake libraries are being exported and imported. My understanding from other people is that there shouldn't be a ZLIB_LIBRARY or a ZLIB_INCLUDE_DIR (or a ZLIB_LIBRARY_DEBUG, etc.), but rather a symbol named ZLIB_LIBRARIES and another symbol named ZLIB_INCLUDE_DIRS.
For that reason, I am asking: are you sure that we shouldn't just have the following:
set(LIBS "${ZLIB_LIBRARIES} ${M_LIBRARY}"
I can imagine that there are pros and cons to each side, but I would like to know your opinion also.
Moreover, take a look at line 598, further up in this CMake file:
target_link_libraries(png ${ZLIB_LIBRARIES} ${M_LIBRARY})
I'd like to be sure that there won't be breakages due to this inconsistency.
There was a problem hiding this comment.
The correct way would be to have a target like ZLib::ZLib with properties INTERFACE_INCLUDE_DIRECTORIES, INTERFACE_COMPILE_OPTIONS (if any present), and at least IMPORTED_LOCATION set (there's also properties for correctly using SONAME, and VERSION as well as SOVERSION.
If build configuration variants are present, they are supposed to be set as IMPORTED_LOCATION_RELEASE, IMPORTED_LOCATION_DEBUG, etc. with the IMPORTED_CONFIGURATIONS property set to the actual variants available. This will ensure CMake picks the correct variant (and usually falls back on the Release variant for RelWithDebInfo and MinSizeRel if present).
(On Windows you should also use IMPORTED_LIBNAME and its configuration-specific variants for the .lib file and if a .DLL exists set that as IMPORTED_LOCATION.)
With that you just run target_link_libraries(png <PRIVATE|PUBLIC> ZLib::ZLib).
Rule of Thumb with CMake 3 is: Use targets, target names, and target properties - don't use variables.
EDIT: It should be noted that this is precisely what CMake's FindZLib.cmake does: If config-specific variants are found, their properties are set and IMPORTED_CONFIGURATIONS will be set accordingly.
There was a problem hiding this comment.
Hi,
Sadly, using ${ZLIB_LIBRARIES} in the pkg-config file generating won't work, because you will have all the variants inside there. Will look for a cleaner solution.
There was a problem hiding this comment.
Hi,
I updated the CMakeLists.txt to reflect on the suggestions that were raised purely on the part that this PR attempts to cover, so I did not replace the occurences of ZLIB_LIBRARIES with ZLIB::ZLIB in any way but instead grabbed the library names for our pkg-config files with get_target_property(ZLIB_LIB ZLIB::ZLIB IMPORTED_LOCATION[_xxx]).
Please note that there is, however, no IMPORTED_IMPLIB[_$<CONFIG>] property from CMake's FindZLIB.cmake, even with DLL builds of ZLib, somehow.
CMakeLists.txt
Outdated
|
|
||
| # Set up links. | ||
| if(PNG_SHARED) | ||
| set_target_properties(png PROPERTIES | ||
| VERSION ${PNGLIB_SHARED_VERSION} | ||
| SOVERSION ${PNGLIB_SHARED_SOVERSION} | ||
| CLEAN_DIRECT_OUTPUT 1) | ||
| if(WIN32 AND NOT CYGWIN AND NOT MINGW) | ||
| foreach (pc_file ${CMAKE_CURRENT_BINARY_DIR}/${PNGLIB_NAME}.pc ${CMAKE_CURRENT_BINARY_DIR}/libpng.pc) | ||
| FILE(READ ${pc_file} png_pc_orig) |
There was a problem hiding this comment.
Could you please use lowercase CMake commands (file, string, etc.), for the sake of stylistic consistency?
The Meson build system makes use of pkg-config files or CMake to find most of the dependencies via its "built-in" dependency search system. As libpng is a very widely used package, this makes the CMake build system generate pkg-config files for libpng even for non-Cygwin/non-MinGW Windows builds, so that Meson could locate libpng via pkg-config as well even on Visual Studio builds, for instance. We also do the following on native, non-MinGW Windows builds: * Replace -lpngXX with libpngXX.lib, for the libpng library that we link to * Use the correct ZLib library .lib name for `libs.private`. * Replace `Requires.private: zlib` with `Requires.private:`, since we already say in `libs.private` that we are linking to ZLib
Use all-lowercase for CMake commands
fanc999
force-pushed
the
cmake-win32-pkgconfig
branch
2 times, most recently
from
11fcfbd
to
edf3ac9
Compare
Use the target properties of ZLIB::ZLIB to get the library names for the pkg-config files that we generate, and unify things with other builds. We need to use the IMPORT_LOCATION[_$<CONFIG>] for ZLib since it does not define IMPORT_IMPLIB[_$<CONFIG>] even on Windows DLL builds, and assume that the $<CONFIG> we use above is RELEASE for both MinSizeRel and RelWithDebInfo builds.
fanc999
force-pushed
the
cmake-win32-pkgconfig
branch
from
edf3ac9
to
c3d2bdf
Compare
|
Hi, here's a heads-up, I am currently applying refactoring changes in order to clean up the CMakefile and to make Clang on Windows work correctly, without regressing other platforms. Your contributions from this pull request may or may not break because of these upcoming changes. On the positive side of the news, this pull request is next on my to-do list. |
Hi,
This PR is opened to support generating pkg-config files also for native (non-Cygwin) Windows builds, in addition to MinGW/mingw-w64 builds, since the Meson build system (which uses pkg-config and CMake to find dependencies for most packages) is becoming more widespread for building items such as Cairo and the GNOME/GTK stack and the current CMake config files generated by the CMake builds are not usable by Meson as a consequence of #179, which should be fixed in a PR of its own.
With blessings, thank you!