ros-industrial / industrial_ci

Easy continuous integration repository for ROS repositories
Apache License 2.0
248 stars 129 forks source link

Refactor ROS_REPO handling #748

Closed mathias-luedtke closed 3 years ago

mathias-luedtke commented 3 years ago

ROS_REPO should not overwrite an existing ROS setup by default (fixes #744) If ROS_REPO does no match the existing ROS setup, a warning will be raised.

mathias-luedtke commented 3 years ago

@InigoMoreno: Did you already have a chance to test this?

InigoMoreno commented 3 years ago

I got warned Using default ROS_REPO=testing and was able to fix my issue using ROS_REPO=false

mathias-luedtke commented 3 years ago

I got warned Using default ROS_REPO=testing

You should not see this in your use case..

mathias-luedtke commented 3 years ago

@inigomartinez: Please test again with the debug commit as soon as I fixed the build errors ;)

mathias-luedtke commented 3 years ago

My test cases pass and show the expected warnings:

InigoMoreno commented 3 years ago

Running the same as your test case but with ros:melodic gives exit code 1:

rosrun industrial_ci run_ci DOCKER_IMAGE=ros:melodic\
                              AFTER_INIT="grep -r ros-testing /etc/apt && ici_exit 1 || ici_exit 2"
mathias-luedtke commented 3 years ago

Running the same as your test case but with ros:melodic gives exit code 1:

I added a test case for this, but it returns 2. Did you test the right version? Without the patches in this PR, the result will be 1. (which is what I tried to fix)

mathias-luedtke commented 3 years ago

rosrun industrial_ci run_ci

Normally I just run it directly to not depend on a ROS build.

industrial_ci/scripts/run_ci DOCKER_IMAGE=ros:melodic AFTER_INIT="grep -r ros-testing /etc/apt && ici_exit 1 || ici_exit 2"
InigoMoreno commented 3 years ago

I made sure I am on the correct branch, but I still get error code 1:

$ git remote get-url origin                                    
https://github.com/ipa-mdl/industrial_ci

$ git status                                              
On branch fix/docker-image-ros-repo
Your branch is up to date with 'origin/fix/docker-image-ros-repo'.

nothing to commit, working tree clean

$ industrial_ci/scripts/run_ci DOCKER_IMAGE=ros:melodic\          
           AFTER_INIT="grep -r ros-testing /etc/apt && ici_exit 1 || ici_exit 2" > /dev/null
FAIL

$ echo $?                                                           
1
InigoMoreno commented 3 years ago

Please check the comment I left in the review.

mathias-luedtke commented 3 years ago

I made sure I am on the correct branch, but I still get error code 1:

Did you fetch the latest updates?

git fetch origin

Please check the comment I left in the review.

I can't see any.

InigoMoreno commented 3 years ago

Did you fetch the latest updates?

Yes

I can't see any

I'll just paste it here: When testing this with ros:melodic I get:

apt-cache policy
N: Unable to locate package ros-noetic-ros-core
Using default ROS_REPO=testing

Is it possible it is getting the $ROS_DISTRO from my current workspace (noetic) instead of the one in the DOCKER_IMAGE (melodic)?

mathias-luedtke commented 3 years ago

Is it possible it is getting the $ROS_DISTRO from my current workspace (noetic) instead of the one in the DOCKER_IMAGE (melodic)?

Yes, this is the case. (https://github.com/ros-industrial/industrial_ci/issues/277)

InigoMoreno commented 3 years ago

Yes, this is the case. (#277)

Okay, then I ran tests from a dind image and got it to work as I expected. LGTM!

mathias-luedtke commented 3 years ago

I will clean this up and merge it later

InigoMoreno commented 3 years ago

I will clean this up and merge it later

Any update on this?

mathias-luedtke commented 3 years ago

Any update on this?

I pushed the clean version, will merge when all tests have passed.