Improve Proto generation & Build document

[junwoo091400]

Why

1. Understanding MAVSDK (Proto) Build process is hard
2. There can be subtle issues where the protoc-gen-mavsdk python library is not up to date and can create undesirable results when files get generated with the tools/generate_from_protos.sh command
   • It resulted in this PR / troubleshooting process : Generate from protos MAVSDK#1779
3. Decoupling the protoc and protoc-gen-mavsdk part and clearly explaining what is doing what is needed

Work in progres

TODOs

• Explain how protoc-gen-mavsdk part works
• Add alternative method to install the library from the source itself (as the CI does in MAVSDK repository)
• [ ]

[julianoes]

Add alternative method to install the library from the source itself (as the CI does in MAVSDK repository)

@devbharat was suggesting to do that too, and actually had a commit to do so.

@JonasVautherin what's your opinion on that? I remember you had some valid reasoning but I forget the details.

[JonasVautherin]

@JonasVautherin what's your opinion on that? I remember you had some valid reasoning but I forget the details.

We just need to have protoc-gen-mavsdk in the PATH at some point, no matter how it is installed. It is just nice to be able to just pip install protoc-gen-mavsdk instead of having to clone the repo and go install it. Especially because protoc-gen-mavsdk almost hasn't changed in years, and when it did, it was in a backward-compatible way.

The way I use it is by quickly installing it in a venv when I need it (so that it is the latest version, and it is in my PATH) 😇.

[julianoes]

clone the repo

But we always need to clone the proto repo if we want to do the generation, so it's already there...

[JonasVautherin]

But we always need to clone the proto repo if we want to do the generation, so it's already there...

I guess the idea was that you could totally have your own repo with your own definitions and use protoc-gen-mavsdk, and so it felt like protoc-gen-mavsdk deserved to be published somewhere 😁. Also to me it's easier to debug if someone has an issue; I can just ask which version they have, instead of asking them to make a clean clone of the repo, re-import their changes, install protoc-gen-mavsdk, and try again.

But let's change the docs everywhere to make users install from sources, I really don't mind! Not sure if we should remove the pip integration, though (I mean if it doesn't hurt, I'm happy to keep it for my personal use, since all I need to do, once every few years, is publish a release and let the CI do everything 😇).

[julianoes]

@devbharat's idea was to do the pip install directly out of cmake, so you don't even require docs to do it.

[JonasVautherin]

@devbharat's idea was to do the pip install directly out of cmake, so you don't even require docs to do it.

Your call, I know my opinion there is not mainstream 😁.

I'm not a big fan of doing more and more custom things in CMake. It is right that then you don't need to understand what happens, but on the other hand you don't understand what happens, and it makes it more difficult (IMO) in case you want to.

My example is always the PX4 build system: it's a custom mix of different things (Makefile calling CMake calling custom actions and shell scripts and probably some python? I'm feeling lucky that the Makefile is not generated by Autoconf 😅), such that I wouldn't even know how to build PX4 without the Makefile, and honestly I really don't want to go learn about that.

For MAVSDK, I have come to think that even the superbuild embedded in the main project is too much. My strategy in new projects is to not run it from the configure step of the main project, but instead document how to run it manually. That's one more step for users, but that's removing a lot of magic. And who knows, maybe on the way some users learn one thing or two about CMake 😇.

[devbharat]

@junwoo091400 @julianoes thanks for bringing it up again!

While I agree with a lot that @JonasVautherin said, and I also agree with the example of how PX4's build system has grown into a somewhat complicated mix of things, it does manage to provide a very unified build 'frontend' to the 'casual' user (with all the complicated stuff happening 'under the carpet') that enables a lot of the new users a very low threshold entry into the project and quickly get to the application development, one reason why it's so popular. Additionally, I think a lot of developers will come to MAVSDK from PX4 background and perhaps will expect to find something similar. Besides, the script, even though python, does generate cpp code so it is not entirely unexpected to find cmake doing it. It feels a bit fractured to have to build part of the code with cmake, part with a python script, that needs a third package installed on your system somewhere, when in principle you have all the tools do it cloned directly in the repo already. So I would say having an optional way to do it 'all in one' might be worth it.

[junwoo091400]

My example is always the PX4 build system: it's a custom mix of different things (Makefile calling CMake calling custom actions and shell scripts and probably some python? I'm feeling lucky that the Makefile is not generated by Autoconf sweat_smile), such that I wouldn't even know how to build PX4 without the Makefile, and honestly I really don't want to go learn about that.

