kamchatka-volcano / figcone

Read JSON, YAML, TOML, XML or INI configuration by declaring a struct
Microsoft Public License
100 stars 2 forks source link

Unnecessary include directory #10

Closed MeanSquaredError closed 1 year ago

MeanSquaredError commented 1 year ago

Thank you for developing figcone, it works quite well.

It seems that when I use cmake's find_package() + target_link_libraries(), it adds one superfluous include library.

Sample cmake code:

find_package (figcone REQUIRED)
target_link_libraries (myprog PRIVATE figcone::figcone_formats)

I have installed figcone in /usr/local Then when cmake starts building, the g++ command line it looks like this:

g++ ... -isystem /usr/local/include/figcone_tree/detail/external ...

Essentially it means that the target figcone::figcone_formats has /usr/local/include/figcone_tree/detail/external listed in its INTERFACE_INCLUDE_DIRECTORIES (my program treats that directory as a system include directory because figcone::figcone_formats is a system target)

It seems that the target figcone::figcone_formats gets the superfluous directory from the target figcone::figcone_tree.

In turn figcone::figcone_tree puts /usr/local/include/figcone_tree/detail/external into INTERFACE_INCLUDE_DIRECTORIES After looking in /usr/local/lib64/cmake/figcone_tree/figcone_treeTargets.cmake it seems that the culprit is on line 63:

set_target_properties(figcone::figcone_tree PROPERTIES
  INTERFACE_COMPILE_FEATURES "cxx_std_17"
  INTERFACE_INCLUDE_DIRECTORIES "${_IMPORT_PREFIX}/include;${_IMPORT_PREFIX}/include/figcone_tree/detail/external"
)

${_IMPORT_PREFIX}/include/figcone_tree/detail/external is added to INTERFACE_INCLUDE_DIRECTORIES

I think that this directory is not needed for the building of any projects using figcone, as all figcone include files that reference includes in detail/external specify relative path to their current directory, e.g. :

/usr/local/include/figcone_tree/stringconverter.h does #include "detail/external/sfun/type_traits.h"

So they don't need /usr/local/include/figcone_tree/detail/external in their include path.

I am not sure how exactly to modify the figcone project to remove that directory, so I am not providing a patch

kamchatka-volcano commented 1 year ago

Hi, thanks for the detailed description and research of the problem.

As you noted, paths from the detail/ folder are included directly without relying on include paths. However, for some reason, the CMakeLists in figcone_tree does the opposite:

SealLake_HeaderOnlyLibrary(
        NAMESPACE figcone
        COMPILE_FEATURES cxx_std_17
        INCLUDES
            figcone_tree/detail/external
)

(which is the same as adding target_include_directories(figcone_tree INTERFACE figcone_tree/detail/external))

So the fix is trivial - the INCLUDES parameter should be removed from there.

I've already fixed it and incremented all dependent library versions, including figcone. Please update to v2.4.10.

MeanSquaredError commented 1 year ago

Thank you!