Improve portability for external pugixml

[bansan85]

I ported openxlsx to vcpkg. I needed to remove local 3rd party (pugixml and nowide).

I needed to patch some part of OpenXLSX to use pugixml without the .cpp source.

If you are interested, you may take a look at https://github.com/microsoft/vcpkg/pull/46633/files (I didn't create pull request because I noticed you don't like it 😉 )

[aral-matrix]

Hi @bansan85 - it appears that our works have overlapped - in the development-aral branch, I am preparing a new version that has the kind of portability for use of a pugixml library on the system which I believe you also needed.

Would you have a look at the development branch?

I need to finish my code review before I can merge it into master, this is why it's not been merged yet: The use of forward declarations (similar to pImpl idiom) introduced some pitfalls where I need to move defaulted constructors, destructors and assignment operators from the header files. And since this only triggers a compiler error when a class is instantiated, and the demos do not instantiate all classes of the library - I need to do this thorough code review.

Nevertheless, the internal use of pugixml is now hidden from the library API, and the CMakeLists.txt now has an option OPENXLSX_ENABLE_LIBPUGIXML to also compile OpenXLSX against libpugixml. Also, use of nowide should be hidden in the headers/detail/OpenXLSXFileSystemTools.hpp functions, which is not exposed to the library API.

(I didn't create pull request because I noticed you don't like it 😉 )

Commendable, thank you very much ;)

[bansan85]

I didn't build, I just read the CMakeLists.txt changes.

Why don't you use find_package instead of

        target_link_libraries(OpenXLSX
                PRIVATE
                -lpugixml)

because you'll miss its include path with target_include_directories.

I checked all other changes, you already made them :)

[aral-matrix]

Why don't you use find_package instead of

Short answer: Because I am really clueless in terms of cmake - the cmake configuration was initially created by @troldal and I have been "learning by doing" when trying to keep changes in source code / linking logic working in cmake.

Can you tell me how that line should look like in your opinion? I'll try it out then.

[bansan85]

I cloned the dev branch.

You should update the embedded Catch library. I couldn't build the tests due to catchorg/Catch2#2421.

I also couldn’t build the tests (textSLCell.cpp) because of the use of a deleted copy constructor.

