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

Fix env behavior to return true on empty vars #97

Merged
merged 7 commits into from
Sep 28, 2020

Conversation

mjcarroll
Copy link
Contributor

Empty environment variables are valid on Linux (and other posix systems), we should return true in those cases.

Closes #96

Signed-off-by: Michael Carroll michael@openrobotics.org

@github-actions github-actions bot added 🏰 citadel Ignition Citadel 📜 blueprint Ignition Blueprint 🔮 dome Ignition Dome labels Sep 18, 2020
Signed-off-by: Michael Carroll <michael@openrobotics.org>
@codecov
Copy link

codecov bot commented Sep 18, 2020

Codecov Report

Merging #97 into ign-common3 will increase coverage by 0.01%.
The diff coverage is 85.00%.

Impacted file tree graph

@@               Coverage Diff               @@
##           ign-common3      #97      +/-   ##
===============================================
+ Coverage        73.94%   73.95%   +0.01%     
===============================================
  Files               69       69              
  Lines             9390     9408      +18     
===============================================
+ Hits              6943     6958      +15     
- Misses            2447     2450       +3     
Impacted Files Coverage Δ
include/ignition/common/Util.hh 100.00% <ø> (ø)
src/Util.cc 83.75% <85.00%> (-0.05%) ⬇️

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 1dd5336...df8c1ab. 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.

Due to the different behavior on Windows, it sounds like we shouldn't rely on empty env vars for any cross-platform code 😕

Besides that and the ABI issue, the rest looks good to me.

include/ignition/common/Util.hh Outdated Show resolved Hide resolved
src/Util_TEST.cc Outdated Show resolved Hide resolved
@mjcarroll
Copy link
Contributor Author

Due to the different behavior on Windows, it sounds like we shouldn't rely on empty env vars for any cross-platform code confused

I believe that is true, but it also breaks expectations on Linux a bit if you are just checking for a value to be set rather than checking the value itself.

Signed-off-by: Michael Carroll <michael@openrobotics.org>
Signed-off-by: Michael Carroll <michael@openrobotics.org>
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.

Code checker is angry:

   ./src/Util_TEST.cc:181:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]
  ./src/Util.cc:342:  Missing space before ( in if(  [whitespace/parens] [5]
  ./src/Util.cc:355:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]
  ./include/ignition/common/Util.hh:220:  Line ends in whitespace.  Consider deleting these extra spaces.  

And Windows CI failed:

D:\Jenkins\workspace\ignition_common-ci-pr_any-windows7-amd64\ws\ign-common\src\Util_TEST.cc(179): error C3861: 'setenv': identifier not found

src/Util.cc Show resolved Hide resolved
Clears up differences between platforms

Signed-off-by: Michael Carroll <michael@openrobotics.org>
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 chapulina September 28, 2020 16:10
@chapulina chapulina merged commit 053083e into ign-common3 Sep 28, 2020
@chapulina chapulina deleted the fix_env_behavior branch September 28, 2020 17:42
mjcarroll added a commit that referenced this pull request Oct 14, 2020
Signed-off-by: Michael Carroll <michael@openrobotics.org>
mjcarroll added a commit that referenced this pull request Oct 19, 2020
Signed-off-by: Michael Carroll <michael@openrobotics.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📜 blueprint Ignition Blueprint 🏰 citadel Ignition Citadel 🔮 dome Ignition Dome
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants