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

Allow to use external miniz & pugixml #99

Closed
SpaceIm opened this issue Jun 16, 2021 · 6 comments
Closed

Allow to use external miniz & pugixml #99

SpaceIm opened this issue Jun 16, 2021 · 6 comments

Comments

@SpaceIm
Copy link

SpaceIm commented Jun 16, 2021

It would be nice to be able to use externally installed miniz & pugixml, vendoring is bad for package managers (ODR violation).

  • Move miniz & pugixml files in specific folders (vendor/miniz & vendor/pugixml for example)
  • edit most cpp files with #include <pugixml.hpp> instead of #include "detail/pugixml.hpp" (this modification is tedious for package managers)
  • Add 2 options in Meson & CMake files, like TMXLLITE_EXTERNAL_MINIZ and TMXLLITE_EXTERNAL_PUGIXML
  • Depending on option values, inject vendored deps or find & inject external deps.
@fallahn
Copy link
Owner

fallahn commented Jun 16, 2021

Hi, thanks for the feedback. This is a good idea.

I'd propose to simplify it a bit by making it something like TMXLITE_EXTERNAL_LIBS in which case you either use the supplied libraries or choose your own. This would also allow further configuration such as alternative zip libraries like zlib or zstdlib (for which there is already an issue) if users prefer.

@ldornbusch
Copy link
Contributor

I have build a ZLIB switch into my fork, but only for the code, I am not so firm with cmake..
see here for details:
ldornbusch@4d61d98

@fallahn
Copy link
Owner

fallahn commented Feb 14, 2023

If you're only using a preprocessor definition then cmake is quite easy to update. First you'd need a cmake variable somewhere near the top of the file:

SET(TMXLITE_EXTERNAL_LIBS FALSE CACHE BOOL "Should tmxlite use external zlib and pugixml libraries?")

which lets the user configure it to true or false. Then:

if(TMXLITE_EXTERNAL_LIBS)
    add_definitions(-DUSE_ZIP_LIB -DUSE_PUGIXML_LIB)
endif()

to add the preprocessor defs. You'll also need to tell cmake to find the libraries (probably the most complicated part depending on the library support for cmake)

if(TMX_EXTERNAL_LIBS)
    find_package(zlib)
    find_package(pugixml)

    #optionally do other stuff here if libs not found like pull from github/package manager/build from source
endif()

and then finally tell cmake where the include dirs and libraries to link are

if(TMX_EXTERNAL_LIBS)
    include_directories(${ZLIB_INCLUDE} ${PUGIXML_INCLUDE})
    target_link_libraries(${ZLIB_LIBRARIES} ${PUGIXML_LIBRARIES})
endif

Although the exact include/lib variables will depend on how cmake finds them. As for meson, I have no idea, I've never used it 😅

@ldornbusch
Copy link
Contributor

ok I will try this out but will take some time

@eli-schwartz
Copy link

The standard way to do this in meson is actually to delete:

  • all custom options
  • the local copies of the library
    and rely on the global builtin option: --wrap-mode=forcefallback.

You'd then use dependency() as the sole way to get either external dependency, and run meson wrap install pugixml && meson wrap install miniz -- this would install two small INI files describing where to download the sources from on demand.

See https://mesonbuild.com/Wrap-dependency-system-manual.html

@fallahn
Copy link
Owner

fallahn commented Apr 5, 2023

Oh, OK. I'm not really familiar with meson, support was kindly provided by @KingDuckZ and I modified it as best I knew how to work with this update. Thanks for the tip!

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

No branches or pull requests

4 participants