search

ros-tooling / rosbag2_storage_mcap

rosbag2 storage implementation for MCAP file format
https://mcap.dev/
Apache License 2.0
32 stars 5 forks source link

Migrate to rosbag2 repo #63

Closed amacneil closed 1 year ago

amacneil commented 1 year ago

As discussed, we should merge the code and packages in this repo into https://github.com/ros2/rosbag2. This will enable easier integration testing, and prepare for making mcap the default storage plugin.

Per discussion it is probably easiest to squash the commit history since there are not many commits. It is possible to merge unrelated branches using git merge --allow-unrelated-histories, but the issue number references will become incorrect.

Since the rosbag2 repo uses a separate branch per ROS release, we would either need to merge each branch separately, or keep this repo for historical release support. Neither option seems great, but probably copying it into each branch and deprecating this repo entirely is the least confusing option.

clalancette commented 1 year ago

As discussed, we should merge the code and packages in this repo into https://github.com/ros2/rosbag2. This will enable easier integration testing, and prepare for making mcap the default storage plugin.

That seems reasonable to me, but I will say that we need to fix Windows support before we do that. https://github.com/ros-tooling/rosbag2_storage_mcap/pull/64 is a step in the right direction, but it is still failing to build on Windows :(.

amacneil commented 1 year ago

Does the rosbag2 repo run CI on Windows? I can't see any builds there, how do you ensure that windows is passing?

clalancette commented 1 year ago

Does the rosbag2 repo run CI on Windows? I can't see any builds there, how do you ensure that windows is passing?

Yes, nightly. See for example https://ci.ros2.org/view/nightly/job/nightly_win_rel/2472/ (last night's build).

james-rms commented 1 year ago

As discussed,

@amacneil where was this discussed? This seems like a questionable idea - the ROS 2 tooling works equally well with packages in multiple repos, and right now rosbag2_storage_mcap CI is reasonably tight - merging it into rosbag2 would lose us that. That's the only downside I can think of, but I don't see the upside listed anywhere.

clalancette commented 1 year ago

So the issue with having it in a separate repository, but also be the default, is that you'll create a circular dependency loop between the two repositories (though interestingly, not between the packages). In turn, that will make it challenging (though not impossible) to release the packages.

In detail, rosbag2_storage_mcap depends on both mcap_vendor (from this repository) and rosbag2_storage (from the rosbag2 repository). And rosbag2_storage_default_plugins (from the rosbag2 repository) will depend on rosbag2_storage_mcap (from this repository), creating the circular dependency.

To do releases, then, you'd have to first release one of them, ignoring errors that dependencies couldn't be found. Then you release the other one, which has all of the dependencies it needs. And then you release the first one again, at which point it has all of its dependencies available.

Because of that, it is easier for everyone if we move it into https://github.com/ros2/rosbag2. That said, if there are other reasons to keep it separate, I'm certainly willing to hear them.

amacneil commented 1 year ago

I forget where exactly where it was discussed - I think in rosbag working group, and maybe Slack. This is the only Github discussion I'm aware of so we can document things here.

For mcap to become the default, at the very least the storage plugin should move into the ros2 github org (not ros-tooling).

Since https://github.com/ros2/rosbag2 is already a monorepo and contains the sqlite plugin package, it seems like the logical place to move the mcap plugin. Keeping both storage plugins in the same repo means that changes can be tested together (e.g. API changes can be tested against both plugins, rather than waiting for things to fail in the ROS Jenkins environment).

If the rosbag2_storage_mcap CI is faster/simpler, maybe we should try to improve the rosbag2 CI?

The main downside I see of merging them is that rosbag2_storage_mcap currently uses one branch to support all ROS distros, whereas rosbag2 uses one branch per distro, so it will require backporting each PR we want to support in older distros. It's a bit clunky, but that is the standard ROS development methodology.

amacneil commented 1 year ago

@james-rms I don't feel strongly about this so will leave you to debate/decide on a path forward