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

Migration to colcon-enabled version (incl. ROS2 support) #358

Open
ipa-mdl opened this issue Apr 6, 2019 · 12 comments

Comments

Projects
None yet
4 participants
@ipa-mdl
Copy link
Member

commented Apr 6, 2019

As already discussed in #333, the migration to colcon is the best way to enable ROS2 support.
(see https://github.com/ros-industrial/industrial_ci/projects/1 as well)
I have prepared a working prototype (https://github.com/ipa-mdl/industrial_ci/tree/colcon), which I#d like to move upstream

This new version introduces some new features, but drops some as well (see below).

To migrate to the new version, I propose the following process:

  • clean up the prototype
  • update documentation
  • add migration guide
  • create colcon branch upstream (from master)
  • create pull request to discuss the changes
  • create legacy branch (same as master for some time)
  • make legacy branch default, keep master updated
  • Announce on ROS discourse ([1])
  • make colcon the default later (when?),delete master

@130s, @miguelprada: What do you think?

Features of the new version:

  • Support for colcon (melodic and newer) AND catkin_tools (ROS1 only)
  • Support for ROS1 and ROS2 (#333)
  • Support for vcstool (#357) and flexible confguration (#147)
  • Strict separation of upstream, target and downstream packages (#106)
  • Refactored workspace management
  • Uses official ROS docker images per default
  • Hook script system (#272)
  • Source folder is writable (#210)
  • Prerelease tests for ROS1 and ROS2 with support for upstream and downstream packages

Dropped featrues:

  • Support for Hydro
  • Support for devel space
  • enforcing of 4 build jobs by defaults
  • testing installed *.test files (#252)

Not yet migrated (-> unchanged, still works):

  • ABI check (ROS1 only)
@130s

This comment has been minimized.

Copy link
Member

commented Apr 8, 2019

Looks great. Thanks so much for taking an initiative on this. No strong opinions from me.

  • I admit I have been ignorant about the changes made so far to industrial_ci. That said, is the plan to provide colcon support in addition to what it's been already available in a single branch in the future? If so that's even greater (and I think that's your plan. Just clarifying). And I assume it'll be backward compatible against non-colcon version of industrial_ci.
  • Is dropping support for devel space well supported idea (I thought there was a discussion in industrial_ci when install became the default but I can't find it)?
    • I discussed with some senior level ros engineers and they didn't like the idea of always using install on developement or for even deployment in some R&D projects (though I insisted using install).

Lastly apology for ignorance...I assume there isn't a CI setup helper in ros2, something easier to setup like ros buildfarm, industrial_ci?

@ipa-mdl

This comment has been minimized.

Copy link
Member Author

commented Apr 8, 2019

That said, is the plan to provide colcon support in addition to what it's been already available in a single branch in the future?

No. The "colcon" branch contains a heavily refactored version of industriaLci to support catkin_tools, colcon, ROS1 and ROS2
And it should become the default after some reasonable transition time (3-6 months from now?).

And I assume it'll be backward compatible against non-colcon version of industrial_ci.

Not for all, but most users.
That's why there will be the "legacy" version as well.

The new version deafults to catkin_tools for ROS1 and colcon for ROS2.

Is dropping support for devel space well supported idea

colcon does not support the devel space anymore (even for ROS1).

I discussed with some senior level ros engineers and they didn't like the idea of always using install on developement or for even deployment in some R&D projects

They don't have to use it for development, but they should add install tags regardless.

@miguelprada

This comment has been minimized.

Copy link
Member

commented Apr 8, 2019

Thanks @ipa-mdl for this thorough revision. I have yet to familiarize with the new codebase, but it does look much better organized than the old version.

The migration path you propose seems reasonable to me.

You can find some additional comments I made regarding the specifics of your colcon branch in #333 (comment).

This was referenced Apr 20, 2019

@ipa-mdl

This comment has been minimized.

Copy link
Member Author

commented May 12, 2019

I just came to the conclusion that it might be easier to just drop the master branch.
For now, legacy would become the default branch and later we can switch to colcon.
This enable the user to select a version or just use the default, without the need of synchronization, which is quite tedious and error-prone.

If it is okay for you, I will make legay the default branch and delete master once the colcon is ready to be announced.
(or just rename master)

@gavanderhoorn

This comment has been minimized.

Copy link
Member

commented May 12, 2019

Would it be an idea to keep master around for a bit, make it basically identical to legacy, but make the scripts in master print a (rather large and hard to miss) warning / notice that the master branch is going to be removed in the near future and that users should switch to either legacy or the new colcon version.

There is a non-zero chance that some setups clone a specific version and/or branch of industrial_ci and it would be unfortunate to break those setups without giving those users a chance to do something about that.

This would be something similar to a tick-tock release process.

@ipa-mdl

This comment has been minimized.

Copy link
Member Author

commented May 12, 2019

There is a non-zero chance that some setups clone a specific version and/or branch of industrial_ci and it would be unfortunate to break those setups without giving those users a chance to do something about that.

Good point!

but make the scripts in master print a (rather large and hard to miss) warning / notice that

Nobody reads warnings, especially if the build passes.
And this would require merges from legacy to update it (instead of pushes).

Would it be an idea to keep master around for a bit, make it basically identical to legacy

As a compromise we could keep master until we switch to the colcon branch as default.
This should get mentioned in the announcement as well.
I have updated the ToDos accordingly.

@ipa-mdl

This comment has been minimized.

Copy link
Member Author

commented May 28, 2019

@miguelprada, @130s: I'd like to switch the default branch to legacy now after #369 was merged, any objections?

@miguelprada

This comment has been minimized.

Copy link
Member

commented May 28, 2019

Nothing other than @gavanderhoorn's suggestion of keeping master around for a bit (and possibly printing out a warning), just in case there's people cloning with -b master.

@ipa-mdl

This comment has been minimized.

Copy link
Member Author

commented Jun 8, 2019

I announced the transition to the legacy branch, but did not mention the new branch name.

colcon might be misleading, any better ideas?

1 . We could stick with colcon for now and move it to master later (this might break existing set-ups, requires early adopters to migrate later).
2. We come up with a more generic term (e.g., nextgen) and treat this as a master/stable.
3. Or we establish a develop branch for colcon and further developments (#129) and sync it over to master eventually (collisions are less likely, early adopters might want to migrate to master later)
4. Like 3., but with stable as the master/default branch (no collisions)

@miguelprada

This comment has been minimized.

Copy link
Member

commented Jun 10, 2019

This search is far from perfect (it possibly searches only default branches), but suggests not many people are cloning master branch explicitly.

Is it so crazy to merge #361 into master and keep legacy as default for a while?

  • It shouldn't break most setups (yet, until switching the default)
  • It sends a clear signal that master (a.k.a. colcon) should be the go-to branch
  • It's consistent with your announcement, which recommends pointing to legacy or default while warning that the latter will be updated

I know I'm contradicting myself here. But most problems are likely to happen when switching the default branch anyways. And the benefits of introducing another named branch may not be worth the added confusion.

@ipa-mdl

This comment has been minimized.

Copy link
Member Author

commented Jun 12, 2019

Is it so crazy to merge #361 into master and keep legacy as default for a while?

No, I agree that it would be the simplest solution - easy to understand for users and easy to maintain.
@130s: Do you agree?

@ipa-mdl

This comment has been minimized.

Copy link
Member Author

commented Jun 20, 2019

@130s: friendly ping

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.