RMW_LIBRARY_PATH patch creates issues when running without Bazel

[matzipan]

The rmw_implementation_library_path.patch creates a nice experience when running via bazel, but it creates a lot of unecessary annoyance when running without. For example, it causes the node processes to look for the rmw lib in external/ros2_rmw_cyclonedds/librmw_cyclonedds.so.

I'm looking to improve this system a bit. Which solution would be acceptable for you?

What if I change the patch to first check at RMW_LIBRARY_PATH and if it's not there fall back to the normal code?

[matzipan]

Maybe something like:

std::shared_ptr<rcpputils::SharedLibrary>
load_library()
{
  // The logic to pick the RMW library to load goes as follows:
  //
  // 1. If the user specified a RMW_LIBRARY_PATH at build time, try that
  //    first.
  // 2. If the user specified the library to use via the RMW_IMPLEMENTATION
  //    environment variable, try to load only that library.
  // 3. Otherwise, try to load the default RMW implementation.
  // 4. If that fails, try loading all other implementations available in turn
  //    until one succeeds or we run out of options.

  const std::string library_path = RMW_LIBRARY_PATH;
  try {
    return std::make_shared<rcpputils::SharedLibrary>(library_path.c_str());
  } catch (const std::exception & e) {}

  std::string env_var;
  try {
    env_var = rcpputils::get_env_var("RMW_IMPLEMENTATION");
  } catch (const std::exception & e) {
    RMW_SET_ERROR_MSG_WITH_FORMAT_STRING(
      "failed to fetch RMW_IMPLEMENTATION "
      "from environment due to %s", e.what());
    return nullptr;
  }

  // User specified an RMW, attempt to load that one and only that one
  if (!env_var.empty()) {
    return attempt_to_load_one_rmw(env_var);
  }

  // User didn't specify, so next try to load the default RMW
  std::shared_ptr<rcpputils::SharedLibrary> ret;

  ret = attempt_to_load_one_rmw(STRINGIFY(DEFAULT_RMW_IMPLEMENTATION));
  if (ret != nullptr) {
    return ret;
  }

  // OK, we failed to load the default RMW.  Fetch all of the ones we can
  // find and attempt to load them one-by-one.
  rmw_reset_error();
  const std::map<std::string, std::string> packages_with_prefixes = ament_index_cpp::get_resources(
    "rmw_typesupport");
  for (const auto & package_prefix_pair : packages_with_prefixes) {
    if (package_prefix_pair.first != "rmw_implementation") {
      ret = attempt_to_load_one_rmw(package_prefix_pair.first);
      if (ret != nullptr) {
        return ret;
      }
      rmw_reset_error();
    }
  }

  // If we made it here, we couldn't find an rmw to load.

  RMW_SET_ERROR_MSG("failed to load any RMW implementations");

  return nullptr;
}

[ahans]

I noticed this as well. Instead of extending the patch, I think it'd be better to remove it completely and instead get closer to the ROS standard setup. When we want to support more than just Cyclone, we will need some mechanism to switch the RMW implementation either way. Instead of inventing something new, sticking to the ROS default is the way to go IMHO.

[mvukov]

This is a valid point. What are your expectations? How would you like to do deployment of a ros2_launch app? Also, if you don't want to make a ros2_launch-like deployment, how would that look like on your end?

In https://github.com/mvukov/rules_ros I also worked out cross-compilation, and there I also worked out https://github.com/mvukov/rules_ros/blob/main/third_party/packaging.bzl, see binary_pkg_tar rule. The intention there was to package a valid runfiles tree (that Bazel generates) in a way that it would work on a target machine. binary_pkg_tar rule piggy-backs on rules_docker (because this way works, and rules_pkg didn't work at that time). Didn't touch this in a while and it's very possible there is a simper solution. One of the issues with rules_pkg is bazelbuild/rules_pkg#115.

My way of thinking was/is: if we use bazel run //path/to:<ros node or ros2_launch> then that should just work without setting any env vars such as RMW_LIBRARY_PATH. Rationale: when we build with Bazel, we know exactly what we are building and there is simply no need for that env var. If we would add another dds impl, then that would be controlled with a flag (using bazel_skylib). In that case we also don't need RMW_LIBRARY_PATH.