cmake: fix pkgconfig and cmake interaction

[jmcnamara]

Proposed fix for microsoft/vcpkg#43653

Fixes previous PR #454

[jmcnamara]

Adding @diizzyy, @dg0yt and @jimwang118 for review.

[diizzyy]

It hardlinks libminizip and library paths gets omitted but it works as long as you already have paths in place

CURRENT

[ 96% 28/29] : && /usr/bin/cc -fPIC -O2 -pipe -march=znver4  -fstack-protector-strong -fno-strict-aliasing -Wno-deprecated-declarations -O2 -pipe -march=znver4  -fstack-protector-strong -fno-strict-aliasing  -DNDEBUG  -fstack-protector-strong   -Xlinker --dependency-file=CMakeFiles/xlsxwriter.dir/link.d -shared -Wl,-soname,libxlsxwriter.so.7 -o libxlsxwriter.so.7 CMakeFiles/xlsxwriter.dir/src/app.c.o CMakeFiles/xlsxwriter.dir/src/chart.c.o CMakeFiles/xlsxwriter.dir/src/chartsheet.c.o CMakeFiles/xlsxwriter.dir/src/comment.c.o CMakeFiles/xlsxwriter.dir/src/content_types.c.o CMakeFiles/xlsxwriter.dir/src/core.c.o CMakeFiles/xlsxwriter.dir/src/custom.c.o CMakeFiles/xlsxwriter.dir/src/drawing.c.o CMakeFiles/xlsxwriter.dir/src/format.c.o CMakeFiles/xlsxwriter.dir/src/hash_table.c.o CMakeFiles/xlsxwriter.dir/src/metadata.c.o CMakeFiles/xlsxwriter.dir/src/packager.c.o CMakeFiles/xlsxwriter.dir/src/relationships.c.o CMakeFiles/xlsxwriter.dir/src/rich_value.c.o CMakeFiles/xlsxwriter.dir/src/rich_value_rel.c.o CMakeFiles/xlsxwriter.dir/src/rich_value_structure.c.o CMakeFiles/xlsxwriter.dir/src/rich_value_types.c.o CMakeFiles/xlsxwriter.dir/src/shared_strings.c.o CMakeFiles/xlsxwriter.dir/src/styles.c.o CMakeFiles/xlsxwriter.dir/src/table.c.o CMakeFiles/xlsxwriter.dir/src/theme.c.o CMakeFiles/xlsxwriter.dir/src/utility.c.o CMakeFiles/xlsxwriter.dir/src/vml.c.o CMakeFiles/xlsxwriter.dir/src/workbook.c.o CMakeFiles/xlsxwriter.dir/src/worksheet.c.o CMakeFiles/xlsxwriter.dir/src/xmlwriter.c.o CMakeFiles/xlsxwriter.dir/third_party/tmpfileplus/tmpfileplus.c.o  -L/usr/lib  -L/lib  -lz  -L/usr/local/lib  -lminizip  -L/usr/lib  -lcrypto && :

PATCH

