Add Expat as a subdirectory instead of external.

[mahge]

• This addresses Configuration of ThirdParty/Expat #6

• There was a dependency on a non-existent (yet) target expatex.
  CMake used to issue a developer warning

  CMake Warning (dev) at Config.cmake/fmixml.cmake:185 (add_dependencies):
  Policy CMP0046 is not set: Error on non-existent dependency in
  add_dependencies. Run "cmake --help-policy CMP0046" for policy details.
  Use the cmake_policy command to set the policy and suppress this warning.

  The dependency target "/home/mahge/fmi-library/build_cmake/CMakeCache.txt"
  of target "expatex" does not exist.

• This a problem for some build tools like "Ninja" which are more strict

• libexpat can instead be added as a subdirectory. Its build tree will be
  in <fmil_binary_dir>/ThirdParty/Expat/exapat-2.1.0.

• This also allows it to inherit common configurations (e.g. build type
  debug/release) from its parent FMI library settings.

[slietzau]

I just tested this and it worked perfectly. Any chance this gets merged?

[filip-stenstrom]

I just tested this and it worked perfectly. Any chance this gets merged?

Most likely not, since it makes the integration with expat tighter (part of sources instead of external). It would be preferable to keep it as an external library and fix cmake issues that prevent this.

[mahge]

@filip-stenstrom This does not move any source directories around. Everything stays where it is. It only changes the way you integrate expat using cmake. expat already has CMake support and you ship it with your sources. So there is probably no need to have it as an external project (I mean CMake's ExternalProject), you can just add the subdirectory to CMake like any other directory.

The description of the PR might make it sound like it moves things around. It only changes the build directory for expat. Which should not affect you or the user in any way. If you check the diff you can probably see what the change is actually doing.

[slietzau]

@filip-stenstrom A different approach would be to use cmake's FindEXPAT script like this:

find_package(EXPAT)

Then you could check if expat was found (EXPAT_FOUND). If it's not found you could build the sources you ship.
As an added bonus, this would decouple fmi-library from expat and allow consumers to supply their own version for dependencies.

For example I'm currently creating a conan package of fmi-library. However it is essential that conan knows about dependencies. Additionally conan must be able to provide dependencies. For fmi-library this is not possible because all dependencies are built together with the project.
So for the conan package I would have to patch the fmi-library sources and remove the build instructions for the 3rd party libraries and replace them with find_package(...) calls. If expat would already discover an available version and only build if it wasn't available, that would help a lot.

[filip-stenstrom]

@slietzau Using find_package might be a way forward. We'll investigate it.

@mahge What I meant was that add_directory will treat it as any other source for FMIL. For example we don't necessarily want to propagate the entire FMIL cmake environment to the expat build, but would rather like to decouple them.

[mahge]

I don't think changing it to subdirectory will prevent you from some level of decoupling. It already sets almost all C and linker flags the same as the parent project through EXPAT_SETTINGS. Any other extra flags that should be set or removed can be done so using something similar to

fmi-library/Config.cmake/fmixml.cmake

Lines 139 to 141 in cf00ebd

	SET(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -DXML_STATIC -DFMI_XML_QUERY")

or even better by setting

target_compile_definitions(expat PUBLIC XML_STATIC)
target_compile_definitions(expat PUBLIC FMI_XML_QUERY)
target_link_options(expat ...)
...

in the parent project after adding it as a subdirectory.

Personally, I think It will simplify your CMakefiles quite a bit.

FMIL (I assume) is used as a subproject by a lot of tools. If there is any improvement that can simplify that process, I think it should be done. The way it is now it warns about a lot of developer issues, some of which are actual issues when you use something other than a Makefiles generator (e.g. Ninja).

That said, I think you have, of course, the right to use any setup you prefer, as long as it works. However, it might be a good idea to fix the warnings since they are warnings for a reason. They do not work in some systems or setups.

[lochel]

This is super helpful, thanks @mahge 👍

@filip-stenstrom Is there any chance this gets merged?

[filip-stenstrom]

@lochel No, since the long-term plan is to allow Expat as an external package (via find_package). That approach should also solve the main problem reported in #6 since then Expat can be found during configuration.