Conversation
|
Polite ping 👼 The current situation does not allow portable use of any modules with optional dependencies on other modules :( |
|
Sorry for the delay, I need to allocate some more time to review it properly. Generally I would to avoid such intrusive changes into the build, but I need to evaluate the benefit |
|
I understand. I tried to think of something cleaner, but in the end this seemed like a decent trade-off. The only other option I can think of is to try to detect these optional dependencies. But Or you could try to detect needed modules based on the namespace of types/constants in items from the generated modules. But this would requires that opencv actually uses sub-namespaces for their modules and strictly adheres to it. I'm pretty sure they don't. Maybe you could ask libclang to figure our which module a type comes from by checking which header declared it. But then you probably run into issues with forward declarations. And a header with the definition may not actually have been parsed by clang, unless you pre-preemptively index all supported modules. I suppose one more option is to detect opencv build-time enabled modules, and have a list of predefined optional dependencies and selectively generate the bindings only for enabled modules that are an optional dependency of one of the modules enabled by a feature flag (recursively). But this also feels rather icky: it will probably end up with a very opencv-version specific list, which is hard to verify and hard to test. And if the feature flags are essentially ignored, this means that the code written by users of this crate don't need to correctly specify the needed features, which also makes their code less portable ("it works on my machine"). (Maybe this could be addressed by generating the bindings, but making them private, so only the other bindings can use them, but not the user.) Anyway, the approach in this PR tries instead makes the crate features take on the function of the build time configuration of the system opencv. It is indeed intrusive to disable a header and take over it's function, but I do think it's actually less error prone (unless you end up building all build-time enabled modules) (until opencv changes the way they manage their Sorry for the wall of text, but if anything here triggers a good idea it will be worth it :) |
|
Did you perhaps have a change to think about the best way to deal with these optional module dependencies? |
@de-vri-es Can you please tell me if you encountered this error scenario in the actual code? In theory (unless there is a bug) the actual set of modules that the crate generates the bindings for is an intersection of what is chosen via cargo features and what actual modules OpenCV is compiled with. So for this scenario even if you enable DNN, the bindings for it won't be generated because OpenCV doesn't contain the |
Previously I solved these kind of problems with cargo feature dependencies like this. This way enabling one module will automatically enable the dependency. You lose some flexibility this way, e.g. you can't drop "dnn" even if you don't need the additional functionality, but it does help prevent the compilation errors in particular configurations. |
|
Sure, reproducing this is easy. You don't even need to write any code to trigger the build failure, since the compile error is in the It depends on your opencv build though, but with Arch Linux: $ cargo init
$ cargo add opencv --no-default-features --features objdetect,features
$ cargo buildGives: Enabling the That why I really think these optional module dependencies need special handling. |
|
Oh, hmm, I just read this bit:
🤔 I was assuming asking for a module that's not available would be an error. If not, enabling the feature does work. Then the only advantage of this PR is that it lets you build less bindings, which saves a bit of time. But then I'm not convinced it's worth it mysel 😄 So... should we just add make the /edit: See #725. And then I guess this can be closed :) |
This PR is an attempt to deal with optional inter-module dependencies.
For example, the
objdetectmodule optionally depends on thednnmodule. There is aCCheckerDetector::create(const dnn::Net &net)overload which is guarded behind#ifdef HAVE_OPENCV_DNN.The problem is:
HAVE_OPENCV_DNNis defined in a header<opencv2/opencv_modules.hpp>which is shipped with opencv itself. But the binding for thednnmodule (which includes thednn::Nettype) is not generated ifdnnfeature was not enabled.Right now this means that if your system opencv comes with the
dnnmodule, and you enable theobjdetectfeature but notdnn, you get compilation errors inside the opencv crate. On the other hand, if your system opencv doesn't come with thednnmodule, and you enable thednnfeature, you also get a compilation error. So if you want to useobjdetectand you don't care about thednnmodule, you have no good choice.This PR solves this by having all enabled module features become a
-DHAVE_OPENCV_$MODULEdefine for the code generation, and providing an alternative version of<opencv2/opencv_modules.hpp>without any defines in it.The end result: the optional code in
objdetectthat depends on thednnmodule will only be included in the bindings if thednnfeature is enabled \o/Additionally, the
objdetectmodule unconditionally depends on thefeaturesmodule, so I added that dependency inCargo.toml