I second that 👏 this is very much a common pain-point that a lot of people face, and having too much 🪄 Magic can do more harm than good in those cases. I think that exposing some critical steps where user needs to troubleshoot on (e.g. the protoc-gen-mavsdk should be documented regarding it's build proces & relationship to the PyPi repository and so on.

How about:

1. Having a section describing the protoc-gen-mavsdk more clearly in the doc?
2. I am not sure if it would do more good than harm in terms of automatically building the library through the cmake process.
3. Or, creating a flag for building the protoc-gen-mavsdk from source could be an option?

p.s. I am a CMake Noob, and have now just started learning more about what those flags / *_PREFIX argument does, and gotta say, it's not such an intuitive concept 🤣

[JonasVautherin]

@devbharat: I do understand your points, and as I said, I see that as a difference of philosophy. But I would like to make a few precisions 😊:

In terms of CMake, we try to keep our config files as "pure" as possible, such that anyone familiar with CMake can easily understand them:

• The actual MAVSDK project is defined here, and is really just basic CMake.
• We define how to build the dependencies (which we typically need to do when we cross-compile) using CMake's ExternalProject_Add, here (feel free to go read the CMakeLists.txt of each dependency). Again, that's very minimal CMake stuff, except for the build_target() custom command (defined here) that does "magic" and that I stopped using in newer projects. In newer projects, I just replace build_target() with add_subdirectory(), and build the dependencies as a separate step (optional step if you know how to handle dependencies yourself).
• We "wrap" those two projects (mavsdk + third_party) into the root CMakeLists, which builds the dependencies at configure time if -DSUPERBUILD=ON. That's fairly simple cmake, but it invokes the magic of build_target, which makes it convoluted. In new projects, I personally don't do that anymore (but I keep the third_party project without build_target(), because that's neat!).

In terms of code generation, we have two parts:

• The MAVLink part, which is part of our dependencies helpers, and not of our CMake project. This means that theoretically, if you disable the superbuild, you can just setup mavlink the way you want. It feels slightly convoluted to me because I would like to do find_package(MAVLink REQUIRED) instead of what we are doing now, but MAVLink is not made to be installed like this. That would be a nice improvement IMO.
• The proto-related part, which we run separately from a shell script. Again, not part of the CMake project. Note that it complains if protoc-gen-mavsdk is not in your PATH, and explains how to install it.
• We version the generated code, so that we don't always need to run the generation part again. That is, the proto-related part: the MAVLink part should IMO be installed as a dependency, as described above.

So building MAVSDK is made of 3 steps:

1. Generate the proto-related code
2. Run the cmake configure step
3. Run the cmake build step

Just as you know that you don't always need to run the cmake configure step, you also don't always need to run the generation step. Each of those steps is trying to use the "standard" tools in a "pure" way. Though I would personally not want to maintain it, nothing prevents you from wrapping those 3 steps into a Makefile that would detect whether or not a step needs to be re-run.

I have absolutely nothing against helpers/wrappers, as long as the lower-level is clean, and as long as I can still use the low-level interface myself. The thing is that your proposition of mixing some generation inside the cmake configuration is not a wrapper: it's making the lower-level interfaces more convoluted, and that's why I'm against that.

If I was to put efforts into improving the build system, I would first try to look into the MAVLink library (to make it work with find_package(MAVLink REQUIRED)), and I would consider removing the superbuild (in order to make the dependencies step explicit and remove the build_target() magic).

But again, that's a philosophy, and that's my opinion. I totally respect yours, and if @julianoes agrees with you, happy to follow with your suggested changes 😊 👍.

[devbharat]

I have absolutely nothing against helpers/wrappers, as long as the lower-level is clean, and as long as I can still use the low-level interface myself. The thing is that your proposition of mixing some generation inside the cmake configuration is not a wrapper: it's making the lower-level interfaces more convoluted, and that's why I'm against that.

Thanks for the nice explanation, I agree with this completely! Perhaps cmake isn't the right place to wrap things, some helper script/makefile outside could do it. And yes, the comments in the script make it clear that protoc-gen-mavsdk is not in the path, but I didn't want to install it on my system (sure I could setup a python venv etc but that yet another step) and a script to do that for you would make that simpler.

[junwoo091400]

General question on differentiating the "Building MAVSDK Library from Source" and "Building MAVSDK Server":

With my PR: mavlink/MAVSDK#1770, I noticed that a lot of the times I just build the MAVSDK Server and Library again at the same time, since I was changing proto files, which changes the whole build situation.

But also I think I wasn't so aware of how the Library / Server build differs, which is the reason why I kept re-building the MAVSDK Server as well.

Question

In which cases should one rebuild the Server vs Library? I think that would be a nice addition to avoid confusion in the User doc: https://mavsdk.mavlink.io/main/en/cpp/guide/build.html#building-mavsdk-library-from-source

*Also related Discuss Forum post I made regarding this issue: https://discuss.px4.io/t/locally-built-mavsdk-not-functional-for-px4-sitl-mavsdk-tests/27589

[JonasVautherin]

In which cases should one rebuild the Server vs Library?

Well if you configured with -DBUILD_MAVSDK_SERVER=ON, then cmake --build will always rebuild the server. But that's quick.

What does make a difference is more "when do you need a clean build, i.e. when do you need to rebuild the dependencies?". And there usually you don't, you just run the configure step once. You need a clean build when we update the dependencies, which does not happen very often.

[junwoo091400]

What does this -H. flag do? @JonasVautherin

[JonasVautherin]

That's the same as -S., but for older cmake 😅. Depending on your system you need -S. or -H., but the newer way is -S.

[junwoo091400]

Here, I was conflicted since below, it says "If you already have run cmake without setting CMAKE_INSTALL_PREFIX, you may need to clean the build first:".

Which implies that doing the configuration step again isn't enough when you want to change the CMAKE_INSTALL_PREFIX setting.

I think that the 'you may need to clean the build first' part should be removed (it is wrong?!), if my understanding that rm -rf build/default is only necessary when the third party changes are present. Thoughts on this?

[JonasVautherin]

I think it works to just re-run the configure step (no need to clean build) 👍. Not 100% sure it always does, though.

[junwoo091400]

I added the commit to explain the configure / build step, as it really wasn't intuitive for me as a CMake Noob, what those two different cmake commands were doing 💩. Let me know what you think!

[julianoes]

@junwoo091400 you need to merge or rebase with upstream main. I have added instructions how to swap out mavlink or change the dialect in the meantime which conflicts with your changes.