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.

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

Deprecate get_env.h and move content to env.{h,c} #340

Merged

Conversation

christophebedard
Copy link
Member

Resolves these TODOs:

// TODO(cottsay): Deprecate get_env.h and eventually merge it here

// TODO(cottsay): Move the stuff in get_env.c in here

I think the intention was originally to deprecate in Galactic and remove in H-turtle (#250 (comment)), but it missed the ⛵ so it'll have to be deprecated in H-turtle and removed in I-turtle. I can create an issue for the removal after this is merged.

Signed-off-by: Christophe Bedard <bedard.christophe@gmail.com>
Signed-off-by: Christophe Bedard <bedard.christophe@gmail.com>
Signed-off-by: Christophe Bedard <bedard.christophe@gmail.com>
Signed-off-by: Christophe Bedard <bedard.christophe@gmail.com>
Signed-off-by: Christophe Bedard <bedard.christophe@gmail.com>
Signed-off-by: Christophe Bedard <bedard.christophe@gmail.com>
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

A few minor things to fix up, but this is a good cleanup. Thanks!

include/rcutils/env.h Outdated Show resolved Hide resolved
include/rcutils/env.h Outdated Show resolved Hide resolved
include/rcutils/get_env.h Show resolved Hide resolved
This reverts commit 4eb6f85.

Signed-off-by: Christophe Bedard <bedard.christophe@gmail.com>
Signed-off-by: Christophe Bedard <bedard.christophe@gmail.com>
Copy link
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

looks good to me

Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

Looks good to me with green CI.

@christophebedard
Copy link
Member Author

Should I do a CI run for this PR + the other related PRs all at once?

@clalancette
Copy link
Contributor

Should I do a CI run for this PR + the other related PRs all at once?

Yeah, that would be my suggestion. And in this case, since we are touching so many different packages, I'll just suggest doing one run where we build and test everything.

@christophebedard
Copy link
Member Author

Test failures on macOS look unrelated.

@clalancette
Copy link
Contributor

Test failures on macOS look unrelated.

Yeah, agreed. Annoying, because it looks new, but not related to this PR.

@clalancette clalancette merged commit 1570091 into ros2:master May 4, 2021
@christophebedard christophebedard deleted the deprecate-rcutils-get-env-header branch May 4, 2021 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants