Default ROS_HOME and ROS_LOG_DIR env vars in ros2_cpp_test #100

lalten · 2023-04-21T16:06:17Z

Default ROS_HOME and ROS_LOG_DIR env vars in ros2_cpp_test to TEST_UNDECLARED_OUTPUTS_DIR.
This is of course inspired by

rules_ros2/ros2/pytest_wrapper.py.tpl

Lines 38 to 39 in c345187

    
           os.environ['ROS_HOME'] = test_outputs_dir 
        
           os.environ['ROS_LOG_DIR'] = test_outputs_dir

TEST_UNDECLARED_OUTPUTS_DIR is optional(https://bazel.build/reference/test-encyclopedia#initial-conditions), only TEST_TMPDIR is required to be present. However I would argue that currently users have to set this manually already via the env attribute, so there just would not be any change for users where the var is not available.

mvukov · 2023-04-22T10:21:29Z

Your observation regarding TEST_UNDECLARED_OUTPUTS_DIR is spot-on. I would leave Bazel macro as-is, and change the templates (test.py.tpl and pytest_wrapper.py.tpl) directly instead. E.g.

test_outputs_dir = os.environ.get('TEST_UNDECLARED_OUTPUTS_DIR')
if test_outputs_dir is None:
    test_outputs_dir = os.environ.get('TEST_TMPDIR')
if test_outputs_dir is None:
    test_outputs_dir = os.getcwd()

os.environ['ROS_HOME'] = test_outputs_dir
os.environ['ROS_LOG_DIR'] = test_outputs_dir

The second if may still be needed if folks run tests with bazel run.

The rclcpp test is fine for me.

lalten · 2023-04-22T23:44:00Z

leave Bazel macro as-is, and change the templates

Thanks for the feedback! This was a lot more difficult than I anticipated at first :D I wanted to make the setting up ROS_HOME in tests the default, but it wasn't super straightforward to just default set_up_ament to True to actually use the wrapper for all tests: There are some $(rootpath //pkg) that then would become $(rootpaths //pkg), which didn't work out.

Anyways, let me know what you think about this new version :)

Btw, I checked and when you bazel run instead of bazel test a test target you still get Bazel's test env setup, so all the env vars should always be there.

…DECLARED_OUTPUTS_DIR

mvukov

OK, I think you want to be able to override ROS_HOME and ROS_LOG_DIR, even outside the sandbox.

I think the following would do the job in templates (test.py.tpl and pytest_wrapper.py.tpl):

test_outputs_dir = os.environ.get('TEST_UNDECLARED_OUTPUTS_DIR')
if test_outputs_dir is None:
    test_outputs_dir = os.environ.get('TEST_TMPDIR')

if os.environ.get('ROS_HOME') is not None:
    os.environ['ROS_HOME'] = test_outputs_dir

if os.environ.get('ROS_LOG_DIR') is not None:
    os.environ['ROS_LOG_DIR'] = test_outputs_dir

then you can override the both with e.g. --test_env=ROS_HOME=/my/custom/path. This looks to me as a very general approach and avoids a lot of other changes in this PR.

ros2/cc_defs.bzl

ros2/test.py.tpl

.bazelignore

mvukov · 2023-04-25T19:01:10Z

Please take a look at my last comment, where I suggested another setup of relevant ROS env vars.

OK, I think you want to be able to override ROS_HOME and ROS_LOG_DIR, even outside the sandbox.

I think the following would do the job in templates (test.py.tpl and pytest_wrapper.py.tpl):
test_outputs_dir = os.environ.get('TEST_UNDECLARED_OUTPUTS_DIR')
if test_outputs_dir is None:
    test_outputs_dir = os.environ.get('TEST_TMPDIR')

if os.environ.get('ROS_HOME') is not None:
    os.environ['ROS_HOME'] = test_outputs_dir

if os.environ.get('ROS_LOG_DIR') is not None:
    os.environ['ROS_LOG_DIR'] = test_outputs_dir
then you can override the both with e.g. --test_env=ROS_HOME=/my/custom/path. This looks to me as a very general approach and avoids a lot of other changes in this PR.

lalten · 2023-04-25T19:18:39Z

Please take a look at my last comment, where I suggested another setup of relevant ROS env vars.

This is in the templates now:

    test_outputs_dir = os.environ.get('TEST_UNDECLARED_OUTPUTS_DIR')
    if test_outputs_dir is None:
        test_outputs_dir = os.environ.get('TEST_TMPDIR')
    if test_outputs_dir:
        if 'ROS_HOME' not in os.environ and 'ROS_LOG_DIR' not in os.environ:
            os.environ['ROS_HOME'] = test_outputs_dir
            os.environ['ROS_LOG_DIR'] = test_outputs_dir

(I'm now seeing that the second if should also get the is None explicit check)

The fallback from TEST_UNDECLARED_OUTPUTS_DIR to TEST_TMPDIR is as you suggested.

What's different is that I set both ROS_HOME and ROS_LOG_DIR iff neither is set. The reason is that if either one of them is set, rclcpp (and rclpy) will work (ROS_LOG_DIR defaults to $ROS_HOME/.log). The issue I'm trying to fix will only appear if both env vars are unset. The reason I'm setting both, and to the same value, is that this is the current behavior in the pytest wrapper. If it's ok to break backwards compatibility (which we could certainly consider) it would be ok to set ROS_HOME only.

mvukov

IMO, set_up_ros_home arg in set_up_ros_home can be avoided. Please take a look at my comments.

ros2/cc_defs.bzl

ros2/test.py.tpl

mvukov · 2023-04-25T21:03:41Z

ros2/test.py.tpl

+    if 'ROS_HOME' not in os.environ and 'ROS_LOG_DIR' not in os.environ:
+        os.environ['ROS_HOME'] = test_outputs_dir
+        os.environ['ROS_LOG_DIR'] = test_outputs_dir


Why not treat them separately as I suggested?

tbh I think the best way would be to set only ROS_HOME when neither ROS_HOME nor ROS_LOG_DIR is set. But as I said, this isn't the current behavior of the pytest wrapper template, and it seems like a good idea to be consistent with that

ros2/pytest_wrapper.py.tpl

ros2/launch.sh.tpl

lalten · 2023-04-26T13:49:19Z

I've simplified the wrapper setup to remove the setup_ros_home arg, and changed the tests to make sure each of the three test drivers is verified:

ros2_cpp_test: launch.sh.tpl
ros2_py_test: launch.sh.tpl
ros2_test with use_pytest = True: launch.py.tpl
ros2_test with use_pytest = False: pytest_wrapper.py.tpl

lalten · 2023-04-26T13:50:26Z

Btw, I think something is wrong with the CI setup, maybe you could check?

ERROR: The Build Event Protocol upload failed: Not retrying publishBuildEvents, no more attempts left: status='Status{code=UNAUTHENTICATED, description=failed to parse API key: missing API Key, cause=null}' UNAUTHENTICATED: UNAUTHENTICATED: failed to parse API key: missing API Key UNAUTHENTICATED: UNAUTHENTICATED: failed to parse API key: missing API Key

mvukov · 2023-04-27T19:01:44Z

https://github.com/mvukov/rules_ros2/pull/102/files might make problems... I need to investigate this further. The API key for buildbuddy remote cache is stored as a GH secret and it might be that with the new config it's not not available any more -- hence, the error.

You can try to revert that in your branch and see if the CI works then.

mvukov · 2023-04-27T19:09:15Z

Just took a look at https://github.com/aspect-build/rules_js/blob/main/.github/workflows/ci.yaml. I need to fix CI config to work on pull requests from forks w/o github secrets.

…st-env-for-ros2-cpp-test

mvukov

To summarize, this PR addresses 3 issues:

Test size routing in custom test macros was missing, this is fixed now.
Without proper handling of ROS_HOME and ROS_LOGGING_DIR, some tests could escape the sandbox. This PR will fix that.
Besides this, it looks like there are cases when users want to set ROS_HOME and ROS_LOGGING_DIR explicitly in tests. This PR will also allow that.

mvukov · 2023-05-03T07:54:05Z

ros2/cc_defs.bzl

@@ -112,7 +114,7 @@ def ros2_cpp_binary(name, ros2_package_name = None, set_up_ament = False, **kwar
    """
    _ros2_cpp_exec(cc_binary, name, ros2_package_name, set_up_ament, **kwargs)

-def ros2_cpp_test(name, ros2_package_name = None, set_up_ament = False, **kwargs):
+def ros2_cpp_test(name, ros2_package_name = None, set_up_ament = True, **kwargs):


IMO, this change is irrelevant to this PR and we can discuss it in a follow up.

mvukov · 2023-05-03T08:00:25Z

ros2/launch.sh.tpl

@@ -2,6 +2,13 @@

 set -o errexit -o nounset -o pipefail

+if [ -z "${ROS_HOME:-}" ] && [ -z "${ROS_LOG_DIR:-}" ]; then


For relation(s) between ROS_HOME and ROS_LOG_DIR folks should read ROS docs, not docs/impl of this repo. Here we should tend to be as general as possible and avoid introducing hidden functionality. Therefore, I'd ask again for separate setup of those two env vars like I explained earlier.

Next to this, this piece of code is only relevant for tests. Thus, this should look like (for better explicitness IMO):

if [[ ! -z "${BAZEL_TEST}" ]]; then bazel_test_output_dir="${TEST_UNDECLARED_OUTPUTS_DIR:-${TEST_TMPDIR}}" if [[ -z "${ROS_HOME:-}" ]]; then export ROS_HOME="${bazel_test_output_dir}" fi if [[ -z "${ROS_LOG_DIR:-}" ]]; then export ROS_LOG_DIR="${bazel_test_output_dir}" fi fi

mvukov · 2023-05-03T08:01:05Z

ros2/py_defs.bzl

@@ -59,7 +61,7 @@ def ros2_py_binary(name, srcs, main, set_up_ament = False, **kwargs):
    """
    _ros2_py_exec(py_binary, name, srcs, main, set_up_ament, **kwargs)

-def ros2_py_test(name, srcs, main, set_up_ament = False, **kwargs):
+def ros2_py_test(name, srcs, main, set_up_ament = True, **kwargs):


Same here: this is for a follow-up PR.

mvukov · 2023-05-03T08:01:35Z

ros2/pytest_wrapper.py.tpl

+    if 'ROS_HOME' not in os.environ and 'ROS_LOG_DIR' not in os.environ:
+        ros_output_dir = os.environ.get('TEST_UNDECLARED_OUTPUTS_DIR')
+        if ros_output_dir is None:
+            ros_output_dir = os.environ.get('TEST_TMPDIR')
+        if ros_output_dir is not None:
+            os.environ['ROS_HOME'] = ros_output_dir


Please use a snippet I posted earlier. Same reasoning as for launch.sh.tpl.

mvukov · 2023-05-03T08:01:40Z

ros2/test.py.tpl

-if test_outputs_dir:
-    os.environ['ROS_HOME'] = test_outputs_dir
-    os.environ['ROS_LOG_DIR'] = test_outputs_dir
+if 'ROS_HOME' not in os.environ and 'ROS_LOG_DIR' not in os.environ:


Please use a snippet I posted earlier.

mvukov · 2023-05-03T08:02:04Z

ros2/test/BUILD.bazel

-)
+load("@com_github_mvukov_rules_ros2//ros2:cc_defs.bzl", "ros2_cpp_test")
+load("@com_github_mvukov_rules_ros2//ros2:interfaces.bzl", "py_ros2_interface_library", "ros2_interface_library")
+load("@com_github_mvukov_rules_ros2//ros2:py_defs.bzl", "ros2_py_test")


Not needed.

mvukov · 2023-05-03T08:02:22Z

ros2/test/BUILD.bazel

-    "ros2_interface_library",
-)
+load("@com_github_mvukov_rules_ros2//ros2:cc_defs.bzl", "ros2_cpp_test")
+load("@com_github_mvukov_rules_ros2//ros2:interfaces.bzl", "py_ros2_interface_library", "ros2_interface_library")


