ros-tooling / action-ros-ci

Github Action to build and test ROS 2 packages using colcon
Apache License 2.0
145 stars 53 forks source link

Support supplementary repo files #108

Closed jacobperron closed 4 years ago

jacobperron commented 4 years ago

Description

In addition to the vcs-repo-file-url input, it would be nice to be able to pass supplemental repo files. The use-case is for projects that require more than one repository that are not part of the core repositories listed at https://github.com/ros2/ros2. Rather than making a copy of the default repositories file and appending to it, it would be nicer to just be able to provide one or more additional files with the extra repositories.

Completion Criteria

Implementation Notes / Suggestions

I think it makes sense to add a new input for supplemental repo files, e.g. vcs-supplemental-repo-file-urls.

Alternatively, the existing input, vcs-repo-file-url, could be refactored to take more than one file. But, I lean towards the first option so we don't have to explicitly pass the base file all the time.

The latter repo files should act as overlays to the previous. I.e. if a repository appears in multiple repo files, then the version of the repository specified in the latter repo file should be used.

Testing Notes / Suggestions

emersonknapp commented 4 years ago

We are thinking that instead of this - we provide another atomic action that combines repo files as an option. Discussion open

seanyen commented 4 years ago

In ROS 1, wstool can do wstool merge xxx.rosinstall to add/override a upstream .rosinstall files. This is handy in the past for our team bootstrapping ROS Melodic on Windows and running CI with patched repositories. If this is also a valid and something ppl need for ROS 2, I would lean toward making this feature happen in vcstool, so every downstream tools can benefit from that.

piraka9011 commented 4 years ago

...I would lean toward making this feature happen in vcstool, so every downstream tools can benefit from that.

cc: @dirk-thomas

jacobperron commented 4 years ago

If this is also a valid and something ppl need for ROS 2, I would lean toward making this feature happen in vcstool, so every downstream tools can benefit from that.

Doesn't vcs import have the same affect as wstool merge? For our ROS 2 CI jobs, we are running vcs import for each repos file we want to merge with the workspace:

https://github.com/ros2/ci/blob/7e2a41d4c09b907dba2cf1cc0ded6a16b0a43e13/ros2_batch_job/__main__.py#L654-L655

jacobperron commented 4 years ago

We are thinking that instead of this - we provide another atomic action that combines repo files as an option. Discussion open

Adding a new action for this is okay with me. I don't have a preference. Maybe listing some pros/cons for each approach would help.

piraka9011 commented 4 years ago

For our ROS 2 CI jobs, we are running vcs import for each repos file we want to merge with the workspace.

I didn't know this, that is good to know. :+1:

seanyen commented 4 years ago

Doesn't vcs import have the same affect as wstool merge? For our ROS 2 CI jobs, we are running vcs import for each repos file we want to merge with the workspace:

https://github.com/ros2/ci/blob/7e2a41d4c09b907dba2cf1cc0ded6a16b0a43e13/ros2_batch_job/__main__.py#L654-L655

Ah, I didn't know vcstool can do the multiple import and merge .repos files. In this case, you can totally disregard my comments. :)

Karsten1987 commented 4 years ago

I just came across that exact use case (pulling in additional sources) and tried to do this with plain github action commands. In the following snippets, rmw_iceoryx is the repo I am placing the GitHub actions, iceoryx is the additional repo I am trying to place into my workspace:

      - name: Checkout Iceoryx
        uses: actions/checkout@v2
        with:
          repository: eclipse/iceoryx
          path: ros_ws/src/iceoryx
          ref: master

However, when I then try to execute the compilation with

        uses: ros-tooling/action-ros-ci@0.0.13
        with:
          package-name: rmw_iceoryx_cpp iceoryx_ros2_bridge iceoryx_posh iceoryx_utils

this fails with:

  Package 'rmw_iceoryx_cpp' specified with --packages-up-to was not found
  Package 'iceoryx_ros2_bridge' specified with --packages-up-to was not found
  Package 'iceoryx_posh' specified with --packages-up-to was not found
  Package 'iceoryx_utils' specified with --packages-up-to was not found

Is there a chance that the workspace gets modified in between? I don't really understand why the additional repos are not available in the workspace.

Full GitHub action workflow can be found here: https://github.com/ros2/rmw_iceoryx/pull/18

thomas-moulard commented 4 years ago

@Karsten1987 Sorry for not answering your question directly, I don't have an answer from the top of my head. Why don't you generate a VCS repo with iceory, and let the action do the setup for you? You could put it on gist, for instance?

I think making vcs-repo-url a list is OK to support this use case. Not sure if I can get someone to send a PR for that, but that should be an easy fix if you'd like to give it a shot.

Karsten1987 commented 4 years ago

agreed, I could do this. But I am generally not a big fan of it since we then have to maintain a copy of that ros2.repos + iceoryx file. I really only try to pull in an additional external repo - or as the motivation of this ticket is to add an addtional repos file.

thomas-moulard commented 4 years ago

Fixed by https://github.com/ros-tooling/action-ros-ci/pull/121. Will close once the repo is released.

rotu commented 4 years ago

@thomas-moulard Did you forget to close this issue? Ah okay, I see. The latest released version is a few days behind master.

Karsten1987 commented 4 years ago

I don't think that this issue should be closed. See here:https://github.com/ros-tooling/action-ros-ci/pull/121#issuecomment-610060636

thomas-moulard commented 4 years ago

@rotu - yes I'd like to release the repo before closing this.

@Karsten1987 - your problem actually is independent from this issue. What I told you is just plain wrong. It's impossible to store repos files in a repo and use this action the way it's designed now, whether it's one repo or not. I'll open another issue to track your problem, in particular. Sorry for the confusion, and thanks for baring with me, I really had to take a look at rmw_iceoryx myself to understand the issue /facepalm

thomas-moulard commented 4 years ago

A new release is out, closing now.