cmake: locally install pip dependencies

[julianoes]

This would be a way to locally install the pip dependencies automatically. This way we don't need to get them separately. I don't have a strong opinion. Requiring the dependencies explicitly manually would also be ok.

[JonasVautherin]

I guess it's a question of preference. I am personally not a big fan of having CMake do custom things: when I run CMake as a user, I don't want it to install stuff on my computer and alter the PYTHONPATH.

I can tolerate the part that runs pymavlink.tools.mavgen (which we have already), but we already see that it may or may not be cross-platform (MAVSDK patches the CMakeLists to work on macOS and Windows). So if it was a Python script to be run manually whenever there is a need to generate code, it would also work (while making it obvious that Python has to be run). If there was a change to make, I would rather do that.

So yeah, I think this goes too far for me 🙈, as it is one more step towards more and more completely non-CMake stuff creeping into the CMakeLists. Again that's personal, but my bar for considering that a build system has "too much custom stuff to be trusted" is pretty low 😅. Quite often it's faster for me to just do the stuff manually than to convince myself that the script I'm told to run won't mess up my system.

[julianoes]

@JonasVautherin I agree in general but it is only installing it locally in the build directory, and the PYTHONPATH is only set for that call.

[JonasVautherin]

Sure. It just makes the CMakeLists look weird, and I need to review it carefully to convince myself that it only does that. But on the other hand, the project won't extend so the CMakeLists should not grow more... maybe it's worth it indeed 👍.

[julianoes]

@JonasVautherin I see what you mean. I too would like the CMakeLists.txt as simple as possible. And cmake downloading stuff is always an unpleasant surprised.

However, we're a bit in middle ground here. On the one hand we are using Python libraries locally (from the submodule) while on the other hand, we are requiring modules installed in the system (or env).
I'm almost tempted to remove the submodule and instead require the user to install pymavlink from pip manually first, which then in turn should download its pip dependencies.

A pip install would be cleaner. I don't actually understand why we have to have the submodule.

python -m pip install --upgrade pymavlink
Defaulting to user installation because normal site-packages is not writeable
Collecting pymavlink
  Using cached pymavlink-2.4.38-py3-none-any.whl (11.3 MB)
Requirement already satisfied: lxml in /usr/lib/python3/dist-packages (from pymavlink) (4.8.0)
Collecting future
  Using cached future-0.18.3-py3-none-any.whl
Installing collected packages: future, pymavlink
Successfully installed future-0.18.3 pymavlink-2.4.38

[julianoes]

@JonasVautherin let's discuss whether the submodule makes sense still in the next dev call. And then based on that outcome we continue here.

[julianoes]

@JonasVautherin I started working on a branch that doesn't require the submodule but got stuck on this: ArduPilot/pymavlink#813

[hamishwillee]

@julianoes Are you aware that the pip module on PyPi is not kosher? Specifically, it is built from the ArduPilot fork of MAVLink which may be ahead of ArduPilotMega.xml on mavlink/mavlink and is a subset of common.xml?

Doesn't that mean this is only workable if we can have a "vanilla" pymavlink that reflects common.xml as used by mavlink/mavlink?

From the discussion, I see this as not ready to merge. Let me know when you have reached a consensus and we should get Peter Barker in for review too.

[hamishwillee]

This is the "not using cmake" path. Don't we still need this instruction then?

[julianoes]

The requirements are installed as part of the cmake build.

[julianoes]

Oh, now I understand what you mean.

[julianoes]

Fixed, and rebased.

[JonasVautherin]

From the discussion, I see this as not ready to merge

@hamishwillee: for me it can be merged like this. This PR is not about using pymavlink from pip, which would be another PR, I would say.

[hamishwillee]

Thanks. This looks sane. I am sure you have both tested thoroughly.

[hamishwillee]

Thanks. This looks sane. I am sure you have both tested thoroughly.