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
Change build path for linux binaries to match GOARCH. #890
Change build path for linux binaries to match GOARCH. #890
Conversation
Codecov Report
@@ Coverage Diff @@
## main #890 +/- ##
=======================================
Coverage 53.69% 53.69%
=======================================
Files 12 12
Lines 419 419
=======================================
Hits 225 225
Misses 177 177
Partials 17 17 Continue to review full report at Codecov.
|
fced26f
to
a094f16
Compare
@@ -78,22 +78,22 @@ all-srcs: | |||
.PHONY: build | |||
build: install-tools lint multimod-verify | |||
GOOS=darwin GOARCH=amd64 $(GOBUILD) $(LDFLAGS) -o ./build/darwin/aoc_darwin_amd64 ./cmd/awscollector | |||
GOOS=linux GOARCH=amd64 $(GOBUILD) $(LDFLAGS) -o ./build/linux/aoc_linux_x86_64 ./cmd/awscollector |
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.
Have we talked with @alolita @anuraaga @Aneurysm9 that being backwards not compatible is okay here
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.
Is this just an intermediate name used during the build process? Does the name of the executable included in the deb
, rpm
, or container image change?
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.
Yes, this is just an intermediate name. create_rpm.sh
and create_deb.sh
both copy the binary in and rename it to aws-otel-collector
. The Dockerfile also renames it, but to awscollector
.
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.
Yeah intermediate names should not need to be preserved.
While we're changing these, can we dedupe, and maybe simplify, by moving into the folder
./build/linux-amd64/aoc
?
@@ -9,16 +9,13 @@ RUN apk --update add ca-certificates | |||
|
|||
WORKDIR /workspace | |||
|
|||
ARG TARGETPLATFORM | |||
RUN echo "Target platform is $TARGETPLATFORM" |
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 think we should echo the target arch we are running for
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.
Wouldn't the TARGETARCH just show up in the COPY instruction when it tries to find the file?
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.
Is it possible to separate out the lint/formatting changes? They make it hard to have confidence that the functional changes have all been identified.
@@ -78,22 +78,22 @@ all-srcs: | |||
.PHONY: build | |||
build: install-tools lint multimod-verify | |||
GOOS=darwin GOARCH=amd64 $(GOBUILD) $(LDFLAGS) -o ./build/darwin/aoc_darwin_amd64 ./cmd/awscollector | |||
GOOS=linux GOARCH=amd64 $(GOBUILD) $(LDFLAGS) -o ./build/linux/aoc_linux_x86_64 ./cmd/awscollector |
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.
Is this just an intermediate name used during the build process? Does the name of the executable included in the deb
, rpm
, or container image change?
a094f16
to
989a774
Compare
@Aneurysm9 I've removed the linting changes. |
989a774
to
a29388c
Compare
a29388c
to
a8bd382
Compare
Description: Simplifies the docker build process by changing the linux binary build paths to match GOARCH/TARGETARCH.
Link to tracking Issue: #880
Testing: Ran
create_rpm.sh
andcreate_deb.sh
(partially). Ranmake docker-build
andmake docker-build-arm
.