Move deps like tinyxml and jsoncpp to plugins

[devbharat]

Now that its possible to selectively enable/disable building plugins with the plugins.txt file, dependencies(like openssl, CURL, tinyxml and json) that are only needed by certain plugins can perhaps move to the needed plugin's own CMakeLists.txt file?

CMake also allows that. Perhaps this is too much of an optimization, but if someone just wants to use core library (and no plugins) downloading and installing these takes a bit of time and seems unnecessary.

[julianoes]

So, there is now an option to disable cURL. However, tinyxml2 and jsoncpp are going to get more important in many parts in a couple of component information features that are coming up, so I'm less motivated to move them.

[JonasVautherin]

Unless there is an actual use-case, I'd say we can keep it the way it is. We provide helpers to help build them, they can be linked statically, and they are really not big :+1:.

[devbharat]

Makes sense. The use-case is rather straightforward, say you just want to use mavsdk passthrough plugin (say for integration testing of another code-base) you shouldn't need much.

Or perhaps you'd want to add your own plugins and build on top of the "core" mavsdk outside the mavsdk repo (not sure if we already can or if it needs a bit more modification to enable such 'external' plugins, need to check) after including it as a library in another project, it seems weird to have to install dependencies for plugins you are not even building. But i agree tinyxml and jsoncpp are not too much. Feel free to close!

[julianoes]

What about the possibility to manually disable the dependencies like we did it for cURL?

I think that might be easier than having it flow out of the CMakeLists.txt configure step.

The problem I see is that if we move the find_package into the plugin's CMakeLists.txt then we also need to set some sort of variable to signal to third_party/CMakeLists.txt that it's not required. However, we run the third_party/CMakeLists.txt first, so we don't know yet at that time. And we have to run it first, otherwise the package won't be found later. :man_shrugging:

[julianoes]

This reminds me that we should actually merge: https://github.com/mavlink/MAVSDK/pull/1955.

[JonasVautherin]

What about the possibility to manually disable the dependencies like we did it for cURL?

My concern is that every such steps adds complexity for a specific use-case. Disabling curl is an answer to "what if I want only a subset of the camera plugin?". Now tinyxml and jsoncpp are for a subset of the other plugins. It's all adding branches in the build system and in the code that make it more convoluted.

My question would be: is this because people don't want to build the deps (curl admittedly takes quite some time to cross-compile), or is it because those deps don't build for their microcontroller?

For the former, I think it's a dependency management problem. I have seen people build MAVSDK on a Pi zero instead of cross-compiling, and complaining that it was slow. But I would argue that it's not MAVSDK's problem to handle this problem: they should learn how to cross-compile. Also dependencies only have to be built once, so people who are frustrated from building them regularly are doing something wrong.

For the latter, I completely understand the problem (though I am not sure if anybody is using MAVSDK on a microcontroller). There we could have one single option, say MAVSDK_LITE, that would remove whatever cannot run on a microcontroller.

Well, anyway for now it's really only for curl, but my point is that we should be careful not to keep adding too much complexity this way :innocent:.

dependencies(like openssl, CURL, tinyxml and json) that are only needed by certain plugins can perhaps move to the needed plugin's own CMakeLists.txt file?

This could actually work by just having the plugin's CMakeLists link the dependency it needs, right? No need for POLICY CMP0079, I would think :thinking:. So essentially move target_link_libraries from the core to the plugins that need it. I think it would be worth a try, because maybe it's really not that much work (essentially moving 3 target_link_libraries), and it could "just work" :+1:.

Also I would not change the helper (so the superbuild would build jsoncpp and tinyxml even if no plugins need them). If people don't want the extra build time, they can manage their dependencies themselves :+1: (instead of use adding CMake options that make the whole thing more convoluted).

@devbharat wanna give it a try? Just try to move those target_link_libraries and see what happens :innocent:

[devbharat]

@devbharat wanna give it a try? Just try to move those target_link_libraries and see what happens 😇

yes that works, I have it on a fork that way. I had to set the policy as it complained that the mavsdk target is not defined in that (plugin) directory.

I just wanted to get your opinion on how and if you wanted to ‚fix‘ this, and if there was a more elegant solution (because now you have to add it individually to each plugin that needs it).

I hadn’t seen the PR that @julianoes linked, for now I had just commented out cURL, so I think the PR helps there for sure.

Also dependencies only have to be built once, so people who are frustrated from building them regularly are doing something wrong.

If you are doing a local build and install of mavsdk with externalproject_add(), you’d need to clone & build for every fresh build, ccache helps but still, in a project my many deps if there are low hanging optimizations to get rid of some perhaps it makes sense, though I also agree we shouldn’t create too many config options. I’ve see libraries (see wolfssl) that just overwhelm with the amount of config options they have and that’s just not elegant.

[JonasVautherin]

If you are doing a local build and install of mavsdk with externalproject_add(), you’d need to clone & build for every fresh build

No you don't. Just:

1. Build the third_party deps and install them somewhere (either by running from ./third_party with -DCMAKE_INSTALL_PREFIX=/path/to/where/you/install/deps or with the MAVSDK option -DDEPS_INSTALL_PATH which does exactly that).
2. Then when building your project, add /path/to/where/you/install/deps to -DCMAKE_PREFIX_PATH. Whether or not you install mavsdk with the deps or use externalproject_add doesn't matter, the MAVSDK CMakeLists will look into CMAKE_PREFIX_PATH at configure time.

That works when cross-compiling, too :+1:.

Dependency management is often over-estimated and wrapped into layers of tooling, I find. But it's pretty simple: you need your dependencies "installed" somewhere, and then you need to tell your build system where to find them. The build system looks on your system by default, but if you install them locally somewhere, you can tell your build system to look there (with CMake that's with -DCMAKE_PREFIX_PATH).

I admit that the wrappers in MAVSDK are a bit convoluted (my bad :see_no_evil:), but now I do it like this, if you are interested.

[julianoes]

What about the find_package calls? Do you just move them to the plugins too?

[devbharat]

What about the find_package calls? Do you just move them to the plugins too?

I did yes.

[devbharat]

The dependency installation thing works as well, I just have it configured so that all build and install artifacts are deleted on make clean. Perhaps I need a clean and a more ‚deeper‘ clean separation and decide what’s deleted when.

[devbharat]

but now I do it like this, if you are interested.

it’s a good read, thanks!

[Consti10]

I'm gonna mention that here, since it is semi related - even though annoying, std::filesystem on Android only has been 'fixed' in the most recent ndk release(s). To build a qt 5.15 android compatible mavsdk, I had to basically hack out the std::filesystem includes. Even though you are already doing an amazing job making sure dependencies do not propagate up unneccessarily, be aware that rare case line the one I described happen more often than expected.

[julianoes]

@Consti10 I'd love a PR that removes the std::filesystem hack. I would be glad to have that gone.

[julianoes]

jsoncpp as an example is used in core now anyway, so the discussion is moot.