Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Move environment functionality to utils #30

Merged
merged 8 commits into from
Nov 23, 2021
Merged

Conversation

mjcarroll
Copy link
Contributor

Since multiple ignition libraries make use of setting/getting environment variables, it makes more sense to locate it here.

Signed-off-by: Michael Carroll <michael@openrobotics.org>
Signed-off-by: Michael Carroll <michael@openrobotics.org>
Signed-off-by: Michael Carroll <michael@openrobotics.org>
@mjcarroll mjcarroll requested a review from azeey as a code owner November 22, 2021 20:37
@github-actions github-actions bot added 🌱 garden Ignition Garden 🏢 edifice Ignition Edifice 🏯 fortress Ignition Fortress labels Nov 22, 2021
@codecov-commenter
Copy link

codecov-commenter commented Nov 22, 2021

Codecov Report

Merging #30 (ac3a46e) into ign-utils1 (bb3f23a) will increase coverage by 0.36%.
The diff coverage is 95.23%.

Impacted file tree graph

@@              Coverage Diff               @@
##           ign-utils1      #30      +/-   ##
==============================================
+ Coverage       94.00%   94.36%   +0.36%     
==============================================
  Files               3        4       +1     
  Lines              50       71      +21     
==============================================
+ Hits               47       67      +20     
- Misses              3        4       +1     
Impacted Files Coverage Δ
src/Environment.cc 95.23% <95.23%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bb3f23a...ac3a46e. Read the comment docs.

Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

LGTM with green CI

@chapulina chapulina linked an issue Nov 22, 2021 that may be closed by this pull request
Signed-off-by: Michael Carroll <michael@openrobotics.org>
@mjcarroll mjcarroll force-pushed the move_environment branch 2 times, most recently from 041c3ea to 166cd35 Compare November 23, 2021 16:07
Signed-off-by: Michael Carroll <michael@openrobotics.org>
Copy link
Contributor

@azeey azeey left a comment

Choose a reason for hiding this comment

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

LGTM. The only caveat is that I don't think env and setenv are thread safe since the underlying functions are not. I'm not referring to the issue of a global char * getting invalidated by multiple calls to getenv from different threads, but the data race that can occur when both setenv and getenv are called from different threads. The windows documentation recommends synchronizing calls to these functions. cppreference for getenv says:

This function is thread-safe (calling it from multiple threads does not introduce a data race) as long as no other function modifies the host environment. In particular, the POSIX functions setenv(), unsetenv(), and putenv() would introduce a data race if called without synchronization.

Perhaps we can just document this in Environment.hh.

Signed-off-by: Michael Carroll <michael@openrobotics.org>
@mjcarroll
Copy link
Contributor Author

Perhaps we can just document this in Environment.hh.

Added documentation.

Signed-off-by: Michael Carroll <michael@openrobotics.org>
@mjcarroll mjcarroll merged commit 07cb45a into ign-utils1 Nov 23, 2021
@mjcarroll mjcarroll deleted the move_environment branch November 23, 2021 19:59
@osrf-triage
Copy link

This pull request has been mentioned on Gazebo Community. There might be relevant details there:

https://community.gazebosim.org/t/new-ignition-releases-2022-03-01-citadel-edifice-fortress/1313/1

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
🏢 edifice Ignition Edifice 🏯 fortress Ignition Fortress 🌱 garden Ignition Garden
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate environment variable functions to utils
5 participants