Rework dependencies to be dynamic, i.e. can be overridden when using from install (+ fix when they's targets) #255

ferdonline · 2019-12-03T20:26:01Z

In order to be used by other projects, HighFive exports targets.
However two problems arose recently:

With recent HDF5 a package-config is provided and it sets HDF5_C_LIBRARIES & friends to target names. Since these targets aren't (can't) be exported alongside, projects depending on
HighFive will fail to build saying "library doesnt exist hdf5::hdf5-shared"
In some setups, the installation is done in a separate environment than the environment being used (e.g. conda). Therefore the target deps must be ignored and recomputed (Boost, CMake, conda-forge #253)

This PR

Tests several attempts on how to deal with such situations.

~~Use HDF5_C_[SHARED]_LIBRARIES instead - Doesn't work: suffers from the same problem~~
If the MPI_C_LIBRARIES is a target, simply finds the target location and uses the hard string for the interface libraries. This works but ends up locking the user to the detected hdf5 library. If the user later chooses to use a third party hdf5 directly in his application we end up linking to two different hdf5.
DoFindPackage within highfiveConfig.cmake. This allows the targets to exist to used from HighFive/directly user app if desired, but also allows the user to find dependencies himself and highfive will use it.

Extra

By searching the dependencies in the PackageConfig.cmake, we effectively enable the use case that the end project is free to choose other versions of the libs, which is desirable since this is a header-only project.
Fixes #253

By searching hdf5 in package-config minimal changes have to be employed and we enable using alternate hdf5s Adding parallel option to highfive when imported Improvements. Added developer doc Exports

tristan0x · 2019-12-05T06:26:21Z

Could you tell me in which version HDF5 started to ship a package-config so that I can test the different scenario?

tdegeus · 2019-12-05T08:41:42Z

@ferdonline Incidentally, I started playing with a solution for #253 yesterday. What I have so far (#257 ) is I think a nice draft to deal with #253, but also the issues being dealt with here.

ferdonline · 2019-12-05T10:13:52Z

Could you tell me in which version HDF5 started to ship a package-config so that I can test the different scenario?

I installed hdf5-parallel from brew. It's still 1.10.5

We now create an INTERFACE target to hold dependencies info This target is created on every configure, from HighFive itself or any using project, trigerred from HighFiveConfig.cmake

ferdonline · 2019-12-05T18:12:48Z

ATM Travis internet is having issues. To be retested

tdegeus

I'm happy to see changes in the spirit of #257 and that also go in the direction of fixing #253 !

tdegeus · 2019-12-05T18:23:24Z

CMake/HighFiveTargetDeps.cmake

+if(USE_BOOST)
+  find_package(Boost REQUIRED COMPONENTS system serialization)
+  target_link_libraries(highfive_deps INTERFACE Boost::boost Boost::serialization)
+  target_include_directories(highfive_deps INTERFACE ${Boost_INCLUDE_DIR})


If I'm not incorrect Boost::boost takes care of the include. I don't think there is need for the target_include_directories (but it would be good to check)

I reverted to not using boost targets for support for cmake < 3.5.
Next main release we bump cmake requirements and start using targets.

I guess that is fine.

One could now have both implementations based on CMake version. Those of use using a new CMake version and that want to pick dependencies carefully now are left with no choice to use old-school behaviour.

tdegeus · 2019-12-05T18:24:01Z

CMake/HighFiveTargetDeps.cmake

+target_compile_definitions(highfive_deps INTERFACE ${HDF5_DEFINITIONS})
+
+# Boost
+if(USE_BOOST)


It would be good the use HIGHFIVE_USE_BOOST to avoid any name conflicts with other libraries

I agree. It's a breaking change though. Let's do that in the next main release

You could ease the transition by externally allowing both and internally using HIGHFIVE_USE_BOOST. One could even add a deprecation warning.

CMake/HighFiveTargetDeps.cmake

CMake/HighFiveConfig.cmake.in

tdegeus · 2019-12-06T07:41:25Z

It is great that you are moving to a way to specify options at compile time (not only at install time). I was thinking though, wouldn't it be great to also allow the user access to the individual components? To select options one could then do either of two things:

The current PR's approach

set(USE_BOOST FALSE)
find_package(HighFive)
target_link_libraries(mylib HighFive)

Even more customisation if desired

EDIT: I have changed the names in the hope to be more convincing. I really think that having this possibility would really be very nice. I think it is nice that boost, eigen, and xtensor remain fully optional in the library itself. I think the same spirit should hold for the CMake build system.

find_package(HighFive)
target_link_libraries(mylib 
    HighFive::HighFive 
    HighFive::hdf5 
    HighFive::use_boost  
    HighFive::use_eigen 
    HighFive::use_xtensor)

or maybe even

find_package(HighFive COMPONENTS HighFive hdf5 boost eigen xtensor)
target_link_libraries(mylib 
    HighFive::HighFive 
    HighFive::hdf5 
    HighFive::use_boost  
    HighFive::use_eigen 
    HighFive::use_xtensor)

ferdonline · 2019-12-06T12:31:45Z

In the current PR the target is that we do

find_package(HighFive)
target_link_libraries(mylib HighFive)

The user will probably want the options with what HighFive was installed, so I hope set(USE_BOOST FALSE) will not be required often. Worse case an unnecessary link happens.

Now he still is free to do

find_package(Boost COMPONENT XYZ)
find_package(HighFive)
target_link_libraries(mylib HighFive)

And the mylib will use the user found Boost, without the need of adding HighFive::boost - I actually don't like that, boost is not a component of HighFive, so it shouldn't look like one)
Of course if the user requires boost directly in his code, he should do (and it should work)

target_link_libraries(mylib HighFive boost::xyz)

tdegeus · 2019-12-06T12:59:40Z

The advantage of allowing the use of disentangled targets is that you could partly customise by replacing some HighFive's targets (let's say HighFive::hdf5) if for a reason that we did not foresee some use-case. Mostly I would like to argue to have at least HighFive::HighFive with just HighFive headers. Then at least one could take the bare minimum in weird case where the proposed build system fails.
The advantage of having HighFive::boost but also HighFive::eigen and HighFive::xtensor is that you immediately take care of the define H5_USE_BOOST, H5_USE_EIGEN, or H5_USE_XTENSOR. It should thus not be interpreted as Boost/Eigen/xtensor being a part of HighFive, but of unlocking functionality in HighFive for the use with Boost/Eigen/xtensor. But it is true that the user can also simply for the defines (or include in the right order), so it is not a very urgent functionality.

tdegeus · 2019-12-06T13:44:33Z

@ferdonline Currently the C++ version is not set...

In file included from /Users/tdegeus/miniconda3/envs/hf/include/highfive/H5Exception.hpp:17:
/Users/tdegeus/miniconda3/envs/hf/include/highfive/bits/H5Utils.hpp:36:12: error: unknown type name 'constexpr'
    static constexpr size_t value = 0;
           ^

........

WINDOWS: Dont auto-link

tdegeus · 2019-12-09T10:57:06Z

@ferdonline Happy to test again when you're ready. Just ping me.

tdegeus · 2019-12-09T13:52:15Z

CMake/HighFiveTargetDeps.cmake

+  target_include_directories(highfive_deps SYSTEM INTERFACE ${Boost_INCLUDE_DIR})
+  target_compile_definitions(highfive_deps INTERFACE BOOST_ALL_NO_LIB H5_USE_BOOST)
+endif()
+


I guess that (in the future) one could consider to act on HIGHFIVE_USE_EIGEN and HIGHFIVE_USE_XTENSOR here.

Yeah, at least a separate PR...

ferdonline · 2019-12-09T14:11:15Z

@tdegeus please go ahead. I hope we dont have to change it much further

tdegeus · 2019-12-09T14:19:47Z

@ferdonline To what should I set HIGHFIVE_USE_INSTALL_DEPS? With the default there are absolute paths in the highfive_deps target (in the installed HighFiveTarget.cmake):

set_target_properties(highfive_deps PROPERTIES
  INTERFACE_COMPILE_DEFINITIONS "_FORTIFY_SOURCE=2;BOOST_ALL_NO_LIB;H5_USE_BOOST"
  INTERFACE_COMPILE_OPTIONS ""
  INTERFACE_INCLUDE_DIRECTORIES "/Users/tdegeus/miniconda3/envs/test/include;/Users/tdegeus/miniconda3/envs/test/include;${_IMPORT_PREFIX}/include"
  INTERFACE_LINK_LIBRARIES "/Users/tdegeus/miniconda3/envs/test/lib/libhdf5.dylib;/usr/lib/libpthread.dylib;/Users/tdegeus/miniconda3/envs/test/lib/libz.dylib;/usr/lib/libdl.dylib;/usr/lib/libm.dylib"
  INTERFACE_LINK_OPTIONS ""
  INTERFACE_SYSTEM_INCLUDE_DIRECTORIES "/Users/tdegeus/miniconda3/envs/test/include;/Users/tdegeus/miniconda3/envs/test/include"
)

In addition there is again the H5_USE_BOOST in that target.

tdegeus · 2019-12-09T15:22:58Z

@ferdonline I found the solution to your problem. In the end it boils down to that you should take care not to export your highfive_deps to the HighFiveTargets.cmake as it will include the state it has. Instead you should only set highfive_deps at the moment the user call fiind_package. Removing the export of highfive_deps was a bit less transparent because of the many includes, but I seem to have managed also that.

What is a good way to propagate the changes? Can I open a PR to your branch? How? In the worst can I can paste my changes here.

ferdonline · 2019-12-09T17:23:14Z

@tdegeus This was a time-consuming ticket where I explored those ideas. It doesnt work since cmake requires to export highfive_deps as well, otherwise it complains HighFive INTERFACE is not valid. Further, with central deployments we indeed want to use the original flags, so it came in handy to have highfive_deps populated and exported.
What we do is, in case that is not desirable, which we put as default, is to erase the properties and recreate them.
If you have a better solution I'd be glad to hear.

tdegeus · 2019-12-09T17:42:01Z

@ferdonline I think your approach is the right one. I agree with using a highfive_deps target. The only thing is: it should not be exported to HighFiveTargets.cmake as it will make it completely system dependent, and that was exactly what you set out to solve. It should be fully constructed in HighFiveConfig.cmake (and through your convention in HighFiveTargetDeps.cmake) at the moment that the user calls find_package(HighFive). Once that is done, you are in business.

ferdonline · 2019-12-09T18:34:39Z

I used INTERFACE IMPORTED libraries and indeed that didnt require us to export the it, but then at import time there were problems. I would need to test it.
The main problem is, without exporting the original libraries, we lose compat in build systems which expect only highfive to look for its own dependencies, not later.
What is the problem you see in the current solution? Did you read the comment carefully? It doesnt matter what is exported, the target ::deps is conditionally erased and rebuilt.

tdegeus · 2019-12-10T08:54:16Z

@ferdonline I see what you want (though maybe you can explain me the relevant use-case for this). What I would do is to conditionally export the dependency target (and now I also see why you chose the name highfive_deps and not HighFive::deps). Please checkout my implementation below. It does what you want in case the you enable HIGHFIVE_USE_INSTALL_DEPS both at install and at compile time. Note that I've tested the implementation locally, so it should work for you without problems

tdegeus · 2019-12-10T08:57:59Z

~~(It would have been great if I could have committed to your PR, but sadly I don't know how to)~~

ferdonline · 2019-12-11T09:56:25Z

@tdegeus Please open a PR to this branch, otherwise it's really difficult to see whatsoever diff. Also code here it clutters a lot the thread, we cant leave it here.

tdegeus · 2019-12-11T09:59:01Z

~~@ferdonline Happy to! How do I do that?~~

tdegeus · 2019-12-11T13:01:25Z

See fix in #259

tdegeus · 2019-12-20T09:49:57Z

@alkino One should still merge #259 here.

ferdonline · 2020-01-10T14:19:35Z

@tdegeus After checking with other maintainers we are not going via the two cmake exports as it was a more complex scenario and seemed less flexible. Improvements can be done later if needed.

tdegeus · 2020-01-10T14:20:26Z

@ferdonline So the conda-forge package remains broken??

ferdonline · 2020-01-10T14:21:01Z

We will be merging this PR. It was working for conda, right?

tdegeus · 2020-01-10T14:28:15Z

It was a while that we discussed this. In my memory: I don't think so in fact. The whole point is that the target generated here always contains absolute paths found at install time (i.e. on conda's CI). These paths typically do not exists for the end-user, resulting in linker errors. Solving this was the only point of my PR.

I will double check tonight and point you to the details.

ferdonline · 2020-01-10T14:33:03Z

Yep, this PR changed a lot, but in the end it aimed to solved exactly that problem. And you see in the import cmake file that we erase the target properties and recreate them dynamically if that's the user choice. It hopefully solves the conda problem! I let you test then before we merge.

tdegeus · 2020-01-10T18:14:51Z

@ferdonline You seem to be right, I guess I understood incorrectly! Go ahead and match it, we can always re-discuss if things don't end up working.

tdegeus · 2020-01-13T12:47:17Z

@ferdonline Can you also release a patch such that we can release a conda-forge package update?

tdegeus · 2020-01-20T09:58:38Z