[ 96% 28/29] : && /usr/bin/cc -fPIC -O2 -pipe -march=znver4  -fstack-protector-strong -fno-strict-aliasing -Wno-deprecated-declarations -O2 -pipe -march=znver4  -fstack-protector-strong -fno-strict-aliasing  -DNDEBUG  -fstack-protector-strong   -Xlinker --dependency-file=CMakeFiles/xlsxwriter.dir/link.d -shared -Wl,-soname,libxlsxwriter.so.7 -o libxlsxwriter.so.7 CMakeFiles/xlsxwriter.dir/src/app.c.o CMakeFiles/xlsxwriter.dir/src/chart.c.o CMakeFiles/xlsxwriter.dir/src/chartsheet.c.o CMakeFiles/xlsxwriter.dir/src/comment.c.o CMakeFiles/xlsxwriter.dir/src/content_types.c.o CMakeFiles/xlsxwriter.dir/src/core.c.o CMakeFiles/xlsxwriter.dir/src/custom.c.o CMakeFiles/xlsxwriter.dir/src/drawing.c.o CMakeFiles/xlsxwriter.dir/src/format.c.o CMakeFiles/xlsxwriter.dir/src/hash_table.c.o CMakeFiles/xlsxwriter.dir/src/metadata.c.o CMakeFiles/xlsxwriter.dir/src/packager.c.o CMakeFiles/xlsxwriter.dir/src/relationships.c.o CMakeFiles/xlsxwriter.dir/src/rich_value.c.o CMakeFiles/xlsxwriter.dir/src/rich_value_rel.c.o CMakeFiles/xlsxwriter.dir/src/rich_value_structure.c.o CMakeFiles/xlsxwriter.dir/src/rich_value_types.c.o CMakeFiles/xlsxwriter.dir/src/shared_strings.c.o CMakeFiles/xlsxwriter.dir/src/styles.c.o CMakeFiles/xlsxwriter.dir/src/table.c.o CMakeFiles/xlsxwriter.dir/src/theme.c.o CMakeFiles/xlsxwriter.dir/src/utility.c.o CMakeFiles/xlsxwriter.dir/src/vml.c.o CMakeFiles/xlsxwriter.dir/src/workbook.c.o CMakeFiles/xlsxwriter.dir/src/worksheet.c.o CMakeFiles/xlsxwriter.dir/src/xmlwriter.c.o CMakeFiles/xlsxwriter.dir/third_party/tmpfileplus/tmpfileplus.c.o  -Wl,-rpath,/usr/local/lib:  -lz  /usr/local/lib/libminizip.so  -lcrypto && :

[jmcnamara]

It hardlinks libminizip and library paths gets omitted but it works as long as you already have paths in place

CURRENT

...  -L/usr/lib  -L/lib  -lz  -L/usr/local/lib  -lminizip  -L/usr/lib  -lcrypto && :

PATCH

...  -Wl,-rpath,/usr/local/lib:  -lz  /usr/local/lib/libminizip.so  -lcrypto && :

That is probably okay but not great. Let me have another look.

[dg0yt]

You may use <Pkg>_LIBRARIES but then you also have to apply the <Pkg>_LIBRARY_DIRS.

[jmcnamara]

That is probably okay but not great. Let me have another look.

I had a look at this and the output is the same in each case according to LDD.

However, I have updated the fix to just avoid using pkgconf when compiling for MSVC. This seems the simplest and in line with the vcpkg fix that @dg0yt proposed.

I'm presuming that I don't also need to add extra sentinel checks like this?:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 969e3808..49b6daa4 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -311,17 +311,17 @@ target_sources(${PROJECT_NAME}
     PRIVATE ${LXW_SOURCES}
     PUBLIC ${LXW_HEADERS}
 )
-if(ZLIB_LDFLAGS)
+if(NOT MSVC AND ZLIB_LDFLAGS)
     target_link_libraries(${PROJECT_NAME} LINK_PUBLIC ${ZLIB_LDFLAGS})
 else()
     target_link_libraries(${PROJECT_NAME} LINK_PUBLIC ${ZLIB_LIBRARIES})
 endif()
-if(MINIZIP_LDFLAGS)
+if(NOT MSVC AND MINIZIP_LDFLAGS)
     target_link_libraries(${PROJECT_NAME} LINK_PUBLIC ${MINIZIP_LDFLAGS})
 else()
     target_link_libraries(${PROJECT_NAME} LINK_PUBLIC ${MINIZIP_LIBRARIES})
 endif()
-if(LIBCRYPTO_LDFLAGS)
+if(NOT MSVC AND LIBCRYPTO_LDFLAGS)
     target_link_libraries(${PROJECT_NAME} LINK_PUBLIC ${LIBCRYPTO_LDFLAGS})
 else()
     target_link_libraries(${PROJECT_NAME} LINK_PUBLIC ${LIB_CRYPTO} ${OPENSSL_CRYPTO_LIBRARY})

[dg0yt]

BTW this is not marked REQUIRED.

[jmcnamara]

That is intentional. For me, at least, it isn't a requirement. @diizzyy can comment on whether it is a requirement or a nice to have.

I've always presumed that Cmake just does the same thing via findpackage and additional logic/searching. Unlike meson which (afaik) relies on pkgconf more (at least on Unix).

[dg0yt]

So the original intention might have been:

Suggested change

	else()
	endif()
	if(NOT ZLIB_FOUND)

(and similar below).

[jmcnamara]

Yes. You are probably correct that was the intention: in case pkgconf is available but still doesn't find the required lib.

@diizzyy was that the intention?

[diizzyy]

The intention was to mimic the logic already in use as close as possible, elseif is "new" syntax according to CMake's documentation and else with args will probably be deprecated at some point (tm). Given that target is still ancient versions I went for the old variant. I didn't define REQUIRED as that would prevent CMake from trying to find the requested library using the "fallback" logic.

Wrapping find_package(PkgConfig) seems to indeed be the easiest solution.

[jmcnamara]

I made some of the simplifying changes and updated the PR if folks would like to have a look.

Given that target is still ancient versions I went for the old variant.

I need to clean up the CMakeList.txt file and remove some of the cruft. Bumping to a more recent version of CMake should be part of that. Any suggestions on what is a reasonable version to support?

[jimwang118]

The variable _LINK_LIBRARIES is explained on the cmake official website as containing the library with the corresponding path, so directly linking this variable can accurately find the library.

[dg0yt]

elseif is "new" syntax according to CMake's documentation and else with args will probably be deprecated at some point (tm).

elseif is documented for CMake 2(!).6:
https://cmake.org/cmake/help/cmake2.6docs.html#command:elseif

else(<Condition>) is as confusing and pointless as endif(<Condition>).

Bumping to a more recent version of CMake should be part of that. Any suggestions on what is a reasonable version to support?

Recent versions of CMake warn when cmake_minimum_required(VERSION a...b) doesn't at least include 3.10.
Ubuntu 18.04 LTS had 3.9.
Ubuntu 20.04 LTS had 3.16.
More: https://repology.org/project/cmake/versions

[jmcnamara]

Thanks for the help folks. I have merged the changes to main.

I have some other small changes to make and they I will release another vcpkg version to close the issues/PRs there.

[dg0yt]

If you want to re-enable pkgconfig in the vcpkg port, it must also work for MSVC.
If you also adopt the use of Requires in the pc file, the port can drop the dependencies patch.

[jmcnamara]

If you want to re-enable pkgconfig in the vcpkg port, it must also work for MSVC.

Personally I don't want to enable it in vcpkg because I don't see (or maybe understand) the benefit. Are you saying that it would be better to enable pkgconfig in vcpkg?

If you also adopt the use of Requires in the pc file, the port can drop the dependencies patch.

Does pc here mean portfile.cmake? (https://github.com/microsoft/vcpkg/blob/master/ports/libxlsxwriter/portfile.cmake) or the generated .pc file.

Could you explain this a bit more.

[dg0yt]

If you want to re-enable pkgconfig in the vcpkg port, it must also work for MSVC.

Personally I don't want to enable it in vcpkg because I don't see (or maybe understand) the benefit. Are you saying that it would be better to enable pkgconfig in vcpkg?

vcpkg has working pkg-config also for Windows. One just has to be aware of using --msvc-syntax explicitly or implicitly.
minizip installs a pkg-config file, but it doesn't provide exported CMake config (outside vcpkg).
Using the same configuration approach across all platforms keeps things simple and consistent.

If you also adopt the use of Requires in the pc file, the port can drop the dependencies patch.

Does pc here mean portfile.cmake? (https://github.com/microsoft/vcpkg/blob/master/ports/libxlsxwriter/portfile.cmake) or the generated .pc file.

Could you explain this a bit more.

"pc file" refered to the module file for pkg-config, suffix .pc. ATM libxlxswriter may use zlib.pc to inform itself about the zlib link libraries. But it doesn't pass the information downstream, it just hardcodes an uninformed -lz. Which may work for must non-Windows installations, but it could fail there as well. The correct solution is to export Requires.private: zlib. And the same for minizip.

This is the patch I refered to:
https://github.com/microsoft/vcpkg/blob/master/ports/libxlsxwriter/dependencies.diff
The first chunk is making of vcpkg's unofficial CMake config for minizip.
The second chunk rectifies the installed pc file.