drop mavlink submodule

[onur-ozkan]

We can improve the automation for fetching dialect files by using a simple script. The script would read a commit hash from a file called "commit_hash" for https://github.com/mavlink/mavlink and pull the dialect files into the directory called "dialect_definitions" which would then be committed (to this repository).

CI could run this script to ensure the expected dialects exist in the repository. When we want to update the dialect files, we would only need to update the commit hash in the commit_hash file and run the script.

This would remove the git prerequisite from the build script and simplify it.

[pv42]

Thats a great idea, since it would also decrease size of the code pulled in from ~33MB to ~1MB, and would also make it clearer what version of the definitions were are currently building against.

[patrickelectric]

what is the problem with https://git-scm.com/docs/git-clone#Documentation/git-clone.txt---shallow-submodules ?

[patrickelectric]

It would remove git but will require something else to download. It also downloading something from a "link", and that sounds unsafe to have a build script that download stuff.

[pv42]

what is the problem with https://git-scm.com/docs/git-clone#Documentation/git-clone.txt---shallow-submodules ?

Not having to pull in pymavlink every time would be an improvement but it would still pull the whole git tree of mavlink when only one version of only the message definitions is needed (git clone mavlink is ~13.28).

[pv42]

It would remove git but will require something else to download. It also downloading something from a "link", and that sounds unsafe to have a build script that download stuff.

I think the idea is to download the definitions with the CI pipeline and then include them into this repository. So the build script would not have to download anything.

[patrickelectric]

what is the problem with git-scm.com/docs/git-clone#Documentation/git-clone.txt---shallow-submodules ?

Not having to pull in pymavlink every time would be an improvement but it would still pull the whole git tree of mavlink when only one version of only the message definitions is needed (git clone mavlink is ~13.28).

you don't need to the full history, that's why I recommended shallow-submodule, you can do the same thing and test with depth=1, the total here is 270KB, not that bad.

I think the idea is to download the definitions with the CI pipeline and then include them into this repository. So the build script would not have to download anything.

That would require the dialect to tracked in the repository, that will confuse people, I believe that the simpler and standard way is much safer and easy.

BTW, what is this issue trying to fix truly ? I can't see the point.

[onur-ozkan]

It would remove git but will require something else to download. It also downloading something from a "link", and that sounds unsafe to have a build script that download stuff.

Nothing needs to be downloaded. The point is that this repository can include the XML definitions statically and the build scripts can use them directly. The build outputs will be exactly the same but the build script will no longer require git.

BTW, what is this issue trying to fix truly ? I can't see the point.

There is another advantage with this approach; if ever needed, we can add additional dialects living in the same directory. So it makes it much simpler to maintain the custom dialects.

There is no strong "issue" barrier here. My proposal is simply aimed at improving the build cycle. Right now we are not building or running anything from the submodule so we can achieve the same result without requiring a git dependency.

Another advantage of this approach is that it makes maintaining custom dialects much simpler, e.g., if needed in the future, we could add custom dialects (maybe for testing/debugging) in the same directory.

edit: Also, from @pv42, "and would also make it clearer what version of the definitions were are currently building against." this is another advantage as well. You can read/review dialect related changes on the PRs way easier.

[onur-ozkan]

That would require the dialect to tracked in the repository, that will confuse people

Can you say more about this? Which part is causing the confusion, maybe we can improve that. The commit hash will be still visible, and dialects will be even more visible than before. At least for me, current approach is way more confusing.

[patrickelectric]

Nothing needs to be downloaded. The point is that this repository can include the XML definitions statically and the build scripts can use them directly. The build outputs will be exactly the same but the build script will no longer require git.

This will directly result in:

• Duplicate the amount of information (two source of truths)
• Create unnecessary diffs
• Track things that are not responsible for this repository

This will indirect result in:

• People will be using this to have "no official" or "patch" versions of the dialect.

There is another advantage with this approach; if ever needed, we can add additional dialects living in the same directory. So it makes it much simpler to maintain the custom dialects.

That's exactly what I'm against. Why ? Why there is a need to have additional dialects out of the official repository ? The name of the repository is "rust-MAVLink" tt should use the official source of it.

There is no strong "issue" barrier here. My proposal is simply aimed at improving the build cycle. Right now we are not building or running anything from the submodule so we can achieve the same result without requiring a git dependency.

How this improve the build cycle ? We already need git to download the project in CI.

Another advantage of this approach is that it makes maintaining custom dialects much simpler, e.g., if needed in the future, we could add custom dialects (maybe for testing/debugging) in the same directory.

This is again, exactly what I'm against and this approach will reflect into it. This will not follow the official MAVLink repository or imply to people that.

edit: Also, from @pv42, "and would also make it clearer what version of the definitions were are currently building against." this is another advantage as well. You can read/review dialect related changes on the PRs way easier.

There is a clear version in github via the interface, or via git submodule status.
Why should we review dialect changes ? This is up to the MAVLink repository.

That would require the dialect to tracked in the repository, that will confuse people

Can you say more about this? Which part is causing the confusion, maybe we can improve that. The commit hash will be still visible, and dialects will be even more visible than before. At least for me, current approach is way more confusing.

I can see only two options here, from what I understood.

1. Replace git with requests or something else to download files from the internet and track necessary files to be tracked.
2. Add the XML files into this repository and keep a separate commit hash file pointing to the MAVLink revision they came from.

I’m against both.

For the first option, we would be replacing a standard and explicit dependency on git with custom download logic. That is not simpler, not more transparent, and not safer. It adds one more things to maintain for something git already solves well and is already a dependency..

For the second option, we would be just copying the messages that are already being tracked, reviewed and approved by the main upstream repository, into this repository while also keeping a reference to the upstream commit (Like, what ? 😕). That creates two sources of truth, the actual upstream MAVLink repository, and the copy here. Even if they are intended to match, the workflow now depends on humans keep them aligned. That is extra process to maintain mirrored data that does not belong to this repository.

This will also imply that people will also push for patches and changes here that do not reflect in the original repository, how we are going to be sure ? how we are going to check ? Why should we allow or not such thing ?

My concern is not only technical, but also about repository boundaries. We should consume the official MAVLink definitions, not become an alternative place where dialect definitions live, get patched, or are reviewd. Once the files are present locally, even if initially copied from upstream, it becomes much easier for the project to drift local changes, test-only dialects, unofficial variants, or convenience patches. I do not think that is a good direction. In my mind it just increases duplication, and opens the door to ambiguity around what is official versus local. I do not see that as an improvement.

The current submodule approach keeps the responsibility clear, official dialects live in the official MAVLink repository, this repository consumes them from there, the pinned revision is explicit and reproducible. Simple and standard.

If the concern is repository size or checkout cost, sa I said shallow submodules already address that well enough without changing the ownership model of the files.

If the concern is visibility of the version in use, that is already available through the submodule revision.

If the concern is reviewability of upstream dialect changes. Those changes should be reviewed in the upstream repository, where they belong.

[onur-ozkan]

That's exactly what I'm against. Why ? Why there is a need to have additional dialects out of the official repository ? The name of the repository is "rust-MAVLink" tt should use the official source of it.

For testings and stuff.

How this improve the build cycle ? We already need git to download the project in CI.

I wasn't talking for CI, it's for people using this library, not every build environment has internet access.

There is a clear version in github via the interface, or via git submodule status.
Why should we review dialect changes ? This is up to the MAVLink repository.

I meant dialect related changes, it doesn't have to be dialect change e.g., updating some tests, docs or code that is using specific dialect.

For the second option, we would be just copying the messages that are already being tracked, reviewed and approved by the main upstream repository, into this repository while also keeping a reference to the upstream commit (Like, what ? 😕). That creates two sources of truth, the actual upstream MAVLink repository, and the copy here. Even if they are intended to match, the workflow now depends on humans keep them aligned. That is extra process to maintain mirrored data that does not belong to this repository.

It actually doesn't need humans to be aligned. It will be automated on CI. A tiny script would be able to easily validate this continuously (with each push) on CI.

I don' think this introduces any risk at all. It's just as safe as the current approach. The official MAVLink dialects will match the ones here 100% and can be easily validated. Right now we fetch them with git on demand. We will still fetch them with git, but instead of doing it on demand for each user, this logic will run once for every bump for all users. That's the only difference. We will still get the same dialect files and the source is not changing. As for the maintenance part, the cost does not grow over time. It's a one-time effort and the script will continue to work just fine afterward.

[onur-ozkan]

An alternative solution would be to add the dialect-generated code to the repository instead of dropping the submodule. In this case the build script would exclude the code generation logic and rely directly on the pre-generated files (so, no git and/or internet access is required in order to build).

I am not certain whether git would be happy with those files since they are quite large, but if it handles them well I think this approach could solve both of our concerns. The CI could then validate whether the generated code is valid.

[joaoantoniocardoso]

Hi, from what I am capturing here, we are discussing two options:

(a) Keep the required 3rd-party assets management as git submodules;
(b) Change the required 3rd-party assets management to pre-downloaded and committed files;

• The (a) requires git dependency to build (a simple .zip of the repo is not enough). The (b) eliminates this dependency, allowing a build directly from the .zip.
• The (a) relies on a standard and future-proof solution (git) for managing the assets, with maintenance-free tooling. (b) requires some harness to bootstrap that management, which adds some maintenance. Both require manual running to update versions.

I may not be understanding the motivations, but I don't see a huge win in getting rid of git dependency for the sake of internet access when we rely on the rust tooling, which is heavily internet-dependent.

[onur-ozkan]

I don't see a huge win in getting rid of git dependency for the sake of internet access when we rely on the rust tooling, which is heavily internet-dependent.

I am not sure if I understand it correctly, but rust has nothing to do with internet itself. If you mean pulling crates from the crates.io registry via cargo, offline builds use vendoring to avoid that. There is a real need for "offline builds", it's not made up or something unimportant, so I am trying to find a solution to that. Perhaps #474 (comment) would be better?