-
Notifications
You must be signed in to change notification settings - Fork 459
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
Add local arm build #1157
Add local arm build #1157
Conversation
Signed-off-by: Efrat19 <efrat890089@gmail.com>
|
Thank you for this contribution! I think it would be great if we could have the makefile choose the build method automatically as you said in your description. If that proves to be too difficult, we can leave in this temporary solution. |
Makefile
Outdated
@@ -178,6 +178,11 @@ set-test-image-vars: | |||
container: | |||
docker build -t ${IMG} --build-arg VERSION_PKG=${VERSION_PKG} --build-arg VERSION=${VERSION} --build-arg VERSION_DATE=${VERSION_DATE} --build-arg OTELCOL_VERSION=${OTELCOL_VERSION} --build-arg TARGETALLOCATOR_VERSION=${TARGETALLOCATOR_VERSION} --build-arg AUTO_INSTRUMENTATION_JAVA_VERSION=${AUTO_INSTRUMENTATION_JAVA_VERSION} --build-arg AUTO_INSTRUMENTATION_NODEJS_VERSION=${AUTO_INSTRUMENTATION_NODEJS_VERSION} --build-arg AUTO_INSTRUMENTATION_PYTHON_VERSION=${AUTO_INSTRUMENTATION_PYTHON_VERSION} --build-arg AUTO_INSTRUMENTATION_DOTNET_VERSION=${AUTO_INSTRUMENTATION_DOTNET_VERSION} . | |||
|
|||
# Build the container ARM image, used only for local dev purposes | |||
.PHONY: container-arm-build | |||
container-arm-build: |
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.
it says arm but the platform is linux/amd64
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.
fixed
Makefile
Outdated
# Build the container ARM image, used only for local dev purposes | ||
.PHONY: container-arm-build | ||
container-arm-build: | ||
docker buildx build --platform linux/amd64 -t ${IMG} --build-arg VERSION_PKG=${VERSION_PKG} --build-arg VERSION=${VERSION} --build-arg VERSION_DATE=${VERSION_DATE} --build-arg OTELCOL_VERSION=${OTELCOL_VERSION} --build-arg TARGETALLOCATOR_VERSION=${TARGETALLOCATOR_VERSION} --build-arg AUTO_INSTRUMENTATION_JAVA_VERSION=${AUTO_INSTRUMENTATION_JAVA_VERSION} --build-arg AUTO_INSTRUMENTATION_NODEJS_VERSION=${AUTO_INSTRUMENTATION_NODEJS_VERSION} --build-arg AUTO_INSTRUMENTATION_PYTHON_VERSION=${AUTO_INSTRUMENTATION_PYTHON_VERSION} --build-arg AUTO_INSTRUMENTATION_DOTNET_VERSION=${AUTO_INSTRUMENTATION_DOTNET_VERSION} . |
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.
I have concerns about maintaining this. The command is very similar to the container
target above and I fear that over time this gets outdated. I would like to find an approach to minimize duplication.
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.
I extracted the flags to DOCKER_BUILD_FLAGS
and eliminated the extra target
@Efrat19 would the following work for you?
I am fine using only buildx. However the command I shared fails on my host when building other archs e.g.
|
@pavolloffay Yep it also works for me on amd64 , I changed it to use buildx only |
Awesome, please try if the build works. Could you please also change |
Sure @pavolloffay I updated container-target-allocator as well |
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.
e2e test failed
Makefile
Outdated
.PHONY: container | ||
container: | ||
docker build -t ${IMG} --build-arg VERSION_PKG=${VERSION_PKG} --build-arg VERSION=${VERSION} --build-arg VERSION_DATE=${VERSION_DATE} --build-arg OTELCOL_VERSION=${OTELCOL_VERSION} --build-arg TARGETALLOCATOR_VERSION=${TARGETALLOCATOR_VERSION} --build-arg AUTO_INSTRUMENTATION_JAVA_VERSION=${AUTO_INSTRUMENTATION_JAVA_VERSION} --build-arg AUTO_INSTRUMENTATION_NODEJS_VERSION=${AUTO_INSTRUMENTATION_NODEJS_VERSION} --build-arg AUTO_INSTRUMENTATION_PYTHON_VERSION=${AUTO_INSTRUMENTATION_PYTHON_VERSION} --build-arg AUTO_INSTRUMENTATION_DOTNET_VERSION=${AUTO_INSTRUMENTATION_DOTNET_VERSION} . | ||
docker buildx build --platform linux/$(go env GOARCH) -t ${IMG} --build-arg VERSION_PKG=${VERSION_PKG} --build-arg VERSION=${VERSION} --build-arg VERSION_DATE=${VERSION_DATE} --build-arg OTELCOL_VERSION=${OTELCOL_VERSION} --build-arg TARGETALLOCATOR_VERSION=${TARGETALLOCATOR_VERSION} --build-arg AUTO_INSTRUMENTATION_JAVA_VERSION=${AUTO_INSTRUMENTATION_JAVA_VERSION} --build-arg AUTO_INSTRUMENTATION_NODEJS_VERSION=${AUTO_INSTRUMENTATION_NODEJS_VERSION} --build-arg AUTO_INSTRUMENTATION_PYTHON_VERSION=${AUTO_INSTRUMENTATION_PYTHON_VERSION} --build-arg AUTO_INSTRUMENTATION_DOTNET_VERSION=${AUTO_INSTRUMENTATION_DOTNET_VERSION} . |
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.
I would prefer to have arch defined as env var in the makefile, It makes it easier to override when running the target.
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.
e2e test failed
docker buildx build --platform linux/ -t local/opentelemetry-operator:e2e --build-arg VERSION_PKG="github.com/open-telemetry/opentelemetry-operator/internal/version" --build-arg VERSION="" --build-arg VERSION_DATE=2022-10-14T08:54:15Z --build-arg OTELCOL_VERSION="0.60.0" --build-arg TARGETALLOCATOR_VERSION="0.60.0" --build-arg AUTO_INSTRUMENTATION_JAVA_VERSION="1.11.1" --build-arg AUTO_INSTRUMENTATION_NODEJS_VERSION="0.31.0" --build-arg AUTO_INSTRUMENTATION_PYTHON_VERSION="0.34b0" --build-arg AUTO_INSTRUMENTATION_DOTNET_VERSION="0.3.1-beta.1" .
ERROR: "" is an invalid component of "linux/": platform specifier component must match "^[A-Za-z0-9_-]+$": invalid argument
the arch is not set
Signed-off-by: Efrat19 <efrat890089@gmail.com>
I extracted |
Makefile
Outdated
@@ -10,6 +10,8 @@ AUTO_INSTRUMENTATION_NODEJS_VERSION ?= "$(shell grep -v '\#' versions.txt | grep | |||
AUTO_INSTRUMENTATION_PYTHON_VERSION ?= "$(shell grep -v '\#' versions.txt | grep autoinstrumentation-python | awk -F= '{print $$2}')" | |||
AUTO_INSTRUMENTATION_DOTNET_VERSION ?= "$(shell grep -v '\#' versions.txt | grep autoinstrumentation-dotnet | awk -F= '{print $$2}')" | |||
LD_FLAGS ?= "-X ${VERSION_PKG}.version=${VERSION} -X ${VERSION_PKG}.buildDate=${VERSION_DATE} -X ${VERSION_PKG}.otelCol=${OTELCOL_VERSION} -X ${VERSION_PKG}.targetAllocator=${TARGETALLOCATOR_VERSION} -X ${VERSION_PKG}.autoInstrumentationJava=${AUTO_INSTRUMENTATION_JAVA_VERSION} -X ${VERSION_PKG}.autoInstrumentationNodeJS=${AUTO_INSTRUMENTATION_NODEJS_VERSION} -X ${VERSION_PKG}.autoInstrumentationPython=${AUTO_INSTRUMENTATION_PYTHON_VERSION} -X ${VERSION_PKG}.autoInstrumentationDotNet=${AUTO_INSTRUMENTATION_DOTNET_VERSION}" | |||
ARCH = $(shell go env GOARCH) | |||
ARCH ?= amd64 |
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.
it should be ARCH ?= $(shell go env GOARCH)
* add local arm build Signed-off-by: Efrat19 <efrat890089@gmail.com> * fix typo Signed-off-by: Efrat19 <efrat890089@gmail.com> * add automatic amd build if needed * fix amd to arm * use buildx only * support arm builds in container-target-allocator * add default ARCH for e2e Signed-off-by: Efrat19 <efrat890089@gmail.com> * fix ARCH var Signed-off-by: Efrat19 <efrat890089@gmail.com> Co-authored-by: Pavol Loffay <p.loffay@gmail.com>
Signed-off-by: Efrat19 efrat890089@gmail.com
Addressing #1133
Do let me know whether further changes are required
(maybe we want the makefile to choose the build method automatically based on the underlying platform?)
@jaronoff97 @pavolloffay