Bug in handling of CONFIG_DOCKER_SEARCH_DIRS config

[paulsonnenschein]

After updating to the DP 3 Release, our current CONFIG_DOCKER_SEARCH_DIRS config isn't handled the same anymore.

It seems that build_base_image.sh now always prepends the path to the isaac_ros_common/scripts/ folder to each listed element, even when using absolute paths.

Im guessing this is a bug in this line: https://github.com/NVIDIA-ISAAC-ROS/isaac_ros_common/blob/c3f4ce59b15ce025d9c6de00c5d6b031ff07ff8a/scripts/build_base_image.sh#L32

Changing the comparison to != '/'* change fixed it for me, but im not a bash expert so this might not be the proper solution.

[hemalshahNV]

I think you're right @paulsonnenschein, it should be '/'*. Nice catch! I'll spin up a PR for you to review shortly after I run the change through some tests locally first.

With enabling BuildKit, we now rely on CONFIG_DOCKER_SEARCH_DIRS much more internally because symlinks are not longer allowed which prompted some of the reworking around the feature.

[ikhann]

Hi, This update causes an error if one of the custom docker files is outside of "isaac_ros_common/scripts" folder.

I'm a bit confused, could you please explain why Dockerfiles have to be in a specific location? I see in the example provided in your README, the user Dockerfiles can be located anywhere.

As an example, I currently have a list of directories included in my Docker search list:

1. /home/daniil/repos/squidrc-ros, my custom Dockerfile is here
2. /home/daniil/repos/squidrc-ros/rc_ws/src/isaac_ros_common/scripts/../docker, Isaac ros Dockerfies are here.

So, when I run a building process, I get the wrong path by adding "/home/daniil/repos/squidrc-ros/rc_ws/src/isaac_ros_common/scripts/" to {1} path:

[paulsonnenschein]

I think you're right @paulsonnenschein, it should be '/'*. Nice catch! I'll spin up a PR for you to review shortly after I run the change through some tests locally first.

With enabling BuildKit, we now rely on CONFIG_DOCKER_SEARCH_DIRS much more internally because symlinks are not longer allowed which prompted some of the reworking around the feature.

Do you have an update on this @hemalshahNV ?

[harishkumarbalaji]

I could see this bug still persists, Have raised a PR to fix this - https://github.com/NVIDIA-ISAAC-ROS/isaac_ros_common/pull/89

[hemalshahNV]

Thanks for your patience, @paulsonnenschein, and your PRs (@harishkumarbalaji)! I finally got a chance to dig into this and verify your fix. The issue was recognizing whether a path was absolute or relative. The following is a distilled version of that check:

#!/bin/bash -x

if [[ "$1" != /* ]]; then
        echo "Relative path"
else
        echo "Absolute path"
fi

Removing the asterisks is not enough it turns out. We also need to remove the quotes around /* too.

The below is the patch going into the next Isaac ROS release with a slight modification of your changes @harishkumarbalaji

diff --git a/scripts/build_base_image.sh b/scripts/build_base_image.sh
index 52f0707..c619468 100755
--- a/scripts/build_base_image.sh
+++ b/scripts/build_base_image.sh
@@ -29,7 +29,9 @@ if [[ -f "${ROOT}/.isaac_ros_common-config" ]]; then
     # Prepend configured docker search dirs
     if [ ${#CONFIG_DOCKER_SEARCH_DIRS[@]} -gt 0 ]; then
         for (( i=${#CONFIG_DOCKER_SEARCH_DIRS[@]}-1 ; i>=0 ; i-- )); do
-            if [[ "${CONFIG_DOCKER_SEARCH_DIRS[i]}" != '/*'* ]]; then
+
+            # If the path is relative, then prefix ROOT to the path
+            if [[ "${CONFIG_DOCKER_SEARCH_DIRS[i]}" != /* ]]; then
                 CONFIG_DOCKER_SEARCH_DIRS[$i]="${ROOT}/${CONFIG_DOCKER_SEARCH_DIRS[i]}"
             fi
         done

[hemalshahNV]

This fix is in Isaac ROS 2.0.0 which is now available. Please check again and close as appropriate.

[paulsonnenschein]

I finally got around to upgrading to the 2.0 release. I can confirm it works as expected now 👍