Conversation
|
Please no submodules. |
julianoes
left a comment
There was a problem hiding this comment.
I appreciate that you want to make this work with vcpkg. If you can, that's great. However, we won't want to break anything that we have currently.
JonasVautherin
left a comment
There was a problem hiding this comment.
Seems like this is removing everything we currently have to handle dependencies, and enforcing the use of VCPKG instead? 😅
My personal opinion is the following: we should use CMake's find_package for dependencies, and the main project should not handle dependencies. Then the user can bring them however they want (installing them on the system, building them manually, using Conan or VCPKG). But this should be the responsibility of the user, we don't want to maintain 6 different dependency managers (it's just too much work). I explain it here.
Now admittedly, MAVSDK uses a weird superbuild that makes it confusing, and while I changed my mind since I wrote it, it's hard to change it because people would not be happy 🙈.
BUT if we were to change it, I would rather remove the superbuild (and do it like here) than move it to an arbitrary package manager like VCPKG or Conan or Nix.
This said, you should be able to use VCPKG to handle the dependencies without having to change anything in this repo. Something like:
- Have VCPKG install the dependencies somewhere
- Build MAVSDK with SUPERBUILD=OFF and CMAKE_PREFIX_PATH pointing to that somewhere
I think it should be it 😊
|
The main idea behind is to remove super build and replace by package manager. There is no forcing of vcpkg, let cmake handle dependencies by find_package(system wide or by package manager). However, if you want to keep superbuild then we need to keep track of both: superbuild and package managers, therefore there is not point of it. |
This is already what MAVSDK does. We just have a superbuild around that. But you can disable the superbuild with
Again, no. Our superbuild is like a custom package manager. It is just the default option here (and I agree that this is not ideal, I would personally remove it). You can already use Conan, or Nix, or the system package manager, and probably VCPKG too (I have never used VCPKG). |
|
What would be your suggestions regarding this PR? Close at keep superbuild as it is? |
|
Yes 👍. I think it is great if you manage to use VCPKG with The problem here is always the same: if we introduce a new package manager, then we have to maintain it, and that's work. What's important is that we don't prevent users from using their preferred package manager. But currently we don't: with |
This is work in progress PR. Looking for suggestions.
Right now it expects vcpkg globally installed, so maybe make vcpkg as submodule of the project?
Changes
Removed the superbuild configuration.
find_package()resolves system-installed dependenciesRetained third-party components that are always downloaded and built:
Added CMake presets for simplified configuration and build workflows
Cleaned up CMake
Removed usage of MAVLINK_INCLUDE_DIR
Tasks
Test on:
Issue
The generated MAVSDK Server files are based on an older Protobuf version than the one installed via vcpkg.
Suggestion:
Generate Protobuf files during the Cmake build process instead of storing them in the repository.
Example how to build
Install vcpkg unless you are working in the mavsdk devcontainer (already installed there).
From the
cppdirectory: