luxonis / depthai-core

DepthAI C++ Library
MIT License
235 stars 127 forks source link

[Feature-Request] Add options to avoid installing some dependencies with Hunter #1021

Open traversaro opened 6 months ago

traversaro commented 6 months ago

Start with the why:

I am trying to integrated depthai-core in an existing C++ code base in which many dependencies (such as opencv and pcl) are managed via the conda package manager, installing packages from conda-forge. In this context, I would like to minimize the chance of packages installed by conda to conflict with packages installed via Hunter. As discussed in https://github.com/luxonis/depthai-core/issues/411, just setting HUNTER_ENABLE to OFF is not an option, as some dependencies are patched to work correctly with depthai-core, so you can't use the stock version. However, I would like to be able to use the conda version of dependencies that should work out of the box and I already used in the rest of the codebase, such as nlohmann_json, bzip2, spdlog and jsoncpp

Move to the what:

I would like to have a set of granular options to specify if hunter_add_package should be called or not for a package. Currently there is HUNTER_ENABLE to enable/disable hunter_add_package for all packages, but this can't work for the reason described in the previous section.

Move to the how:

My idea was to have options such as DEPTHAI_USE_EXTERNAL_<pkgname> for each pkgname . These variable will be mark_as_advanced and set to FALSE by default, to not interfere with existing workflow. If a given DEPTHAI_USE_EXTERNAL_<pkgname> option will be enabled, hunter_add_package(<pkgname>) will not be called, and the "normal" find_package(<pkgname>) will be used instead.

traversaro commented 6 months ago

I will be happy to work on a PR for this proposal, but I wanted first to check with maintainers if this is something you would be open to.

themarpe commented 6 months ago

Hi @traversaro

Thanks for bringing this up - and yes, it is something that I do view would be beneficial.

I lean towards Hunter actually, as it seems like the best place where this would be managed (& properly propagated to subdependencies as well)

See the (original) proposal: https://github.com/cpp-pm/hunter/issues/557

I will be happy to work on a PR for this proposal, but I wanted first to check with maintainers if this is something you would be open to.

We'd be very open for this - so if its in form of Hunter, we'd gladly switch to that build of Hunter with this functionality in.


WRT the depthai-ros - there is a branch, ros-release which "locally" already makes an attempt at handling this for nlohmann_json (but note, its just change on DAI level - if there was a dependency that was still under Hunter and it relied on the disabled nlohmann json, it would still be pulled, as it doesn't propagate - which is why I brought up the change in Hunter instead)

traversaro commented 6 months ago

Thanks for the quick reply! I see the point of transitive hunter dependencies, however in https://github.com/cpp-pm/hunter/issues/557 it does not seem that upstream is really interested in that feature, or to have Hunter interact with any other package manager from what I understand.

To be completely honest, I never used Hunter, so clearly for me it is tricky to estimate the effort to actually get a fully fledged feature in Hunter itself, and I am not sure I will be able to look into that in the near future. On the other side, I see why you would like to have this in Hunter and not as local options in depthai-core.

themarpe commented 6 months ago

@traversaro

however in https://github.com/cpp-pm/hunter/issues/557 it does not seem that upstream is really interested in that feature, or to have Hunter interact with any other package manager from what I understand.

I don't think they'd be against it per-se. Also, it functions similarly to HUNTER_ENABLE=OFF, but specifically.

IMO the approach would be much simpler going from Hunter side than from DAI side and we can rely on this fork of Hunter directly.

Let me know if you'll happen to check it out - would be interesting for a couple of integration paths (Yocto, ROS, ...)

traversaro commented 6 months ago

I don't think they'd be against it per-se.

To keep it simple I just asked this to the mantainers in https://github.com/cpp-pm/hunter/issues/557#issuecomment-2110378565 .

themarpe commented 6 months ago

To keep it simple I just asked this to the mantainers

Even better! Thanks!

themarpe commented 5 months ago

@traversaro any updates from the Hunter's side of approach? If you fork works as expected, we could potentially utilize that as well, if upstream won't be too fast on merging

traversaro commented 5 months ago

@traversaro any updates from the Hunter's side of approach? If you fork works as expected, we could potentially utilize that as well, if upstream won't be too fast on merging

Sorry for the radio silence. As mentioned in https://github.com/cpp-pm/hunter/issues/557#issuecomment-2112591832, I tried to impement the feature in way that handled corrected caching, but after spending a few hours on this, I could not find a solution, probably also due to my limited knowledge of Hunter's internals. Instead, I quickly implemented the solution with a modification of depthai-core side, and it has been working fine (see https://github.com/traversaro/depthai-core/commit/649841af39b166f4b35f41f916e9c8d384442d7b), so I am currently using that as a local patch.