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

[COIN-795] Enhance compatibility for Apple Silicon #2

Merged
merged 23 commits into from
Sep 4, 2023
Merged

[COIN-795] Enhance compatibility for Apple Silicon #2

merged 23 commits into from
Sep 4, 2023

Conversation

hoonahn
Copy link

@hoonahn hoonahn commented Aug 25, 2023 •
edited
Loading

Changes

  • _arm64 postfix를 붙인 toolchain을 추가하여 Apple Silicon(M1) 호환성 개선
  • kubectl 버전이 1.16으로 너무 낮아서 1.25.13으로 업그레이드
  • yq 버전을 4.35.1로 업그레이드

@hoonahn
Copy link
Author

hoonahn commented Aug 28, 2023

q; _arm64 붙인 것처럼 _amd64를 붙이는게 덜 헷갈릴까요? @moonk-banksalad

moonk-banksalad
moonk-banksalad requested changes Aug 28, 2023
View reviewed changes
toolchains/helm-3/BUILD Outdated
name = "helm_v3.12.2_linux_arm64_toolchain",
exec_compatible_with = [
"@platforms//os:linux",
"@platforms//cpu:x86_64",
Copy link

Choose a reason for hiding this comment

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

이게 cpu:arm64가 되어야 할 듯?

Copy link

Choose a reason for hiding this comment

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

참고로 Bazel에서 사용할 수 있는 @platforms 값들은 여기에 정의되어 있습니다.
https://github.com/bazelbuild/platforms/blob/main/cpu/BUILD

Copy link
Author

Choose a reason for hiding this comment

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

089ef2e

toolchains/helm-3/BUILD Outdated Show resolved Hide resolved
toolchains/helm-3/BUILD Show resolved Hide resolved
toolchains/helm-3/BUILD Show resolved Hide resolved
toolchains/kubectl/BUILD Outdated Show resolved Hide resolved
toolchains/kubectl/BUILD Show resolved Hide resolved
toolchains/kubectl/BUILD Show resolved Hide resolved
@moonk-banksalad
Copy link

q; _arm64 붙인 것처럼 _amd64를 붙이는게 덜 헷갈릴까요? @moonk-banksalad

굳이 붙일 필요는 없을 것 같습니다. 코드 수정도 많이 해야 하고...

@hoonahn hoonahn changed the title [COIN-795] Enhance compatibility for Apple Silicon and upgrade kubectl to 1.25.13 [COIN-795] Enhance compatibility for Apple Silicon Aug 31, 2023
@hoonahn
Copy link
Author

hoonahn commented Aug 31, 2023

image

드디어 M1에서 bazel build를 성공했습니다 ㅎ..

@moonk-banksalad
Copy link

moonk-banksalad commented Aug 31, 2023 •
edited
Loading

Integration tests 만 고치면 될 것 같습니다! 에러를 보니 새로운 yq 바이너리가 깨진듯?
line 43: external/yq_v4.35.1_linux/file/downloaded: cannot execute binary file: Exec format error

@hoonahn
Copy link
Author

hoonahn commented Aug 31, 2023

Integration tests 만 고치면 될 것 같습니다! 에러를 보니 새로운 yq 바이너리가 깨진듯? line 43: external/yq_v4.35.1_linux/file/downloaded: cannot execute binary file: Exec format error

넵 exec 가능한 것으로 바꿨습니다 ㅎㅎ

@hoonahn
Copy link
Author

hoonahn commented Sep 1, 2023

PR상에 변경사항과 어떤 관계가 있는지 모를 에러가 계속 발생하네요 😵
짐작이 가는 부분이 있으실까요? cc @moonk-banksalad

Error:         	            	ERROR: /home/runner/work/helm-bazel-rules/helm-bazel-rules/tests/charts/nginx/BUILD:7:11: Action tests/charts/nginx/nginx.tgz failed: (Exit 1): nginx_chart_helm_bash failed: error executing command bazel-out/k8-fastbuild/bin/tests/charts/nginx/nginx_chart_helm_bash
Error:         	            	Error: 1:1: invalid input text "w"

@hoonahn
Copy link
Author

hoonahn commented Sep 1, 2023 •
edited
Loading

[이슈 공유]

기존 upstream에서 사용하던 yq의 버전이 v2이었는데, 이 경우 apple silicon과 호환되는 패키지가 없었어서 최신 v4 버전으로 올렸는데 기존 yq에서 w라는 커맨드를 사용하던게 v4에서는 변경이 되었었던 것이었고 그래서 에러가 발생했습니다.

v3까지는 유지되었던 것으로 보여서 v3로 내리려고 하니 v3에서도 apple silicon과 호환되는 패키지가 없어서... apple silicon에서 사용하기 위해서는 yq를 v4로 올려야했고, 패키지 내에서 yq를 사용하는 곳 중 deprecated 커맨드를 사용하는 곳들을 migration 가이드에 따라 변경해보고 있습니다. cc. @moonk-banksalad, (도움 주신) @namkwangwoo

moonk-banksalad
moonk-banksalad requested changes Sep 4, 2023
View reviewed changes
helm/helm-chart-package.sh.tpl Outdated
@@ -68,12 +68,12 @@ if [ -n $DIGEST_PATH ] && [ "$DIGEST_PATH" != "" ]; then
REPO_URL="{IMAGE_REPOSITORY}"
else
# if image_repository attr is not provided, extract it from values.yaml
REPO_URL=$({YQ_PATH} r {CHART_VALUES_PATH} {VALUES_REPO_YAML_PATH})
REPO_URL=$({YQ_PATH} "{VALUES_REPO_YAML_PATH}" {CHART_VALUES_PATH})
Copy link

Choose a reason for hiding this comment

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

yq r 명령어에 대한 변경시에도 selector는 "."을 붙여주여야 합니다.
https://mikefarah.gitbook.io/yq/upgrading-from-v3

Copy link

Choose a reason for hiding this comment

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

그리고 굳이 ""를 사용할 필요가 있나요?

Copy link
Author

Choose a reason for hiding this comment

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

수정했습니다!

helm/helm-chart-package.sh.tpl Outdated
fi

# appends @sha256 suffix to image repo url value if the repository value does not already contains it
if ([ -n $REPO_URL ] || [ -n $REPO_SUFIX ]) && ([[ $REPO_URL != *"$REPO_SUFIX" ]] || [[ -z "$REPO_SUFIX" ]]); then
{YQ_PATH} w -i {CHART_VALUES_PATH} {VALUES_REPO_YAML_PATH} ${REPO_URL}${REPO_SUFIX}
{YQ_PATH} -i '.{VALUES_REPO_YAML_PATH} = $REPO_URL$REPO_SUFIX' {CHART_VALUES_PATH}
Copy link

Choose a reason for hiding this comment

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

굳이 {}를 제거할 필요가 있나요? ${REPO_URL} -> $REPO_URL

Copy link
Author

Choose a reason for hiding this comment

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

수정했습니다!

@hoonahn
Copy link
Author

hoonahn commented Sep 4, 2023 •
edited
Loading

yq v4에서 env를 가져다 쓰는 것도 다른 방식으로 변동된 것 같습니다 ㅜ
그래서 env 값을 못가져와서 계속 이상하게 ci가 깨졌던 것도 있네요. 이제 integration test에 문제 없습니다.

https://mikefarah.gitbook.io/yq/operators/env-variable-operators

cc. @moonk-banksalad @namkwangwoo

namkwangwoo
namkwangwoo approved these changes Sep 4, 2023
View reviewed changes
Copy link

@namkwangwoo namkwangwoo left a comment

Choose a reason for hiding this comment

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

lgtm

@hoonahn hoonahn merged commit 9a828a1 into master Sep 4, 2023
1 check passed
@hoonahn hoonahn deleted the coin-795 branch September 4, 2023 08:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@moonk-banksalad moonk-banksalad moonk-banksalad approved these changes

@namkwangwoo namkwangwoo namkwangwoo approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hoonahn @moonk-banksalad @namkwangwoo