@ferdonline I can confirm the Conda is working as desired (conda-forge/highfive-feedstock#9)! Thanks a lot for the fix.

ferdonline added 2 commits December 3, 2019 20:46

Fix to HighFive-config.cmake to bring hdf5 lib loc

66c6cd6

Alternative way

df85ab5

ferdonline requested a review from alkino December 3, 2019 20:26

ferdonline force-pushed the build_find_hdf5 branch from 5234231 to 80af9d5 Compare December 4, 2019 14:54

ferdonline requested a review from tristan0x December 4, 2019 22:26

ferdonline added 2 commits December 5, 2019 00:24

Method translating arget to lib location

be4d6c1

Make target always available, support alt hdf5

51418c7

By searching hdf5 in package-config minimal changes have to be employed and we enable using alternate hdf5s Adding parallel option to highfive when imported Improvements. Added developer doc Exports

ferdonline force-pushed the build_find_hdf5 branch from a141c32 to 51418c7 Compare December 4, 2019 23:27

ferdonline mentioned this pull request Dec 4, 2019

Boost, CMake, conda-forge #253

Closed

All flags from target. Same for boost. Cleanup

d211d89

ferdonline force-pushed the build_find_hdf5 branch 2 times, most recently from bc12284 to 82de660 Compare December 5, 2019 17:30

Reorganizing to detect dependencies on the fly

a4dff36

We now create an INTERFACE target to hold dependencies info This target is created on every configure, from HighFive itself or any using project, trigerred from HighFiveConfig.cmake

ferdonline force-pushed the build_find_hdf5 branch from 82de660 to a4dff36 Compare December 5, 2019 17:51

ferdonline closed this Dec 5, 2019

ferdonline reopened this Dec 5, 2019

tdegeus reviewed Dec 5, 2019

View reviewed changes

ferdonline closed this Dec 5, 2019

ferdonline reopened this Dec 5, 2019

tdegeus reviewed Dec 6, 2019

View reviewed changes

CMake/HighFiveConfig.cmake.in Outdated Show resolved Hide resolved

Polishing around, improve compat

bbb329c

Travis should fetch conda boost-cpp for binary lib

9b8843f

WINDOWS: Dont auto-link

ferdonline added 2 commits December 6, 2019 17:57

Fix to avoid H5_USE_BOOST to stay permanently

005c2ae

Added depend deploy mode. Prefix USE_x w HIGHFIVE_

5125dc9

tdegeus reviewed Dec 9, 2019

View reviewed changes

HIGHFIVE_USE_INSTALL_DEPS as an option with default

6220971

alkino approved these changes Dec 20, 2019

View reviewed changes

ferdonline merged commit 2f07be8 into master Jan 13, 2020

ferdonline deleted the build_find_hdf5 branch January 13, 2020 23:18

tdegeus mentioned this pull request Jan 14, 2020

Windows package pulling-in nonexistent Boost path conda-forge/highfive-feedstock#10

Closed

alkino mentioned this pull request Mar 10, 2020

Release 2.2 #303

Merged

Rework dependencies to be dynamic, i.e. can be overridden when using from install (+ fix when they's targets) #255

Rework dependencies to be dynamic, i.e. can be overridden when using from install (+ fix when they's targets) #255

Conversation

ferdonline commented Dec 3, 2019 • edited Loading

This PR

Extra

tristan0x commented Dec 5, 2019

tdegeus commented Dec 5, 2019

ferdonline commented Dec 5, 2019

ferdonline commented Dec 5, 2019

tdegeus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdegeus commented Dec 6, 2019 • edited Loading

The current PR's approach

Even more customisation if desired

ferdonline commented Dec 6, 2019

tdegeus commented Dec 6, 2019 • edited Loading

tdegeus commented Dec 6, 2019

tdegeus commented Dec 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ferdonline commented Dec 9, 2019

tdegeus commented Dec 9, 2019

tdegeus commented Dec 9, 2019 • edited Loading

ferdonline commented Dec 9, 2019

tdegeus commented Dec 9, 2019 • edited Loading

ferdonline commented Dec 9, 2019

tdegeus commented Dec 10, 2019 • edited Loading

tdegeus commented Dec 10, 2019 • edited Loading

ferdonline commented Dec 11, 2019

tdegeus commented Dec 11, 2019 • edited Loading

tdegeus commented Dec 11, 2019 • edited Loading

tdegeus commented Dec 20, 2019

ferdonline commented Jan 10, 2020

tdegeus commented Jan 10, 2020

ferdonline commented Jan 10, 2020

tdegeus commented Jan 10, 2020

ferdonline commented Jan 10, 2020

tdegeus commented Jan 10, 2020

tdegeus commented Jan 13, 2020

tdegeus commented Jan 20, 2020

ferdonline commented Dec 3, 2019 •

edited

Loading

tdegeus commented Dec 6, 2019 •

edited

Loading

tdegeus commented Dec 6, 2019 •

edited

Loading

tdegeus commented Dec 9, 2019 •

edited

Loading

tdegeus commented Dec 9, 2019 •

edited

Loading

tdegeus commented Dec 10, 2019 •

edited

Loading

tdegeus commented Dec 10, 2019 •

edited

Loading

tdegeus commented Dec 11, 2019 •

edited

Loading

tdegeus commented Dec 11, 2019 •

edited

Loading