Skip to content

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

Merged
clalancette merged 8 commits intoros2:masterfrom
christophebedard:deprecate-rcutils-get-env-header
May 4, 2021
Merged

Deprecate get_env.h and move content to env.{h,c}#340
clalancette merged 8 commits intoros2:masterfrom
christophebedard:deprecate-rcutils-get-env-header

Conversation

@christophebedard
Copy link
Copy Markdown
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
Copy Markdown
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!

This reverts commit 4eb6f85.

Signed-off-by: Christophe Bedard <bedard.christophe@gmail.com>
Signed-off-by: Christophe Bedard <bedard.christophe@gmail.com>
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member Author

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

@clalancette
Copy link
Copy Markdown
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
Copy Markdown
Member Author

Test failures on macOS look unrelated.

@clalancette
Copy link
Copy Markdown
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