-
Notifications
You must be signed in to change notification settings - Fork 119
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
merge env values which were split on semicolons #101
Conversation
ament_cmake_test/cmake/run_test.py
Outdated
key = previous_key | ||
value = env[key] + ';' + env_str | ||
else: | ||
key, value = separate_env_vars(env_str, 'env', parser) | ||
log(' - {0}={1}'.format(key, value)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This log output is going to be strange right? It would print for each "token" in a path for an env variable, where before it would have printed the combined path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the output will contain multiple rows (see referenced test job). I am not sure if a "pretty print" is worth the extra effort / logic. Therefore I didn't implement any merging logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds like extra technical debt to me. I'll try a patch to fix the logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I run another round of CI builds or are you confident the print changes are good? I would be fine to merge it after the current CI passed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Up to you, I ran it locally, but who knows what the CI might turn up. OTOH, we can always fix it up if it breaks something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤞
* print set env message all at once * address comments
Fixes #66.
Instead of introducing a custom separator which doesn't collide with semicolon used for list item separation in CMake this patch modifies the Python script to put the separated arguments back together.
The following test job shows the correct passing of such arguments: http://ci.ros2.org/job/ci_windows/3062/consoleFull (search for
foo=bar
).