-
Notifications
You must be signed in to change notification settings - Fork 30
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
support arm builds again #7
Conversation
@@ -177,7 +177,7 @@ tmpd=`mktemp -d` | |||
mkdir $tmpd/src | |||
curl -sk https://raw.githubusercontent.com/ros2/ros2/master/ros2.repos -o $tmpd/ros2.repos | |||
vcs import $tmpd/src --input $tmpd/ros2.repos | |||
export CI_ARGS="$CI_ARGS --src-mounted --workspace-path $tmpd" | |||
export CI_ARGS="$CI_ARGS -- --src-mounted --workspace-path $tmpd" |
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 logic could end up duplicated in another part of the code and then we might one day end up with --ament-test-args -- -- --src-mounted --workspace-path
(which current makes ament invocations fail). I know it's not an issue for now, but I ticketed it at ament/ament_tools#111
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.
if ament/ament_tools#111 is to be marked as wontfix, I suggest moving the extra delimiter to where any catch-all argument are appended.
Otherwise, if we decide not to append the --ament-test-args
for one reason or another, or we append another argument that is not catch-all before we get to this line, we will end up with -- --src-mounted --workspace-path
which will cause ament to fail
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 good to me, see e.g. https://github.com/ros2/ci/pull/7/files#diff-1cba2a8b70d9923b4b888bca2911fd9aR144
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.
could you add the additional --
delimiter for windows too https://github.com/ros2/ci/blob/bf4179b931f7db8d2a79d45a517db6fb13d52c56/job_templates/ci_job.xml.template#L244..L248
Good catch. Added the trailing delimiter to the windows args. See e.g. https://github.com/ros2/ci/pull/7/files#diff-1cba2a8b70d9923b4b888bca2911fd9aR245 |
--src-mounted --workspace-path
as part of the greedy--ament-test-arg
they need to be prefixed with--
.use_opensplice
flag for ARM jobs will fail the build).false
.