Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add conanfile #110

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Add conanfile #110

wants to merge 3 commits into from

Conversation

Ujifman
Copy link
Contributor

@Ujifman Ujifman commented Apr 28, 2022

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

Ujifman and others added 3 commits April 5, 2022 18:02
It looks like there is a bug because EXPAT_INCLUDE_DIR is set, but EXPAT_INCLUDE_DIRS is used. So expat paths were not visible while make running. Fixing name helped.
… for debug build on Windows

- Added optional include of conanbuildinfo.txt in project CMakeLists.txt
- Added conanfile.py with receipt to build conan package and written dependenices as conan packages
- Added conan build instruction to README.md
@brechtsanders
Copy link
Owner

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
Copy link
Contributor Author

Ujifman commented Apr 29, 2022

@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
Copy link
Owner

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
Copy link
Contributor Author

Ujifman commented Apr 29, 2022

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
Copy link
Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants