-
Notifications
You must be signed in to change notification settings - Fork 154
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Log environment variables with which gazebo was launched #680
base: ros2
Are you sure you want to change the base?
Conversation
This can be useful for debugging scenarios where we want to debug why a simulation is not loading the correct files. Signed-off-by: Arjo Chakravarty <[email protected]>
This is something usefull, let me know when it's ready for review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great idea! Just a small comment.
@@ -126,7 +127,9 @@ def launch_gz(context, *args, **kwargs): | |||
else: | |||
on_exit = None | |||
|
|||
return [ExecuteProcess( | |||
return [ | |||
LogInfo(f"Launching gazebo with the environment variables: {env}"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if it's always safe to print the environment variables. Maybe be it can be enabled via a launch config?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about using a LogDebug
? So by default it wont be enabled but if we set the log level to debug the information is shown.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer to be more explicit. Consider the scenario that ROS log messages are being sent to a remote logging service. If env
contains credentials, then this is a potential leak.
Signed-off-by: Arjo Chakravarty <[email protected]>
if debug_env == 'true': | ||
return [ | ||
LogInfo(f"Launching gazebo with the environment variables: {env}"), | ||
ExecuteProcess( | ||
cmd=[exec, exec_args, '--force-version', gz_version], | ||
name='gazebo', | ||
output='screen', | ||
additional_env=env, | ||
shell=True, | ||
prefix=debug_prefix, | ||
on_exit=on_exit | ||
)] | ||
else: | ||
return [ | ||
ExecuteProcess( | ||
cmd=[exec, exec_args, '--force-version', gz_version], | ||
name='gazebo', | ||
output='screen', | ||
additional_env=env, | ||
shell=True, | ||
prefix=debug_prefix, | ||
on_exit=on_exit | ||
)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can deduplicate this
if debug_env == 'true': | |
return [ | |
LogInfo(f"Launching gazebo with the environment variables: {env}"), | |
ExecuteProcess( | |
cmd=[exec, exec_args, '--force-version', gz_version], | |
name='gazebo', | |
output='screen', | |
additional_env=env, | |
shell=True, | |
prefix=debug_prefix, | |
on_exit=on_exit | |
)] | |
else: | |
return [ | |
ExecuteProcess( | |
cmd=[exec, exec_args, '--force-version', gz_version], | |
name='gazebo', | |
output='screen', | |
additional_env=env, | |
shell=True, | |
prefix=debug_prefix, | |
on_exit=on_exit | |
)] | |
ld = [ExecuteProcess( | |
cmd=[exec, exec_args, '--force-version', gz_version], | |
name='gazebo', | |
output='screen', | |
additional_env=env, | |
shell=True, | |
prefix=debug_prefix, | |
on_exit=on_exit | |
) | |
] | |
if debug_env == 'true': | |
ld.insert(0, LogInfo(f"Launching gazebo with the environment variables: {env}")) | |
return ld |
🎉 New feature
Closes #
Summary
This can be useful for debugging scenarios where we want to debug why a simulation is not loading the correct files.
Test it
Checklist
codecheck
passed (See contributing)Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining
Signed-off-by
messages.