ros-tooling / cross_compile

A tool to build ROS and ROS2 workspaces for various targets
Apache License 2.0
188 stars 60 forks source link

Use local mixins in the build, not the remote master copy #38

Closed thomas-moulard closed 4 years ago

thomas-moulard commented 4 years ago

EDIT: Narrowing this to one specific feature request: The colcon mixins in this repository are not directly used, but instead are fetched from the master branch on GitHub. This can result in confusing situations:

Instead we should copy the local mixins into the build container for use.

This should apply to both local developer checkouts and binary installations.


Original conversation for context below. Groomed this bug report as a feature request.

Request is to be able to specify a URI (file:// or http[s]://) for a colcon mixin repository, so that users can use their own custom remote or local mixins in place of the master branch mixins on this project.

Docker ADD can use a URI, so it should be fairly simple as a dockerfile argument https://docs.docker.com/engine/reference/builder/#add

Original Report

Initially reported in #35, but the PR did not address this problem itself.

The following line seems wrong to me:

RUN colcon mixin add cc_mixin \
    https://raw.githubusercontent.com/ros-tooling/cross_compile/master/mixins/index.yaml

Could we copy the mixin and use instead:

RUN colcon mixin add cc_mixin \
    file://.../mixins/index.yaml

The main issue with the current approach is that anyone developing on a feature branch will end up using the mixin from master. So it will be impossible to change the mixin locally w/o changing the Dockerfile.

Generally, it also open the door to other subtle issues where the script and the mixin aren't in sync because they pull from different locations.

thomas-moulard commented 4 years ago

@piraka9011 maybe that could be tackled as part as your oncall?

piraka9011 commented 4 years ago

The reason we do this is because we copy the Dockerfile_workspace to the sysroot directory where all the packages are cross compiled. We don't know where the mixins will be located relative to the sysroot directory and so we can't copy them to the Docker image.

We can add this functionality and it will involve the following:

  1. Add a command to clone the cross_compile in the Dockerfile_workspace.
  2. Point colcon mixin add to the mixin in the cloned repo.
thomas-moulard commented 4 years ago

Well you don't want to clone right, otherwise you're back to step 1 (is it the right branch?). The goal is also to minimize the network access as it increases flakyness.

Could we copy the mixin in the user workspace in this case, for instance? Or pass the mixin path as a Dockerfile param?

piraka9011 commented 4 years ago

Could we copy the mixin in the user workspace in this case, for instance? Or pass the mixin path as a Dockerfile param?

We can do this as well, so copying the mixin to the sysroot and then copying into the Docker image.

zmichaels11 commented 4 years ago

@emersonknapp update with in progress work

emersonknapp commented 4 years ago

There isn't work in progress on this - after reviewing I have retitled and rewritten the issue description to describe the actual problem and solution.