Skip to content
This repository has been archived by the owner on Nov 30, 2022. It is now read-only.

Migrate to rosbag2 repo #63

Closed
Tracked by #1160
amacneil opened this issue Oct 23, 2022 · 7 comments · Fixed by ros2/rosbag2#1163
Closed
Tracked by #1160

Migrate to rosbag2 repo #63

amacneil opened this issue Oct 23, 2022 · 7 comments · Fixed by ros2/rosbag2#1163
Labels
enhancement New feature or request

Comments

@amacneil
Copy link
Collaborator

amacneil commented Oct 23, 2022

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.

@amacneil amacneil added the enhancement New feature or request label Oct 23, 2022
@clalancette
Copy link
Contributor

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. #64 is a step in the right direction, but it is still failing to build on Windows :(.

@amacneil
Copy link
Collaborator Author

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

@clalancette
Copy link
Contributor

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
Copy link
Contributor

james-rms commented Nov 14, 2022

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
Copy link
Contributor

clalancette commented Nov 14, 2022

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants