spack: Add ${PUGIXML_INCLUDE_DIR} to target_include_directories

[bernhardkaindl]

@prudhomm

When pugixml is not installed in a standard location (e.g. use pugixml build by spack),
the include directory needs to be added to the target's include directories

See the PR for adding a feelpp recipe into spack at spack/spack#46396 (review) for more info.

A different way to do this would be like upstream does meanwhile:
https://github.com/NTNU-IHB/FMI4cpp/blob/71d30042e4044bdd12ccf9fa756046b801e46502/cmake/FindPugiXML.cmake#L26

See this for the diff:

master...NTNU-IHB:FMI4cpp:master#diff-2bd3d1ab5072e7e84c9bdea1f59038fda719facac5721f1ba2ee9a39eb139bfeR24-R28

Maybe it's better so sync / merge with the upstream?

[bernhardkaindl]

@prudhomm: FYI, this blocks adding a generally working feelpp to spack.

[prudhomm]

thanks I believe that it is best to push that to upstream. We do not want to diverge

[bernhardkaindl]

@prudhomm wrote:

thanks I believe that it is best to push that to upstream. We do not want to diverge

Maybe there is some misunderstanding (or maybe not), I don't yet know for sure, or in which direction.

To clarify and restart, I try to re-phrase what I tried to say:

In these changes in the upstream merge of NTNU-IHB#139, upstream improved the way(s) that upstream can find the path to the pugixml headers if they are not in a standard location like /usr/include, /usr/local/include or so:

https://github.com/NTNU-IHB/FMI4cpp/pull/139/files#diff-2bd3d1ab5072e7e84c9bdea1f59038fda719facac5721f1ba2ee9a39eb139bfeL8-R28

These changes set or provide PUGIXML_INCLUDE_DIR and try to export it as an interface to downstream packages like feelpp that may need it so their cmake can import those variables from the this package's generated cmake interface files of these downstreams (more or less, maybe I wrote this in a very complicated way).

If pugixml is installed into the host's default include search path, then downstreams could still compile, but in the case where I tried to build feelpp on a very pristine build host, it failed while reviewing the PR on feelpp.

For this reason, I tried to export it using this minimal patch here.

This may be used as mitigation (using a patch) in spack to fix the spack install of feelpp in this configuration, and you could also use it on your fork of FMI4cpp.

But as you say correctly, it's better not to diverge from the upstream https://github.com/NTNU-IHB/FMI4cpp.

I haven't tested the upstream change in spack, but it looks like the most modern way to fix this issue using the modern CMake way using cmake target-specific interfaces for downstream packages like feelpp.

Such modern CMake methods may not work in very old CMake versions, so you may need to raise the minimum CMake version requirement along with it if you update your fork of https://github.com/NTNU-IHB/FMI4cpp.
to a newer upstream commit. Unfortunately, https://github.com/NTNU-IHB/FMI4cpp hasn't tagged a new version since 2022, so you'd have to pick a new commit and rebase your fork on top of it (or merge).

I just wanted to point out what I understood what I saw while triaging this build issue.

The minimal one-line patch that I posted with this PR is only to provide a working fix for the current code base that I've tested to work in in the spack environment that reproduced the build issue.

If the cmake improvement in https://github.com/NTNU-IHB/FMI4cpp/pull/139/files#diff-2bd3d1ab5072e7e84c9bdea1f59038fda719facac5721f1ba2ee9a39eb139bfeL8-R28 works for a spack build, then you could use this instead, only if their https://github.com/NTNU-IHB/FMI4cpp/pull/139/files#diff-2bd3d1ab5072e7e84c9bdea1f59038fda719facac5721f1ba2ee9a39eb139bfeL8-R28 is not enough to fix a failing spack build on a freshly installed VM or host, then pushing that change to upstream would be needed.

The smallest change to fix the spack build would be to add this minimal change here as a local patch to the spack recipe that builds this fork. I hope I could do it, but there are so many spack PRs that would need review.

But, it would be good to know if you would approve of this one-line fixup patch (like spack carries such small fixups to fix builds in general) to be added to the spack build for fixing the build of spack/spack#46396 (review) that failed while reviewing the PR for feelpp in spack.

[bernhardkaindl]

I just saw: spack/spack#46396 (comment)

@bernhardkaindl thank you for your extensive work ! I made some other changes to support rocm, cuda, kokkos that I will push along side your propositions.

Great, I'll wait for your updates to be pushed!

[prudhomm]

@bernhardkaindl I believe we can simplify the CMake configuration by removing the explicit PUGIXML_INCLUDE_DIR from target_include_directories. Since we’re linking to pugixml::pugixml in target_link_libraries, CMake automatically handles the include directories associated with pugixml. This way, we rely on CMake’s target properties to manage dependencies, reducing redundancy and potential errors.

[prudhomm]

I just checked the pugixml installed by spack and that's what it does.
Now I need to check that pugixml::pugixml is properly added to target_link_libraries by fmi4cpp

[prudhomm]

@bernhardkaindl I just checked fmi4cpp and indeed libzip and pugixml are added privately via target_link_libraries
https://github.com/feelpp/FMI4cpp/blob/master/src/CMakeLists.txt#L104

[prudhomm]

@bernhardkaindl I created a package for fmi4cpp waiting for review here : spack/spack#47240

[prudhomm]

I also made a PR for fmi4cpp regarding the MPI addon to avoid unzipping possibly thousands of FMU in parallel
see NTNU-IHB#152
I also asked the author if he could make a release of fmi4cpp here NTNU-IHB#150