Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Allow configuring logging directory through environment variables #460

Merged
merged 10 commits into from Oct 13, 2020
Merged

Allow configuring logging directory through environment variables #460

merged 10 commits into from Oct 13, 2020

Conversation

christophebedard
Copy link
Member

@christophebedard christophebedard commented Sep 22, 2020 •
edited

This allows configuring the logging directory through environment variables using the following logic:

  • Use $ROS_LOG_DIR if ROS_LOG_DIR is set and not empty.
  • Otherwise, use $ROS_HOME/log, using ~/.ros for ROS_HOME if not set or if empty.

Includes a test.

Perhaps this logic should be moved to launch_ros in the future.

Relates to ros2/rcl_logging#50

See also ros2/rcl_logging#53

@christophebedard
Allow configuring logging directory through environment variables
bfdcc71 
Signed-off-by: Christophe Bedard <bedard.christophe@gmail.com>
@christophebedard christophebedard force-pushed the allow-configuring-logging-dir-through-env-vars branch from 3eae739 to bfdcc71 Compare September 22, 2020 17:16
@ivanpauno ivanpauno self-requested a review September 29, 2020 14:40
ivanpauno
ivanpauno approved these changes Sep 29, 2020
View reviewed changes
Copy link
Member

@ivanpauno ivanpauno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

launch/launch/logging/__init__.py Outdated Show resolved Hide resolved
launch/test/launch/test_logging.py Outdated Show resolved Hide resolved
launch/package.xml Outdated Show resolved Hide resolved
launch/test/launch/test_logging.py Show resolved Hide resolved
launch/test/launch/test_logging.py Show resolved Hide resolved
@christophebedard
Use ROS_* env vars instead of ROS2_*
a68a975 
Signed-off-by: Christophe Bedard <bedard.christophe@gmail.com>
@christophebedard
Copy link
Member Author

Switched to ROS_{LOG_DIR,HOME} as discussed here ros2/rcl_logging#53 (comment)

@christophebedard christophebedard mentioned this pull request Sep 30, 2020
Merged
@christophebedard
Add test that sets both env vars
80ea7fe 
Signed-off-by: Christophe Bedard <bedard.christophe@gmail.com>
@christophebedard
Add comment
46e727d 
Signed-off-by: Christophe Bedard <bedard.christophe@gmail.com>
@christophebedard
Get home directory in a portable way
2ff0698 
Signed-off-by: Christophe Bedard <bedard.christophe@gmail.com>
@christophebedard
Use pathlib instead of os.path
4f9a041 
Signed-off-by: Christophe Bedard <bedard.christophe@gmail.com>
@christophebedard
Use mock from unittest
16dd37a 
Signed-off-by: Christophe Bedard <bedard.christophe@gmail.com>
ivanpauno
ivanpauno reviewed Sep 30, 2020
View reviewed changes
launch/test/launch/test_logging.py Outdated Show resolved Hide resolved
launch/test/launch/test_logging.py Outdated Show resolved Hide resolved
launch/test/launch/test_logging.py Outdated Show resolved Hide resolved
@christophebedard
Replace jointpath() with operator/
0ee368c 
Signed-off-by: Christophe Bedard <bedard.christophe@gmail.com>
@christophebedard christophebedard mentioned this pull request Sep 30, 2020
Merged
@christophebedard
Normalize paths to convert path sep
4aca595 
Signed-off-by: Christophe Bedard <bedard.christophe@gmail.com>
@christophebedard
Add one more path sep test
7bd7e86 
Signed-off-by: Christophe Bedard <bedard.christophe@gmail.com>
@christophebedard
Copy link
Member Author

CI from ros2/rcl_logging#53 including that PR and this one:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@hidmic
Copy link
Contributor

hidmic commented Oct 13, 2020

This one looks ready to go :)

@ivanpauno
Copy link
Member

Thanks for the contribution @christophebedard !

@ivanpauno ivanpauno merged commit d648e19 into ros2:master Oct 13, 2020
@christophebedard christophebedard deleted the allow-configuring-logging-dir-through-env-vars branch October 13, 2020 21:30
@tgreier
Copy link

tgreier commented Nov 13, 2020

Will this PR be merged into Foxy?

@hidmic
Copy link
Contributor

hidmic commented Nov 13, 2020

Looks like a safe backport. @ivanpauno would you mind?

@hidmic hidmic added this to Proposed in Foxy Patch Release 4 via automation Nov 13, 2020
@hidmic hidmic moved this from Proposed to Needs backport in Foxy Patch Release 4 Nov 13, 2020
@ivanpauno
Copy link
Member

If we backport this one, we should also backport ros2/rcl_logging#53 for consistency.
I think that both are "ok" to backport, but we don't really need to do so.

@tgreier feel free to open PRs proposing the addition to Foxy.

@jacobperron
Copy link
Member

Bumping to Foxy patch release 5, since no one has gotten to this yet.

@jacobperron jacobperron added this to Proposed in Foxy Patch Release 5 via automation Dec 8, 2020
@jacobperron jacobperron removed this from Needs backport in Foxy Patch Release 4 Dec 8, 2020
@jacobperron jacobperron moved this from Proposed to Needs backport in Foxy Patch Release 5 Dec 8, 2020
tgreier added a commit to tgreier/launch that referenced this pull request Dec 21, 2020
@tgreier
Alow config log dir via env vars. ros2#460
e1c237d
@tgreier tgreier mentioned this pull request Dec 21, 2020
Merged
tgreier added a commit to tgreier/launch that referenced this pull request Dec 21, 2020
@tgreier
Alow config log dir via env vars. ros2#460
1f594cd 
Signed-off-by: tgreier <tgreier@moog.com>
tgreier pushed a commit to tgreier/launch that referenced this pull request Dec 22, 2020
@christophebedard @tgreier
Allow configuring logging directory through environment variables (ro…
b886a74 
…s2#460)

Signed-off-by: Christophe Bedard <bedard.christophe@gmail.com>
hidmic pushed a commit that referenced this pull request Dec 22, 2020
@tgreier @christophebedard
Allow configuring logging directory through environment variables (#460…
bb375c9 
…) (#477)

Signed-off-by: Christophe Bedard <bedard.christophe@gmail.com>
Co-authored-by: Christophe Bedard <bedard.christophe@gmail.com>
@christophebedard christophebedard mentioned this pull request Mar 2, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ivanpauno ivanpauno ivanpauno approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@christophebedard @hidmic @ivanpauno @tgreier @jacobperron