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

Add rcutils_set_env function #250

Merged
merged 8 commits into from
May 20, 2020
Merged

Add rcutils_set_env function #250

merged 8 commits into from
May 20, 2020

Conversation

cottsay
Copy link
Member

@cottsay cottsay commented May 14, 2020

This change adds a simple function for setting or un-setting an environment variable for the process.

The existing environment variable functions in rcutils are under a header called get_env.h. It might be good to deprecate that header and move the functionality to the more generically-named env.h. I'd love to hear feedback on that.

If we decide to keep a separate get_env.h header moving forward, I should probably rename this one to set_env.h.

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

This change adds a simple function for setting or un-setting an
environment variable for the process.

The existing environment variable functions in rcutils are under a
header called `get_env.h`. It might be good to deprecate that header and
move the functionality to the more generically-named `env.h`.

Signed-off-by: Scott K Logan <logans@cottsay.net>
@cottsay cottsay added enhancement New feature or request in review Waiting for review (Kanban column) labels May 14, 2020
@cottsay cottsay self-assigned this May 14, 2020
Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

LGTM, just check the env vars in the CMake

CMakeLists.txt Outdated Show resolved Hide resolved
@clalancette
Copy link
Contributor

The existing environment variable functions in rcutils are under a header called get_env.h. It might be good to deprecate that header and move the functionality to the more generically-named env.h. I'd love to hear feedback on that.

Yes, I'd be all for that.

include/rcutils/env.h Outdated Show resolved Hide resolved
src/env.c Show resolved Hide resolved
src/env.c Show resolved Hide resolved
cottsay added 2 commits May 14, 2020 10:36
Signed-off-by: Scott K Logan <logans@cottsay.net>
It's too late to deprecate get_env.h in Foxy, but we can at least make
the new header compatible in Foxy.

Signed-off-by: Scott K Logan <logans@cottsay.net>
@cottsay
Copy link
Member Author

cottsay commented May 14, 2020

The existing environment variable functions in rcutils are under a header called get_env.h. It might be good to deprecate that header and move the functionality to the more generically-named env.h. I'd love to hear feedback on that.

Yes, I'd be all for that.

It's too late to deprecate get_env.h in Foxy, but we can at least make the new header compatible in Foxy. We can deprecate get_env.h in G and remove it in H.

I added get_env.h to env.h in 9c67776.

@dirk-thomas
Copy link
Member

What is the reason to add this function at all? What is it needed for?

@cottsay
Copy link
Member Author

cottsay commented May 14, 2020

What is the reason to add this function at all? What is it needed for?

I need to modify the environment for some feature testing in another package, where changing the environment for the entire test isn't desirable. This change clearly demonstrates that there isn't a foolproof cross-platform solution for doing it. If I'm going to do the work to create that mechanism and the accompanying tests, I thought it best to put it somewhere others can use it, especially because there are already some other environment functions to accompany it.

@cottsay
Copy link
Member Author

cottsay commented May 14, 2020

I had another thought while reading the MSDN docs: Since you can't create an empty environment variable on Windows (setting it to "" just removes the variable), should I replicate that behavior on *nix as well, so that the behavior is always consistent across platforms?

@dirk-thomas
Copy link
Member

I don't think prevent users on Linux to do what is possible on that platform is a good idea. I would rather document the difference in the docblock.

@cottsay
Copy link
Member Author

cottsay commented May 14, 2020

I don't think prevent users on Linux to do what is possible on that platform is a good idea. I would rather document the difference in the docblock.

Perfectly acceptable to me. Thanks for the feedback.

Signed-off-by: Scott K Logan <logans@cottsay.net>
*sigh*

Signed-off-by: Scott K Logan <logans@cottsay.net>
@cottsay
Copy link
Member Author

cottsay commented May 14, 2020

I would rather document the difference in the docblock.

Done in 4ba9470

Signed-off-by: Scott K Logan <logans@cottsay.net>
@cottsay cottsay requested a review from clalancette May 14, 2020 22:02
cottsay added 2 commits May 15, 2020 13:48
Signed-off-by: Scott K Logan <logans@cottsay.net>
Signed-off-by: Scott K Logan <logans@cottsay.net>
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,

src/env.c Show resolved Hide resolved
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.

The code looks good to me.

Are you intending to land this for Foxy, or can we defer this to after that?

@cottsay
Copy link
Member Author

cottsay commented May 19, 2020

Are you intending to land this for Foxy...?

It's a well-reviewed and tested addition to the API, and I'd like to use it in some tests in a downstream package, so it would be nice to land in Foxy. This package hasn't bumped to 1.0.0 yet, so I think there is another release that needs to happen anyway. Sound okay, @jacobperron?

@jacobperron
Copy link
Member

jacobperron commented May 20, 2020

@cottsay I think since this is an addition (not modifying existing API), it's fine to land it for Foxy.

@cottsay
Copy link
Member Author

cottsay commented May 20, 2020

...it's fine to land it for Foxy.

Grazie!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request in review Waiting for review (Kanban column)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants