brechtsanders / xlsxio

XLSX I/O - C library for reading and writing .xlsx files
MIT License
421 stars 112 forks source link

Add conanfile #110

Open Ujifman opened 2 years ago

Ujifman commented 2 years ago

Added conanfile.py for building as Conan package, with linked dependencies It is good idea to push xlsxio to center.conan.io in future as Conan package

brechtsanders commented 2 years ago

FindEXPAT.cmake is part of CMake, why do you rewrite it in your pull request?

Also, what is the reason the line:

INCLUDE_DIRECTORIES(${EXPAT_INCLUDE_DIRS})

was changed to:

INCLUDE_DIRECTORIES(${EXPAT_INCLUDE_DIR})

As for Conan, I don't have any experience with that (yet), but I'm okay with that if you have tested your files on different platforms supported by Conan.

Ujifman commented 2 years ago

@brechtsanders

  1. I took FindEXPAT.cmake from CMake distrib and modified it, because by default it cannot find expat lib with d suffix (debug build on Windows), so I added this opportunity
  2. I changed EXPAT_INCLUDE_DIRS on EXPAT_INCLUDE_DIR, because if you build without Conan, then CMake build fails, because FindEXPAT creates EXPAT_INCLUDE_DIR variable, and your CMakeLists looks for EXPAT_INCLUDE_DIRS variable
brechtsanders commented 2 years ago

Wouldn't it make more sense to report to the CMake developer(s) that FindEXPAT.cmake needs some tweaking for debug builds on Windows?

FindEXPAT.cmake contains:

if(EXPAT_FOUND)
  set(EXPAT_LIBRARIES ${EXPAT_LIBRARY})
  set(EXPAT_INCLUDE_DIRS ${EXPAT_INCLUDE_DIR})

so if Expat is found EXPAT_INCLUDE_DIRS is definitely defined. Is it possible there was another issue?

Ujifman commented 2 years ago

I'l try to explain chronologically. At first is tried to build xlsxio manually (v0.2.31).

  1. I found dependencies in Conan Center, installed and built them. expat, minizip, zlib
  2. Then I executed CMake configure, and filled out EXPAT_DIR, MINIZIP_DIR, ZLIB_DIR variable. I've used CMake GUI on Windows
  3. Then I executed CMake configure again. minizip and zlib were successfully found, but expat still showed error image
  4. I started research to find out why it is still not found, and concluded, that CMake expat module not works, because it is not standart location, but in xlsxio CMakeLists.txt, one uses find_path and write value to variable EXPAT_INCLUDE_DIR but then includes variable EXPAT_INCLUDE_DIRS, and that is the problem, why CMake cannot find expat image
  5. Then I fixed variable name in include to EXPAT_INCLUDE_DIR and successfully built xlsxio

On this point i created first pull request


After that I created Conan package, for my purposes and decided that it will be usefull for somebody. When I started to build it through Conan, I found out that Conan uses default find_package scripts of CMake. And on Windows expat builded in Debug mode has name libexpatd.dll, with d suffix, and default CMake find_package cannot recognize library. Thats why I added modified FindEXPAT.cmake here, for Conan builds to be able to find expat.

brechtsanders commented 4 months ago

108 already fixes CMakeLists.txt to use EXPAT_INCLUDE_DIR instead of EXPAT_INCLUDE_DIRS