To use find_package, do the following (See https://github.com/microsoft/vcpkg/pull/46633/files for the full example):

diff --git a/OpenXLSX/CMakeLists.txt b/OpenXLSX/CMakeLists.txt
index 9e6b0e3..09ea155 100644
--- a/OpenXLSX/CMakeLists.txt
+++ b/OpenXLSX/CMakeLists.txt
@@ -58,7 +58,7 @@ if (NOT OPENXLSX_ENABLE_LIBZIP)
 endif ()

 if (OPENXLSX_ENABLE_LIBPUGIXML)
-    # nothing to do: pugixml headers should already be in include path, and libpugixml is added in the library link statements below
+    find_package(PugiXML REQUIRED)
 else()
     add_library(PugiXML INTERFACE IMPORTED)
     target_include_directories(PugiXML SYSTEM INTERFACE $<BUILD_INTERFACE:${CMAKE_CURRENT_LIST_DIR}/external/pugixml/>)
@@ -155,8 +155,8 @@ if ("${OPENXLSX_LIBRARY_TYPE}" STREQUAL "STATIC")
             $<BUILD_INTERFACE:${CMAKE_CURRENT_BINARY_DIR}>)     # For export header
     if (OPENXLSX_ENABLE_LIBPUGIXML)
         target_link_libraries(OpenXLSX
-                PRIVATE
-                -lpugixml)
+                PUBLIC
+                pugixml::pugixml)
     else()
         # pugixml is used in header only mode - so whatever this does...
         target_link_libraries(OpenXLSX
@@ -199,8 +199,8 @@ if ("${OPENXLSX_LIBRARY_TYPE}" STREQUAL "SHARED")
             $<BUILD_INTERFACE:${CMAKE_CURRENT_BINARY_DIR}>)     # For export header
     if (OPENXLSX_ENABLE_LIBPUGIXML)
         target_link_libraries(OpenXLSX
-                PRIVATE
-                -lpugixml)
+                PUBLIC
+                pugixml::pugixml)
     else()
         # pugixml is used in header only mode - so whatever this does...
         target_link_libraries(OpenXLSX

And also update OpenXLSXConfig.cmake (you need to add find_dependency(... CONFIG) in this file when your CMakeLists.txt will use find_package(... REQUIRED)):

include(CMakeFindDependencyMacro)
if(@OPENXLSX_ENABLE_LIBPUGIXML@)
    find_dependency(pugixml CONFIG)
endif()
if(WIN32)
    find_dependency(nowide CONFIG)
endif()

include("${CMAKE_CURRENT_LIST_DIR}/OpenXLSXTargets.cmake")

And update CMakeLists.txt:

 configure_file(OpenXLSXConfig.cmake
         "${CMAKE_CURRENT_BINARY_DIR}/OpenXLSX/OpenXLSXConfig.cmake"
        @ONLY
         )

install(
        FILES
        "${CMAKE_CURRENT_BINARY_DIR}/OpenXLSX/OpenXLSXConfig.cmake"
        "${CMAKE_CURRENT_BINARY_DIR}/OpenXLSX/OpenXLSXConfigVersion.cmake"
        DESTINATION ${ConfigPackageLocation}
)

You should add an option OPENXLSX_ENABLE_LIBNOWIDE and use find_package(nowide REQUIRED) / target_link_libraries(... PUBLIC nowide::nowide) and update OpenXLSXConfig.cmake.

When using target_link_libraries with a library target, always use PUBLIC for non-header-only dependencies. This is important for static linkage.

You may have an application that links to OpenXLSX, which needs pugixml. With static linkage, when linking the application against OpenXLSX, the linker will also need pugixml, which will be missing if you use a PRIVATE link.

In CMakeLists.txt, the variable dir is never set I think :

# Install interface headers
install(
        FILES ${OPENXLSX_CXX_INTERFACE_HEADERS}
        DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/OpenXLSX/${dir}
)

Let me know if everything is clear.

[aral-matrix]

Let me know if everything is clear.

Everything is not clear right now - whoa, that's a lot of changes. I will have a look and see if I can make sense of them. I don't want to just copy lines you suggest without knowing what they do. Did I mention I hate cmake? It's really hot garbage in terms of usability & documentation.

You should update the embedded Catch library. I couldn't build the tests due to catchorg/Catch2#2421.

To be honest, I haven't ever used that and the tests are really really outdated anyways, because in the past year, I added a lot of functionality to the library, and a lot has changed, and not a single test was updated or written newly. So there's not really any point in trying to compile the tests right now, if you ask me. I'll focus on the libpugixml recommendations first.

[bansan85]

No problem for the tests.

CMake is easy to start with, but takes many painful hours to master. It needs lots of workaround.

[aral-matrix]

I am currently working on first getting nowide out of the way, because your proposed solution doesn't align with the philosophy that OpenXLSX comes shipped with the required dependencies, and when adding compatibility for system installed libraries, the shipped source code modules should remain.
And the line

find_package(nowide CONFIG REQUIRED)

simply breaks my setup (when testing nowide), so I am going for a solution that ideally uses CPMFindPackage triggered by a TBD flag, while also being able to work directly with the boost nowide headers installed on the system.

[aral-matrix]

Hi again @bansan85,
would you be so kind to test the latest commit 39bd1b0 in terms of enabling an external dependency on boost nowide?

I added two new cmake options:

• added to cmake and GNU make: option OPENXLSX_FORCE_NOWIDE - set to ON to use force the use of nowide even on non-Windows systems (this flag is for testing purposes)
• added to cmake and GNU make: option OPENXLSX_ENABLE_LIBBOOST_NOWIDE - set to ON to use nowide from an installed libboost

I tried my best to integrate it with cmake as well as my own GNU Makefile. If this works as intended for your packaging attempts, then I will address the pugixml changes you ask for next.

[bansan85]

This looks good. You do:

if (OPENXLSX_ENABLE_NOWIDE)
    if (OPENXLSX_ENABLE_LIBBOOST_NOWIDE)
        find_package(Boost 1.74 REQUIRED COMPONENTS nowide) # on Linux at least, this requires libboost-nowide-dev to be installed
    else()
        add_library(NoWide INTERFACE IMPORTED)
        target_include_directories(NoWide SYSTEM INTERFACE $<BUILD_INTERFACE:${CMAKE_CURRENT_LIST_DIR}/external/nowide/>)
    endif()
endif()

But for the linker, you do:

    if (OPENXLSX_ENABLE_NOWIDE)
        if (NOT OPENXLSX_ENABLE_LIBBOOST_NOWIDE)
            # boost nowide is header-only, a "library" only makes sense in the context of the nowide header files shipped with OpenXLSX
            target_link_libraries(OpenXLSX
                    PRIVATE
                    $<BUILD_INTERFACE:NoWide>)
        endif()
    endif ()

The linker won't know what to do if OPENXLSX_ENABLE_NOWIDE and if OPENXLSX_ENABLE_LIBBOOST_NOWIDE. You need to do like in CMake examples. I suggest:

    if (OPENXLSX_ENABLE_NOWIDE)
        if (OPENXLSX_ENABLE_LIBBOOST_NOWIDE)
            target_link_libraries(OpenXLSX
                    PRIVATE
                    Boost::nowide)
        else()
            # boost nowide is header-only, a "library" only makes sense in the context of the nowide header files shipped with OpenXLSX
            target_link_libraries(OpenXLSX
                    PRIVATE
                    $<BUILD_INTERFACE:NoWide>)
        endif()
    endif ()

[aral-matrix]

Done, thank you! I forgot that there are some versions of nowide that are not header-only. I had actually intended to leave the linker without instructions because I thought there was nothing to do.

I'll hopefully get around to implementing your pugixml suggestions for the CMakeFile in the next days.

[bansan85]

If nowide may not be header-only, you will probably need to use PUBLIC in target_link_libraries to allow static linkage.

[aral-matrix]

If nowide may not be header-only, you will probably need to use PUBLIC in target_link_libraries to allow static linkage.

Oh yeah that was something I did not understand in your original suggestion: Is this really an issue for functions that are hidden from the (OpenXLSX) library API?
I was under the impression that a statically linked OpenXLSX library would have the dependencies also linked into the library binary file, so that applications using OpenXLSX would not need the information about linkage of library dependencies?

Both nowide and pugixml (also the zip implementations IIRC) are hidden from the public library interface of OpenXLSX.

[aral-matrix]

Hi again! I commited 17aee0e which implements find_package for LibZip and PugiXML. However, I just wasted 3+ hours of my life trying to find out how the hell to tell cmake's find_package to ignore unneeded components of LibZip: Could you @bansan85 or anyone tell me how to tell find_package to shut up about missing components of LibZip that this library does not need at all? Relevant line is here: https://github.com/troldal/OpenXLSX/blob/development-aral/OpenXLSX/CMakeLists.txt#L60 - the QUIET option (and also OPTIONAL_COMPONENTS) turned out to be absolutely useless :(