Please revert formatting as in original.

mvukov · 2023-05-03T08:09:33Z

ros2/pytest_wrapper.py.tpl

+        ros_output_dir = os.environ.get('TEST_UNDECLARED_OUTPUTS_DIR')
+        if ros_output_dir is None:
+            ros_output_dir = os.environ.get('TEST_TMPDIR')
+        if ros_output_dir is not None:


TEST_TMPDIR is always defined, we don't need this if-clause here. The previous line should read: ros_output_dir = os.environ['TEST_TMPDIR'].

BTW, I would rename ros_output_dir to bazel_test_output_dir for more explicitness.

mvukov · 2023-05-03T08:12:37Z

ros2/test/test_rclcpp.cpp

+  // 'Failed to get logging directory, at external/ros2_rcl_logging/rcl_logging_spdlog/src/rcl_logging_spdlog.cpp:83'  // NOLINT
+  // clang-format on
+  rclcpp::init(argc, argv);
+  return 0;


Maybe return rclcpp::shutdown()?

mvukov · 2023-05-07T15:31:22Z

The PR is in good shape now IMO. Thanks for your efforts!

lalten force-pushed the set-bazel-test-env-for-ros2-cpp-test branch from bc07f3a to a4a0435 Compare April 21, 2023 16:27

lalten force-pushed the set-bazel-test-env-for-ros2-cpp-test branch from 81118f5 to 1bfe4fb Compare April 22, 2023 23:43

lalten and others added 2 commits April 23, 2023 01:49

Default ROS_HOME and ROS_LOG_DIR env vars in ros2_cpp_test to TEST_UN…

a004cc2

…DECLARED_OUTPUTS_DIR

Move setup to launch wrapper, add rclpy test

54908cb

lalten force-pushed the set-bazel-test-env-for-ros2-cpp-test branch from 1bfe4fb to 54908cb Compare April 22, 2023 23:50

mvukov reviewed Apr 23, 2023

View reviewed changes

ros2/cc_defs.bzl Show resolved Hide resolved

ros2/cc_defs.bzl Show resolved Hide resolved

ros2/test.py.tpl Outdated Show resolved Hide resolved

.bazelignore Outdated Show resolved Hide resolved

lalten added 2 commits April 23, 2023 11:26

Adhere to Py styleguide; use any existing ROS_HOME

2bea859

undo bazelignore change

90ed041

lalten requested a review from mvukov April 25, 2023 18:42

mvukov reviewed Apr 25, 2023

View reviewed changes

lalten added 2 commits April 26, 2023 15:42

simplify

afd2f59

Test all three Python test drivers

083e371

fix lint

802be17

Merge branch 'main' of github.com:mvukov/rules_ros2 into set-bazel-te…

8362e00

…st-env-for-ros2-cpp-test

mvukov force-pushed the set-bazel-test-env-for-ros2-cpp-test branch from ab04275 to 8362e00 Compare April 29, 2023 20:19

Protect against Processes under test stopped before tests completed

770bcde

lalten requested a review from mvukov April 30, 2023 09:15

mvukov requested changes May 3, 2023

View reviewed changes

lalten added 4 commits May 5, 2023 11:20

format BUILD, add size, return shutdown

7d7e40c

revert default to set_up_ament = False

496d4ac

separate ROS_HOME and ROS_LOG_DIR env vars

8f20de0

Fix lint

f4d189f

lalten requested a review from mvukov May 5, 2023 09:41

mvukov approved these changes May 7, 2023

View reviewed changes

mvukov merged commit bad3cd0 into mvukov:main May 7, 2023

lalten mentioned this pull request May 8, 2023

Default set_up_ament to True for ros2_cpp_test, ros2_py_test #105

Merged

lalten deleted the set-bazel-test-env-for-ros2-cpp-test branch May 8, 2023 07:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Default ROS_HOME and ROS_LOG_DIR env vars in ros2_cpp_test #100

Default ROS_HOME and ROS_LOG_DIR env vars in ros2_cpp_test #100

lalten commented Apr 21, 2023

mvukov commented Apr 22, 2023

lalten commented Apr 22, 2023 •

edited

Loading

mvukov left a comment

mvukov commented Apr 25, 2023

lalten commented Apr 25, 2023 •

edited

Loading

mvukov left a comment

mvukov Apr 25, 2023

lalten Apr 25, 2023

lalten commented Apr 26, 2023

lalten commented Apr 26, 2023

mvukov commented Apr 27, 2023

mvukov commented Apr 27, 2023

mvukov left a comment

mvukov May 3, 2023

mvukov May 3, 2023

mvukov May 3, 2023

mvukov May 3, 2023

mvukov May 3, 2023

mvukov May 3, 2023

mvukov May 3, 2023

mvukov May 3, 2023

mvukov May 3, 2023

mvukov commented May 7, 2023

	os.environ['ROS_HOME'] = test_outputs_dir
	os.environ['ROS_LOG_DIR'] = test_outputs_dir

		@@ -2,6 +2,13 @@

		set -o errexit -o nounset -o pipefail

		if [ -z "${ROS_HOME:-}" ] && [ -z "${ROS_LOG_DIR:-}" ]; then

Default ROS_HOME and ROS_LOG_DIR env vars in ros2_cpp_test #100

Default ROS_HOME and ROS_LOG_DIR env vars in ros2_cpp_test #100

Conversation

lalten commented Apr 21, 2023

mvukov commented Apr 22, 2023

lalten commented Apr 22, 2023 • edited Loading

mvukov left a comment

Choose a reason for hiding this comment

mvukov commented Apr 25, 2023

lalten commented Apr 25, 2023 • edited Loading

mvukov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lalten commented Apr 26, 2023

lalten commented Apr 26, 2023

mvukov commented Apr 27, 2023

mvukov commented Apr 27, 2023

mvukov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvukov commented May 7, 2023

lalten commented Apr 22, 2023 •

edited

Loading

lalten commented Apr 25, 2023 •

edited

Loading