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

Add in utilities needed for log location #155

Merged
merged 10 commits into from
May 6, 2019
Merged

Add in utilities needed for log location #155

merged 10 commits into from
May 6, 2019

Conversation

clalancette
Copy link
Contributor

This PR provides a few new utilities that are needed to put logging in the users ~/.ros/log directory.

Connects to ros2/rcl_logging#9

Not yet ready for review, opening for visibility.

@clalancette clalancette added the in progress Actively being worked on (Kanban column) label May 3, 2019
dirk-thomas
dirk-thomas reviewed May 3, 2019
View reviewed changes
include/rcutils/get_env.h Show resolved Hide resolved
include/rcutils/get_env.h Outdated Show resolved Hide resolved
include/rcutils/get_env.h Outdated Show resolved Hide resolved
include/rcutils/program.h Outdated Show resolved Hide resolved
include/rcutils/program.h Outdated Show resolved Hide resolved
src/get_env.c Outdated Show resolved Hide resolved
src/program.c Outdated Show resolved Hide resolved
wjwwood
wjwwood requested changes May 3, 2019
View reviewed changes
include/rcutils/get_env.h Outdated Show resolved Hide resolved
include/rcutils/get_env.h Outdated Show resolved Hide resolved
include/rcutils/program.h Outdated Show resolved Hide resolved
src/program.c Outdated Show resolved Hide resolved
src/program.c Outdated Show resolved Hide resolved
src/program.c Outdated Show resolved Hide resolved
@clalancette
Add in cross-platform rcutils method to get home directory.
b8159b6 
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette
Add cross-platform way to get the PID and program name.
0adb3ae 
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette
Move basename fetching into rcutils_get_program_name.
736be92 
That way it is easier to use from the outside.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette
Review feedback.
de1bbc3 
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette
Review fixes.
cc25d97 
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette
Fix include guards.
ffa6b42 
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette
Fixes for Windows.
e9a4167 
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette
Wrap check for USERPROFILE in _WIN32.
3352d19 
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
wjwwood
wjwwood approved these changes May 4, 2019
View reviewed changes
Copy link
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

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

lgtm, other than the nitpick about the doc comment, but that could be fixed after green CI or after merge.

@clalancette
Make sure to #include libgen.h
0fe3bab 
We need this to get basename on macOS.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette
Double-stars so doxygen picks it up.
8dee943 
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette clalancette mentioned this pull request May 6, 2019
Merged
@clalancette clalancette added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) in review Waiting for review (Kanban column) labels May 6, 2019
@dirk-thomas dirk-thomas requested a review from wjwwood May 6, 2019 17:35
nuclearsandwich
nuclearsandwich approved these changes May 6, 2019
View reviewed changes
Copy link
Member

@nuclearsandwich nuclearsandwich left a comment

Choose a reason for hiding this comment

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

I've reviewed and the logic grabbing the process basename (which is the bulk of the new code) looks sound.

I did not assess whether the preprocessor conditionals for cross-platform handling are consistent with the rest of rcutils. I believe @clalancette mentioned that he used get_env.c as a template and this is consistent with that, although it is also more involved.

src/process.c
return NULL;
}
#else
#error "Unsupported OS"
Copy link
Member

Choose a reason for hiding this comment

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

I haven't found examples of other cross-platform code where we explicitly error in fall-through cases. I don't know if that's because other code is structured to assume Linux/Unix in the fall-through case or if runtime errors are being used instead.

Copy link
Member

Choose a reason for hiding this comment

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

I had more to say about this, but GitHub deleted my review... I'm fine with it as-is.

Copy link
Member

Choose a reason for hiding this comment

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

I had more to say about this, but GitHub deleted my review...

:rage4:

@nuclearsandwich nuclearsandwich merged commit b2f16e8 into master May 6, 2019
@delete-merged-branch delete-merged-branch bot deleted the log_location_fix branch May 6, 2019 20:22
@nuclearsandwich nuclearsandwich removed the in review Waiting for review (Kanban column) label May 6, 2019
@nuclearsandwich nuclearsandwich mentioned this pull request May 6, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dirk-thomas dirk-thomas dirk-thomas left review comments

@nuclearsandwich nuclearsandwich nuclearsandwich approved these changes

@wjwwood wjwwood Awaiting requested review from wjwwood wjwwood is a code owner

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

Successfully merging this pull request may close these issues.
4 participants
@clalancette @wjwwood @nuclearsandwich @dirk-thomas