Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move Tesseract_ros to its own package #207

Closed
mpowelson opened this issue Jan 20, 2020 · 15 comments
Closed

Move Tesseract_ros to its own package #207

mpowelson opened this issue Jan 20, 2020 · 15 comments

Comments

@mpowelson
Copy link
Contributor

I know this has been discussed, and I believe there are some roadblocks to this. What are they and how do we plan on resolving them?

@schornakj
Copy link
Contributor

I think that the packages are fully separated now. When I build Tesseract for ROS2 I just put a COLCON_IGNORE file in tesseract/tesseract_ros and Colcon uses tesseract_ros2 instead, which indicates that there aren't any dependencies between tesseract_ros and the rest of the core Tesseract packages.

@Levi-Armstrong
Copy link
Contributor

I came across this repository showing an example having ros1 and ros2 packages in the same repository. I created a PR showing how to merge them into a single package so the package name does not change between versions of ros. I wanted to get your thoughts on using this approach to merge in tesseract_ros2 into this repository? @schornakj @mpowelson @marip8 @jrgnicho @jdlangs

@jrgnicho
Copy link
Contributor

jrgnicho commented Jan 30, 2020

@Levi-Armstrong your proposed changes there look fine to me, although just adding a COLCON_IGNORE file would be much simpler and it would still build under ROS1 as long as tesseract_ros2 remains in its own repo

@gavanderhoorn
Copy link

Not using the ugly conditionals in package manifests would also be much less .. well .. ugly ;)

@Levi-Armstrong
Copy link
Contributor

Currently there are three options.

  • Option 1:
    • Core Libraries in its own repository
    • ROS1 Libraries in its own repository
    • ROS2 Libraries in its own repository
  • Option 2:
    • All in the same repository but ROS1 and ROS2 packages must have different names. Example tesseract_monitoring (ROS1) then tesseract_monitoring2 (ROS2). Also the CMakeLists.txt must check for Catkin and if not found do not build shown in the original example.
  • Option 3:
    • All in the same repository where ROS packages can have the same names between ros version. But it requires a conditional package.xml and different folder structure shown the modified example.

I would like to get everyone's opinion about which one they prefer. @gavanderhoorn @schornakj @mpowelson @marip8 @jrgnicho @jdlangs

@mpowelson
Copy link
Contributor Author

I think my preference would be first Option 1. Then Option 3.

Option 1 allows non ROS users to simply not clone the unused repositories rather than having them there but not building them.

Option 3 is easy and I suppose is a bit more convenient. However, I'm already imagining either myself or code following in an IDE getting confused and changing the wrong headers. Plus, I would think in a year or two a large percentage of our projects will be in ROS2 making this structure more inconvenient than useful. Though I suppose we could just restructure to remove ROS1 at that point.

I'm not sure I have any reasons for not liking 2. It just seems messy to add 2 to the package name.

Do any of these options make a future release easier/harder?

@gavanderhoorn
Copy link

gavanderhoorn commented Jan 31, 2020

I posted a (very) long comment this (my) morning, but for some reason it seems to have disappeared. :(

not sure what happened here.


Edit: did any of you watching this issue/repository receive a github email notification about my comment?


Edit2: summarising my comment from this morning (perhaps I was hallucinating): I favour alternative 1. No confusion, no ugly conditionals in package manifests, no work-arounds in build scripts, no potential confusion with editors (good point @mpowelson) and only a minor amount of overhead.

I had two general guidelines for when not to put things in the same repository:

  1. if they don't need to be versioned together (ie: A increases version just because B needs an increase, but the increase does not reflect any change whatsoever in A)
  2. there is little opportunity for cross-branch merging / cherry-picking (with business logic actually being hosted in a separate repository already this does seem like it could be the case)

I also had included a link to ROS2 Transition Strategy on ROS Discourse.

@mpowelson
Copy link
Contributor Author

@gavanderhoorn I didn't see an email on my end.

@jdlangs
Copy link
Contributor

jdlangs commented Jan 31, 2020

I agree that option 1 seems to be the cleanest but as a developer I am pretty scared of having to coordinate changes that would affect both the core libraries and the wrappers across multiple repositories. To me, the core and wrappers are inherently a single project, and so it makes sense for them to live in a single repo where they will always be in sync and versioned together.

So I actually prefer option 2 here. I also don't like putting the number 2 on package names though; my suggestion would be to use names like tesseract_monitoring and tesseract_monitoring_ros1, which also is a not-so-subtle hint that ROS-I has moved on from ROS1.

@gavanderhoorn
Copy link

To me, the core and wrappers are inherently a single project, and so it makes sense for them to live in a single repo where they will always be in sync and versioned together.

One argument against this is that we separate "core libraries" and wrappers exactly because we don't see them as "a single project", but a library with business logic and some other entity that makes that available in the context of a ROS node graph (ie: a wrapper node).

@jdlangs
Copy link
Contributor

jdlangs commented Jan 31, 2020

Business logic changes are one thing but wouldn't any change to the client interface / API of the core require a corresponding change in the wrapper? Are they not tightly coupled at that layer?

@gavanderhoorn
Copy link

gavanderhoorn commented Jan 31, 2020

Not necessarily. I would say that depends rather heavily of the type of change(s).

We could re-implement the entirety of a class without changing any of the interfaces, which would mean no changes in the wrapper either.

O wait, I interpreted your comment as inverted to what you're writing.

Business logic changes are one thing but wouldn't any change to the client interface / API of the core require a corresponding change in the wrapper? Are they not tightly coupled at that layer?

They are certainly coupled. But only in one way: the wrapper depends on the core library. Not the other way around.

If core > wrapper, something has gone rather wrong.

@jdlangs
Copy link
Contributor

jdlangs commented Jan 31, 2020

Definitely the one-way dependency is important. My point is that if the dependency is such that if changes to the interface of the core require changes to the wrapper, it strongly suggests all those changes should be happening in a unified way. Admittedly, I haven't dug much into a project that uses a structure like this so I don't have much intuition about how often this is the case.

@Levi-Armstrong
Copy link
Contributor

@jdlangs and @gavanderhoorn Thank you for the discussion. I personally have struggled with the two option discussed. Keeping everything in one repository is nice from a maintains perspective, but from a developers perspective it is nice to keep them isolated. I am still not sure what is the best way forward is. I think we will keep down the original path of isolating them in separate repositories and see how things go.

@mpowelson
Copy link
Contributor Author

As of #241 this repo doesn't depend on ROS. We now have essentially 4 repos

  1. Tesseract - Core Cmake Libraries and Python wrappers
  2. Tesseract_ext - External packages
  3. Tesseract_ros - ROS 1 wrappers and interfaces with Python wrappers
  4. Tesseract_ros2 - ROS 2 wrappers and interfaces

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants