OpenXLSX icon indicating copy to clipboard operation
OpenXLSX copied to clipboard

CMake install command does not copy the external directory

Open fauder opened this issue 1 year ago • 15 comments

Hi,

I cloned the latest master today & built OpenXLSX with CMake. I'm not well versed in CMake but shouldn't it copy the external directory to the target install directory too?

fauder avatar Sep 06 '24 10:09 fauder

Doğrusu: I have not much of a clue about cmake at all - so I only ever used it to build executables - and the Makefile.GNU that you find in the repository was created by me so I don't have to deal with cmake all the time. Could you tell me what your use case is here?

As far as I understand, the "target" directory is the one where the libOpenXLSX.a binary ends up (on a make install) so that you can include it from other projects? Wouldn't that mean that there are multiple target directories, with another one for the include files to use the library?

So do I correctly understand that what you miss is that the header files from external are not being copied to the destination? ==> I think that might be because those are header-only libraries, and they are used "under the hood" - i.e. nowide, zippy and pugixml functions are not available directly to programs using OpenXLSX.

Is there any functionality that you are missing that is not working for you?

aral-matrix avatar Sep 09 '24 08:09 aral-matrix

@aral-matrix, hello. I have the same question.

I use the OpenSLX library, which is installed by the "cmake install" command. This command copies headers from the "OpenXLSX/headers" folder, but does not copy headers from the "OpenXLSX/external" folder. However, the "XLXmlParser.hpp" header includes the "external/pugixml/pugixml.hpp" file, and since the "external" folder is not copied, this causes a compilation error (the compiler cannot find such a file).

The line #include <external/pugixml/pugixml.hpp> was added to "XLXmlParser.hpp" file in commit https://github.com/troldal/OpenXLSX/commit/f07956dcfec106d85c8d281b50aa19030b4ff875, so the problem did not exist before that commit.

So I repeat the question asked by @fauder. Shouldn't CMake copy the external directory to the target install directory?

adkandrin avatar Oct 13 '24 13:10 adkandrin

Thank you @adkandrin for the explanation. Now I understood what the issue is. This is my fault, I made the XLXmlParser extend the pugi::xml_node by a lot of functionality, and didn't realize that somehow, this was previously working without an explicit include in the header / I didn't realize the implication when adding that. I will see if I can fix this so that the pugixml include can be hidden in the CPP file again.

Otherwise & just in case - would you happen to know enough about cmake to tell me how I can include this copy instruction for the external folder headers in the CMakeFile? I'd appreciate a pointer :)

aral-matrix avatar Oct 13 '24 14:10 aral-matrix

Sorry I could not get back to you on this.

@adkandrin summed it up well 👍

fauder avatar Oct 13 '24 17:10 fauder

@aral-matrix In the file "OpenXLSX/CMakeLists.txt" there is an instruction that copies headers from the "headers" folder:

file(GLOB OPENXLSX_HEADER_LIST ${CMAKE_CURRENT_LIST_DIR}/headers/*.hpp)
install(
        FILES ${OPENXLSX_HEADER_LIST}
        DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/OpenXLSX/headers
)

I think you can similarly add an instruction that copies files from the "external/pugixml" folder:

file(GLOB OPENXLSX_EXTERNAL_PUGIXML_FILE_LIST ${CMAKE_CURRENT_LIST_DIR}/external/pugixml/*)
install(
        FILES ${OPENXLSX_EXTERNAL_PUGIXML_FILE_LIST}
        DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/OpenXLSX/external/pugixml
)

All files are copied here, not just headers, because the file "pugihml.hpp" includes the file "pugixml.cpp".

Although it would be better, of course, to find a solution that hides external dependencies in the CPP-file.

adkandrin avatar Oct 13 '24 20:10 adkandrin

Although it would be better, of course, to find a solution that hides external dependencies in the CPP-file.

Sorry for not looking at this issue in a while - I am only now getting down in the backlog to look at older issues again - I will need a few days, if not weeks to think about a solution.

aral-matrix avatar Jan 06 '25 16:01 aral-matrix

Hi! I also encountered this problem when trying to connect OpenXLSX to my project on Windows as separated library. After two days of suffering, I managed to build the following "crutch".

When using OpenXLSX in a project, you need to do the following:

  1. If the OpenXLSX library has not yet been built via cmake, make sure that in the files
./OpenXLSX/sources/XLXmlParser.cpp
./OpenXLSX/headers/XLXmlParser.hpp

includes look like this:

#include "../external/pugixml/pugixml.hpp"
#include "XLException.hpp"

and ONLY after that compile the library via cmake

//--- FOR CONVENIENCE --\

  • run PowerShell as Administrator
  • go to the folder with the sources (OpenXLSX)
  • enter the commands:
mkdir build
cd build
cmake ..
cmake --build . --target OpenXLSX --config Release
cmake --install .

In my case library automaticaly installed in C:\Program Files (x86)\OpenXLSX.Library\. 2) Copy the OpenXLSX/external folder to your project's source folder

  1. In Visual Studio Community 2022:
  • open your project
  • select Project -> Properties
  • in Configuration Properties -> General in "C++ Language Standard" use ISO C++17 Standard (/std:c++17)
  • in C/C++ -> General, in "Additional Include Directories", add a line with a link to the external folder and to installed library For example:
C:\Program Files (x86)\OpenXLSX.Library\include
<my project folder>\source\external
  • (optional) in C/C++ -> Advanced in "Disable Specific Warnings" enter 4996
  • in Linker -> General, in "Additional Library Directories", add lines
C:\Program Files (x86)\OpenXLSX.Library\lib
<my project folder>\source\external
  • in Linker -> Input, in "Additional Dependencies", add "OpenXLSX.lib"
  • click "Apply", then "OK"
  1. In your code, include the library as #include <OpenXLSX/OpenXLSX.hpp>

After these manipulations library began to work correctly.

PS. I didn't check whole functional after this, so you can get another problems.

EvgenyEletskikh avatar Mar 04 '25 07:03 EvgenyEletskikh

hi @EvgenyEletskikh - thank you for the additional data points - as I wrote above I will look for a generic solution, that should make your life easier. But I am currently busy on a different project, so my apologies for the wait.

aral-matrix avatar Mar 04 '25 09:03 aral-matrix

Update! @fauder @EvgenyEletskikh @adkandrin

Although it would be better, of course, to find a solution that hides external dependencies in the CPP-file.

I think today I finally managed to properly hide the whole XLXmlParser module behind the OpenXLSX library interface.

I reworked the library (code review is still pending) to hide the pugixml header behind the wrapper header XLXmlParser.hpp and then only include the XLXmlParser from module files, but not(!) from header files.

This means that I was able to remove all pugixml include statements except for the XLXmlParser wrapper module (header and cpp files): 665310262031d8098069e1a299d9b931ac533a00 and ce2a3c99714976207c3d4ea040523f6dd23d34fe

I would appreciate very much if you could test the latest version on the development-aral branch and give me some feedback here whether this issue can be closed after all this time.

aral-matrix avatar Jul 14 '25 20:07 aral-matrix

@aral-matrix I'll try it soon (I hope) and get back to you. Thanks for your hard work!

fauder avatar Jul 18 '25 14:07 fauder

@aral-matrix I'll try it soon (I hope) and get back to you. Thanks for your hard work!

Teşekkür ediyorum :) Birkaç şeye daha bakmam lazım, ama yavaşça (ilk önce) debian için bir "package" hazırlamaya uğraşmaktayım.

aral-matrix avatar Jul 18 '25 16:07 aral-matrix

Hi, I'm upgrading my OpenXLSX installation and will test it now.

Is your dev branch development-aral safe for production work?

Current master is behind those commits you mentioned.

fauder avatar Sep 01 '25 12:09 fauder

The current development-aral branch is reasonably safe to use, I have performed a thorough review on the code that is affected by hiding pugixml as a library internal tool. Obviously, I can never guarantee that I did everything bug-free.

The one thing that currently does not work is automatic download & inclusion of libzip from a repository - but if you stick to the default settings, miniz will work just fine.

aral-matrix avatar Sep 01 '25 12:09 aral-matrix

I had similar problems reported by others in this issue. But branch development-aral fixes it! Thanks.

BTW, I created a Debian package by running:

echo 'include(CPack)' >> CMakeLists.txt

cmake -D CPACK_PACKAGE_CONTACT="email address" -D CPACK_DEBIAN_PACKAGE_MAINTAINER="Jacob Burckhardt <email address>" -D CPACK_PACKAGE_VERSION_MAJOR=2025 -D CPACK_PACKAGE_VERSION_MINOR=10 -D CPACK_PACKAGE_VERSION_PATCH=8-`git rev-parse HEAD` .

cmake --build . --target OpenXLSX

cpack -G DEB

If you want to officially release a Debian package this way, I think you can move the include(CPack) and the -D options into the CMakeLists.txt and change my name to the appropriate maintainer's name.

Jacob-Burckhardt avatar Oct 09 '25 01:10 Jacob-Burckhardt

I had similar problems reported by others in this issue. But branch development-aral fixes it! Thanks.

BTW, I created a Debian package by running: [..] If you want to officially release a Debian package this way, I think you can move the include(CPack) and the -D options into the CMakeLists.txt and change my name to the appropriate maintainer's name.

Thank you so much @Jacob-Burckhardt - it took a bit of fiddling until I got it working, but 7c1a23c400b935651044d5999393daa322ba3b54 implements your suggestions - it's a work in progress for now, because I need to correctly identify the package dependencies (and ideally build the .deb directly from CMake via an option), but you helped me make some good progress today!

aral-matrix avatar Oct 09 '25 11:10 aral-matrix