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(docker): avoid empty string in bake's arguments when targets not specified #3303

Merged

Conversation

kazuki0824
Copy link
Contributor

@kazuki0824 kazuki0824 commented Mar 2, 2023

Description

Pre-review checklist for the PR author

The PR author must check the checkboxes below when creating the PR.

In-review checklist for the PR reviewers

The PR reviewers must check the checkboxes below before approval.

Post-review checklist for the PR author

The PR author must check the checkboxes below before merging.

  • There are no open discussions or they are tracked via tickets.

After all checkboxes are checked, anyone who has write access can merge the PR.

@kazuki0824 kazuki0824 force-pushed the fix/docker_sh_trim_blank_arg branch from 13d5110 to 99a61b9 Compare March 2, 2023 15:11
…ets` not specied

Signed-off-by: maleicacid <kazukuro0824+dev@gmail.com>
@kazuki0824 kazuki0824 force-pushed the fix/docker_sh_trim_blank_arg branch from 99a61b9 to 3d87c7e Compare March 2, 2023 15:16
@HansRobo
Copy link
Member

HansRobo commented Mar 2, 2023

@kazuki0824 Thank you for your contribution!
Could you describe what happens without this PR, and how did you fix it?

@HansRobo
Copy link
Member

HansRobo commented Mar 2, 2023

@kenji-miyake Can you review this?

@kenji-miyake kenji-miyake changed the title fix(docker): fix insufficient arguments in docker/build.sh when `targ… fix(docker): fix insufficient arguments in docker/build.sh when targets not specified Mar 3, 2023
…t specified

Signed-off-by: maleicacid <kazukuro0824+dev@gmail.com>
@kazuki0824
Copy link
Contributor Author

kazuki0824 commented Mar 5, 2023

I found another solution using array expansion: by making targets an array, the contents can properly be considered as multiple values through "${targets[@]}" instead of a single string. See f44fdee

@kazuki0824
Copy link
Contributor Author

kazuki0824 commented Mar 5, 2023

@kazuki0824 Thank you for your contribution! Could you describe what happens without this PR, and how did you fix it?

@HansRobo

Without this PR, docker/build.sh will fail.
If the option --no-prebuilt is not specified, then $targets will be set as follows:

autoware/docker/build.sh

Lines 38 to 44 in 3555383

# Set prebuilt options
if [ "$option_no_prebuilt" = "true" ]; then
targets="devel"
else
# default targets include devel and prebuilt
targets=""
fi

and then an empty string will be added to the bottom of the argv of the bake :

autoware/docker/build.sh

Lines 66 to 76 in 3555383

docker buildx bake --no-cache --load --progress=plain -f "$SCRIPT_DIR/autoware-universe/docker-bake.hcl" \
--set "*.context=$WORKSPACE_ROOT" \
--set "*.ssh=default" \
--set "*.platform=$platform" \
--set "*.args.ROS_DISTRO=$rosdistro" \
--set "*.args.BASE_IMAGE=$base_image" \
--set "*.args.PREBUILT_BASE_IMAGE=$prebuilt_base_image" \
--set "*.args.SETUP_ARGS=$setup_args" \
--set "devel.tags=ghcr.io/autowarefoundation/autoware-universe:$rosdistro-latest$image_name_suffix" \
--set "prebuilt.tags=ghcr.io/autowarefoundation/autoware-universe:$rosdistro-latest-prebuilt$image_name_suffix" \
"$targets"

@kazuki0824 kazuki0824 changed the title fix(docker): fix insufficient arguments in docker/build.sh when targets not specified fix(docker): avoid empty string in bake's arguments when targets not specified Mar 5, 2023
@kenji-miyake kenji-miyake enabled auto-merge (squash) March 5, 2023 09:18
@kenji-miyake kenji-miyake merged commit 33e39f6 into autowarefoundation:main Mar 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants