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.

Sign up for GitHub

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

Jump to bottom

merge env values which were split on semicolons #101

Merged
merged 2 commits into from
Aug 4, 2017
Merged

merge env values which were split on semicolons #101

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0fa8ed8
merge env values which were split on semicolons
dirk-thomas Aug 4, 2017
00e3224
print set env message all at once (#102)
wjwwood Aug 4, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 2 additions & 26 deletions ament_cmake_test/cmake/ament_add_test.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,19 +88,7 @@ function(ament_add_test testname)
list(APPEND cmd_wrapper "--output-file" "${ARG_OUTPUT_FILE}")
endif()
if(ARG_ENV)
list(APPEND cmd_wrapper "--env")
foreach(_env ${ARG_ENV})
# TODO(wjwwood): remove this when we have a better way to pass PATH lists
# to environment variables on Windows.
if(WIN32)
# In order to pass a ;-separated list of paths into this function on
# Windows, the calling code may have had to replace ;'s to avoid it
# them from being interpreted as a CMake list, so undo that
# substitution here.
string(REPLACE "\;" ";" _env "${_env}")
endif()
list(APPEND cmd_wrapper "${_env}")
endforeach()
list(APPEND cmd_wrapper "--env" ${ARG_ENV})
endif()
if(ARG_APPEND_LIBRARY_DIRS)
if(WIN32)
Expand All @@ -117,19 +105,7 @@ function(ament_add_test testname)
endforeach()
endif()
if(ARG_APPEND_ENV)
list(APPEND cmd_wrapper "--append-env")
foreach(_env ${ARG_APPEND_ENV})
# TODO(wjwwood): remove this when we have a better way to pass PATH lists
# to environment variables on Windows.
if(WIN32)
# In order to pass a ;-separated list of paths into this function on
# Windows, the calling code may have had to replace ;'s to avoid it
# them from being interpreted as a CMake list, so undo that
# substitution here.
string(REPLACE "\;" ";" _env "${_env}")
endif()
list(APPEND cmd_wrapper "${_env}")
endforeach()
list(APPEND cmd_wrapper "--append-env" ${ARG_APPEND_ENV})
endif()
list(APPEND cmd_wrapper "--command" ${ARG_COMMAND})

Expand Down
20 changes: 18 additions & 2 deletions ament_cmake_test/cmake/run_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,20 +129,36 @@ def log(msg, **kwargs):
env = dict(os.environ)
if args.env:
log('-- run_test.py: extra environment variables:')
previous_key = None
for env_str in args.env:
key, value = separate_env_vars(env_str, 'env', parser)
# if CMake has split a single value containing semicolons
# into multiple arguments they are put back together here
if previous_key and '=' not in env_str:
key = previous_key
value = env[key] + ';' + env_str
else:
key, value = separate_env_vars(env_str, 'env', parser)
log(' - {0}={1}'.format(key, value))
Copy link
Contributor

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Same below.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤞

env[key] = value
previous_key = key
if args.append_env:
log('-- run_test.py: extra environment variables to append:')
previous_key = None
for env_str in args.append_env:
key, value = separate_env_vars(env_str, 'append-env', parser)
# if CMake has split a single value containing semicolons
# into multiple arguments they are put back together here
if previous_key and '=' not in env_str:
key = previous_key
value = env[key] + ';' + env_str
else:
key, value = separate_env_vars(env_str, 'append-env', parser)
log(' - {0}={1}'.format(key, value))
if key not in env:
env[key] = ''
if not env[key].endswith(os.pathsep):
env[key] += os.pathsep
env[key] += value
previous_key = key

log("-- run_test.py: invoking following command in '%s':\n - %s" %
(os.getcwd(), ' '.join(args.command)))
Expand Down