Use pure third-party libraries instead of a secondary development version after fork

zyy37 commented 1 year ago

Describe problem solved by the proposed feature

Use pure third-party libraries instead of a secondary development version after fork.

[submodule "src/modules/mavlink/mavlink"]
    path = src/modules/mavlink/mavlink
    url = https://github.com/mavlink/mavlink.git
    branch = master
[submodule "src/drivers/uavcan/libuavcan"]
    path = src/drivers/uavcan/libuavcan
    url = https://github.com/dronecan/libuavcan.git
    branch = main
[submodule "Tools/simulation/jmavsim/jMAVSim"]
    path = Tools/simulation/jmavsim/jMAVSim
    url = https://github.com/PX4/jMAVSim.git
    branch = main
[submodule "Tools/simulation/gazebo-classic/sitl_gazebo-classic"]
    path = Tools/simulation/gazebo-classic/sitl_gazebo-classic
    url = https://github.com/PX4/PX4-SITL_gazebo-classic.git
    branch = main
[submodule "src/drivers/gps/devices"]
    path = src/drivers/gps/devices
    url = https://github.com/PX4/PX4-GPSDrivers.git
    branch = main
[submodule "platforms/nuttx/NuttX/nuttx"]
    path = platforms/nuttx/NuttX/nuttx
    url = https://github.com/PX4/NuttX.git
    branch = px4_firmware_nuttx-10.3.0+
[submodule "platforms/nuttx/NuttX/apps"]
    path = platforms/nuttx/NuttX/apps
    url = https://github.com/PX4/NuttX-apps.git
    branch = px4_firmware_nuttx-10.3.0+
[submodule "Tools/flightgear_bridge"]
    path = Tools/simulation/flightgear/flightgear_bridge
    url = https://github.com/PX4/PX4-FlightGear-Bridge.git
[submodule "Tools/simulation/jsbsim/jsbsim_bridge"]
    path = Tools/simulation/jsbsim/jsbsim_bridge
    url = https://github.com/PX4/px4-jsbsim-bridge.git
[submodule "src/drivers/cyphal/libcanard"]
    path = src/drivers/cyphal/libcanard
    url = https://github.com/opencyphal/libcanard.git
[submodule "src/drivers/cyphal/public_regulated_data_types"]
    path = src/drivers/cyphal/public_regulated_data_types
    url = https://github.com/opencyphal/public_regulated_data_types.git
[submodule "src/drivers/cyphal/legacy_data_types"]
    path = src/drivers/cyphal/legacy_data_types
    url = https://github.com/PX4/public_regulated_data_types.git
    branch = legacy
[submodule "src/lib/crypto/monocypher"]
    path = src/lib/crypto/monocypher
    url = https://github.com/PX4/Monocypher.git
    branch = px4
[submodule "src/lib/events/libevents"]
    path = src/lib/events/libevents
    url = https://github.com/mavlink/libevents.git
    branch = main
[submodule "src/lib/crypto/libtomcrypt"]
    path = src/lib/crypto/libtomcrypt
    url = https://github.com/PX4/libtomcrypt.git
    branch = px4
[submodule "src/lib/crypto/libtommath"]
    path = src/lib/crypto/libtommath
    url = https://github.com/PX4/libtommath.git
    branch = px4
[submodule "src/modules/uxrce_dds_client/Micro-XRCE-DDS-Client"]
    path = src/modules/uxrce_dds_client/Micro-XRCE-DDS-Client
    url = https://github.com/PX4/Micro-XRCE-DDS-Client.git
    branch = px4

For example, it is better to use a clean version of the third-party library (https://github.com/apache/nuttx.git), rather than a secondary development of the maintenance repository (https://github.com/PX4/NuttX.git).

Other third-party libraries can already provide the full functionality as standalone libraries.
If you have any bugs or needs, you can submit them upstream or avoid them through software architecture design, rather than simply using the secondary development of third-party libraries, or the divide will become much bigger after a long time.
Secondary development and forking third-party libraries not only require a lot of time to synchronize and maintain, but also have latency. Because you fork a pure version of a third-party library such as nuttx, if someone else does secondary development based on your px4 project, they have to fork the third-party library version of your fork. This will create a large number of nesting dolls. But this should be avoided by software architecture design, that is, no matter how many times the secondary development, the modified file should be the same.

Describe your preferred solution

Use pure third-party libraries instead of a secondary development version after fork. Avoiding secondary development of third-party libraries through software architecture design. This not only reduces synchronization and maintenance, but also makes it cleaner.

Describe possible alternatives

N/A

Additional context

No response

dagar commented 1 year ago

@zyy37 I actually agree with the sentiment, but we don't do this for practical reasons.

upstream repository can change out from under us and break old branches (this has happened several times believe it or not)
often we'll need somewhere to stage our own changes when contributing to upstream projects

I am still strongly in favor of keeping in sync with the upstream repositories and contributing back to them. For example with NuttX (our fork predates apache/nuttx) the master branch is automatically updated to track upstream (https://github.com/PX4/NuttX/tree/master). If we make bug fixes on our current NuttX branch they almost always are contributed upstream first, then backported. When we're ready to upgrade NuttX (requires a lot of validation) nearly all our extra commits can be dropped.

In general I try to keep the master/main branches in sync with upstream and then have a separate px4 branch so it's clear.

Does that make sense?

zyy37 commented 12 months ago

Well, keeping a separate repository in sync with upstream, which is commonly seen in projects of Chromium-base or Android-base, which are all to customer.

Developers using third-party libraries can either compile from source code or install apt-get, which is very free and versatile. Your goal is also to use third-party libraries, but users have to pull your fixed third-party library version, which is very limited. Users can install pure third-party libraries without knowing the code implementation, because this is the generic functionality provided. But the user pulls your secondary development of the third-party library (e.g.: PX4/NuttX), users may need to compare what modifications you have made to the official version, and do not know whether it is specifically suitable for your px4 project.

According to the principles of Open Closed Principle, decoupling, and modularization in software design, you can decouple the secondary development of the third-party library (e.g.: PX4/NuttX) into a pure version of the third-party library (e.g.: apache/nuttx) and a subunit implementation of your own project. The secondary development repo of the third-party library always feels like a temporary one. This subunit implementation is your asset. If the pure third-party library source code is not available in the future, or you can choose to adapt to other RTOS projects, these scenarios are applicable.

Are some sub modules also directly used as pure third-party libraries? for example https://github.com/mavlink/libevents.git. Shared/dynamic libraries means that the same copy can be shared, and reducing consumption, otherwise there will be an additional version of the px4 libevents library in memory.

Perhaps you should consider it internally, thanks.

dagar commented 12 months ago

I'm not sure what you're proposing exactly. We have these 3rd party dependencies, we keep forks out of necessity, but we also keep the master branch in sync with the upstream project and are committed to contributing all of our changes. The goal is always to be able to use the pure version, but things get out of sync over time. Whenever I've contributed to NuttX I've done the development within the NuttX submodule in PX4.

When we do NuttX upgrades we're getting back in sync with the pure upstream version, then over time it diverges again (due to upstream NuttX development).

Are some sub modules also directly used as pure third-party libraries? for example https://github.com/mavlink/libevents.git

This one we control completely, but exists to be kept in sync with other projects like QGroundControl.

zyy37 commented 12 months ago

I said that if you want to use a third-party library for secondary development, multiple versions will appear in memory, one is customized by you and the other is a dynamic library that can be shared by other processes (such as libmicroxrcedds_client library in the figure above). This takes up consumption. If you use a pure third-party library, there is no such issue.

Additionally, if you submit git commit modifications to their third-party library, you can generally delete the draft branch.

dagar commented 12 months ago

In the case of microxrcedds_client we're in sync with upstream, except for one little commit we carry that's a bit of a hack to use PX4's monotonic time source. https://github.com/PX4/Micro-XRCE-DDS-Client/commit/4248559f3b111155c783e524e461ccc83e768103

It would be great to figure out how to make that change properly in a way that can be contributed and get us back in sync 100%, but at the moment I'm not sure what that would look like.

As for consumption, by far the most common deployment target for PX4 is NuttX (flash constrained embedded system) where all of this code is statically linked and anything unused is stripped. The px4_sitl_default build mirrors this for development and testing on Linux and MacOS. So even if memory usage was a constraint (it's not) it's far more beneficial for development to keep Linux and NuttX aligned rather than supporting dynamic libraries in one case.

Overall I think we're in agreement on the importance of keeping 3rd party libraries in sync, making it as easy as possible to contribute to those projects.

zyy37 commented 12 months ago

For your example https://github.com/PX4/Micro-XRCE-DDS-Client/commit/4248559f3b111155c783e524e461ccc83e768103 mentioned above or others, maybe you can use the Open Closed Principle. This is an example website for Open Closed Principle, which mentions that develop requirements should not be coded by appending if-else every time. "What it means though is that we should strive to write code that doesn’t have to be changed every time the requirements change." This is just a kind suggestion.

I think third-party libraries are designed for general scenarios or objects, and px4 should also be included.

PX4 / PX4-Autopilot