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

[env] Add set_env_var function #150

Merged
merged 4 commits into from
Nov 11, 2021
Merged

[env] Add set_env_var function #150

merged 4 commits into from
Nov 11, 2021

Conversation

aprotyas
Copy link
Member

@aprotyas aprotyas commented Nov 2, 2021

This PR adds the set_env_var function, which is essentially an rcpputils wrapper around rcutils_set_env.

I've also renamed get_env.{hpp,cpp} to env.{hpp,cpp} given the addition of a new method to the "environment helper" functions.

Downstream PRs necessitated by this change:

Closes #149.

Signed-off-by: Abrar Rahman Protyasha [email protected]

This commit adds the `set_env_var` function, which is essentially an
`rcpputils` wrapper around `rcutils_set_env`.

In doing so, also:
- Rename `get_env.{hpp,cpp}` to `env.{hpp,cpp}`
- Update FEATURES.md with new signature

Signed-off-by: Abrar Rahman Protyasha <[email protected]>
Also, renamed `test_get_env.cpp` to `test_env.cpp` to reflect
the addition of a new function.

Signed-off-by: Abrar Rahman Protyasha <[email protected]>
@aprotyas
Copy link
Member Author

aprotyas commented Nov 2, 2021

Running CI.
Repos file: https://gist.github.com/aprotyas/96874af9175859555af0144398e0bf25
Build/Test args: --packages-above-and-dependencies rcpputils
ROS distro: Rolling
Job: https://ci.ros2.org/job/ci_launcher/9287

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

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.

lgtm

@aprotyas
Copy link
Member Author

aprotyas commented Nov 2, 2021

About the macOS test failures: these have been reported as flaky in ros2/rcl#750, and they don't look related. Oddly, these tests also failed in a CI run for ros2/rcl#946.

About the windows test failures: not sure what's going on. All failing tests seem to be of the "graceful termination" type.

It's better to deprecate the header on a tick-tock cycle rather than
removing it outright, so this commit reverts the deletion and actually
deprecates the header.

Signed-off-by: Abrar Rahman Protyasha <[email protected]>
@aprotyas
Copy link
Member Author

aprotyas commented Nov 3, 2021

I just realized that the get_env.hpp header file should not be removed outright, but rather be deprecated in a tick-tock cycle, as was done for rcutils/get_env.h in ros2/rcutils#340.

Having said that, I've reverted the deletion of get_env.hpp and deprecated said header in 18b406b.

@fujitatomoya
Copy link
Collaborator

Ah, i missed that, sorry. there should be a lifecycle for deprecation.

@aprotyas
Copy link
Member Author

aprotyas commented Nov 3, 2021

Running CI again following 18b406b.

Repos file: https://gist.github.com/aprotyas/96874af9175859555af0144398e0bf25
Build args: --packages-above-and-dependencies rcpputils
Test args: --packages-above rcpputils
ROS distro: Rolling
Job: https://ci.ros2.org/job/ci_launcher/9295

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

@aprotyas
Copy link
Member Author

Two approvals and CI is green, so I'll merge this in. Thanks for the reviews!

@aprotyas aprotyas merged commit cb117a5 into master Nov 11, 2021
@aprotyas aprotyas deleted the aprotyas/add_set_env branch November 11, 2021 07:19
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.

Environment helpers: function to set environment variable?
3 participants