add build tool option

[dirk-thomas]

This patch is a combination of #548 and #577. With more and more changes coming in I think it isn't feasible to keep the colcon specific branch from #548 separate for long term. Therefore this PR adds a build_tool option to the source and doc build files to choose the build tool (catkin_make_isolated vs. colcon). In order to distinguish between ROS 1 and ROS 2 (which in certain parts require different handling) another option - ros_version is being added to the index file.

To ease the review the following changes are in separate commit:

• rename modules and scripts to remove catkin from their names (without any changes to the files)
• update code to use renamed directories / scripts / modules
• add ros_version and build_tool configuration options
• add ROS 2 and colcon specific builds to Travis: devel, prerelease and external-prerelease for ROS 2 Bouncy as well as a ROS 1 devel build using colcon

---

• The last commit only exists to allow testing this branch. It must be removed before merging this PR.

[nuclearsandwich]

I'm about 3/4 through a review but need to wrap for the day. I've raised one question but nothing blocking has come up. Since this adds features that allow a mixed mode for ROS 1 and ROS 2 are you planning to do any testing of a merged rosdistro index before merging?

[tfoote]

I noticed ('catkin_make_isolated', 'colcon') called out in code in several places, but didn't see a good place to centralize them into a single list of valid build tools that could be referenced multiple times.

And the script replacement seems fragile, but I'm assuming that was done because parameterizing the scripts would be a significant effort.

[tfoote]

This seems fragile for longer term maintenance.

[dirk-thomas]

It is, but I don't see a good way to change this at the moment. Proposals how to refactor it are of course welcome.

[dirk-thomas]

Since this adds features that allow a mixed mode for ROS 1 and ROS 2 are you planning to do any testing of a merged rosdistro index before merging?

Can you clarify what you mean with "mixed mode" here?

Each distro has exactly one ROS version associated to it - so a single build never uses more than one ROS version.

[nuclearsandwich]

Since this adds features that allow a mixed mode for ROS 1 and ROS 2 are you planning to do any testing of a merged rosdistro index before merging?

Can you clarify what you mean with "mixed mode" here?

Yeah that was a poor wording. Let me try to rephrase:

This script allows us to run different behavior for ros1 and ros2 distros. Were you planning to do any testing of a unified rosdistro config and/or ros_buildfarm_config before merging? Or just deploy it to both buildfarms and let any issues that come with running a unified buildfarm come up when we are indeed running one.

[dirk-thomas]

Were you planning to do any testing of a unified rosdistro config and/or ros_buildfarm_config before merging?

I see that as a separate step. Atm we are using two different branches of this package for the two Jenkins instances. And each of them uses it's own rosdistro repo.

This patch only aims to consolidate the branches - not yet use a single rosdistro repo.

The one area were i expect to make another PR to before being able to use a single rosdistro repo is for the status page generation. There is logic in there which picks a "previous" distro - that needs to take into account the new ROS version flag in order to support a mixed rosdistro repo (only considering distros with the same ROS version).

The main question is: does this patch change behavior anywhere based on reading the patch (especially for ROS 1) which needs to be tested beyond the Travis tests? I don't expect it to but there is of course always a chance of a regression.

[dirk-thomas]

I'm about 3/4 through a review

@nuclearsandwich Did you have a chance to finish reviewing the other 1/4?

[dirk-thomas]

I'm not sure if they should be changed here, or if I'm doing something wrong:

Neither of the two items contain enough context. Please describe in detail what you are trying to do.

[nuclearsandwich]

Raised one new question about hard-coding colcon packages to install in two different places. Other than that and the issues raised by other reviewers the code looks sound to me. I've not deployed it anywhere to test, which it sounds like @cottsay has been doing.

[dirk-thomas]

Thank you for the feedback.

If there are no further comments I plan to squash the latest commits and merge this on Monday morning (in order to deploy this on the live farms during business days rather than over the weekend).

[dirk-thomas]

I updated this PR to match the change described in ros2/ros_buildfarm_config#12 (comment).

[dirk-thomas]

Requires ros-infrastructure/rosdistro#124 to pass CI, be merged and released.

[dirk-thomas]

The updated commits are only a squashed version of the previous state.

The last commit should be removed before merging this (without squashing).

[nuclearsandwich]

Overall things look good.

I have one question in the interest of edification: Why are we installing colcon from pip rather than from the apt repositories?

[dirk-thomas]

Why are we installing colcon from pip rather than from the apt repositories?

Can you clarify which location you are referring to? The devel task installs the Python 3 Debian packages (https://github.com/ros-infrastructure/ros_buildfarm/pull/585/files#diff-90baed7be1747bb986535aa30818e5d8R108), so does the doc task (https://github.com/ros-infrastructure/ros_buildfarm/pull/585/files#diff-2875658fd26a99ebc3e4697f62421507R529).

[nuclearsandwich]

Can you clarify which location you are referring to? The devel task installs the Python 3 Debian packages... so does the doc task.

Ah, you're right. I had a wire crossed looking at this line

ros_buildfarm/ros_buildfarm/templates/devel/devel_task.Dockerfile.em

Lines 57 to 58 in e479474

	@[if build_tool == 'colcon']@
	RUN python3 -u /tmp/wrapper_scripts/apt.py update-install-clean -q -y python3-pip

Is pip still needed for colcon and if so what for?

[dirk-thomas]

Is pip still needed for colcon and if so what for?

I don't see a reason why it would still be necessary. I will try to remove it in fd83c76 and if that passes CI squash the commit into one of the other ones.

[dirk-thomas]

I don't see a reason why it would still be necessary.

My memory is slowly coming back: https://github.com/colcon/colcon-core/blob/83bf0391323514ab8ada7aacc693fd16f59c1037/colcon_core/package_identification/python.py#L11-L23

'setuptools' needs to be at least version 30.3.0, if a newer version is not available from the package manager use 'pip3 install -U setuptools' to update to the latest version

I added a comment to it instead of removing it: c8c376c

Re-reviewing after churn

[cottsay]

Things like assert self.build_tool in ('catkin_make_isolated', 'colcon') appear in quite a few spots - seems a good candidate for moving to common.py. Other than that, I don't have any actionable feedback.

[dirk-thomas]

seems a good candidate for moving to common.py

I don't think it's worth moving into a single location. I each case the condition checks that the input to the following logic is ok. So I don't see moving this condition into a single place as strictly benefitial

[dirk-thomas]

I squashed the last commit and remove the commit which alters the branch name to point to this PR. CI is expected to fail for this now.

Merging and 🤞

[dirk-thomas]

Recently triggered devel jobs are failing: e.g. http://build.ros.org/job/Mdev__common_msgs__ubuntu_bionic_amd64/11/

While this could be addressed by reconfiguring the jobs (which will add the missing --build-tool) argument I would rather add another commit to gracefully work without requiring that...

[dirk-thomas]

This change introduced a regression, see the failing tests in this devel job: http://build.ros.org/job/Mdev__ros_comm__ubuntu_bionic_amd64/70/