-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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 build docker images for arm64 arch #1906
Conversation
I have no permission to test push related things, build pass at local. |
Prometheus also support arm, but i saw corss build only generate binary for amd64 and arm64, so do not add it. |
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.
Thanks a lot!
But I think we can simplify here a lot. Let's instead use the way Prometheus is doing this, so https://github.com/prometheus/prometheus/blob/337a1fbffd17ad3c7952dadf1246ee6eedf57b01/Makefile.common#L213
And dockerfile taking from promu (we already use promu for the build process):
https://github.com/prometheus/prometheus/blob/bca6e90ea6788100a56809e73f9eeaaa11480036/Dockerfile#L6
What do you think? (:
@bwplotka I use the way Prometheus does, please review again. |
A few things from my Prometheus experience:
And there might other things that I don't remember ;-) |
@simonpasquier yep, but i have no premissions to test while process:( |
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 like it, but indeed only the maintainer can test it and I will do this in free time.
Thanks for nice tips @simonpasquier !
Also make build
with promu build just ./thanos
so make build
and building docker will not work anymore... This was a nice manual flow that some dev uses. I think this is fine though, we adjust it in local dev script.
@bwplotka wait for your feedback~ |
@bwplotka do you have time to test it? |
This looks good to me. The only real way to be sure it works would be to merge and see what happens :-) |
This issue/PR has been automatically marked as stale because it has not had recent activity. Please comment on status otherwise the issue will be closed in a week. Thank you for your contributions. |
@bwplotka hi, do you have time to test it? |
Yea not really. Will speak offline how we can move this forward (:
…On Sat, 21 Mar 2020 at 09:59, Xiang Dai ***@***.***> wrote:
@bwplotka <https://github.com/bwplotka> hi, do you have time to test it?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1906 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABVA3O5LBKJGYGVO4YLRDNTRISFZNANCNFSM4J4HBSTQ>
.
|
Hey, I'm interested in this and I think I will be able to help, ping me on monday :) |
@povilasv thanks! |
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.
So right github actions for e2e are failing and are not flaky, we need to fix this.
The issue is believe that build step produces binaries not in .build/${OS}-${ARCH}/thanos
:
build: check-git deps $(PROMU)
@echo ">> building binaries $(GOBIN)"
@$(PROMU) build --prefix $(PREFIX)
I suggest trying to change that to:
build: check-git deps $(PROMU)
@echo ">> building binaries $(GOBIN)"
@$(PROMU) crossbuild --prefix $(PREFIX) -p 'linux/amd64'
I think this should put the binary into correct dir.
The CI test result show that we can not use
Just change "linux" to "$OS" is enough. |
0ff6d16
to
b68a5fb
Compare
@GiedriusS @hanjm Please review again. |
Signed-off-by: Xiang Dai <[email protected]>
Signed-off-by: Long Dai <[email protected]>
Signed-off-by: Giedrius Statkevičius <[email protected]>
Signed-off-by: Giedrius Statkevičius <[email protected]>
Only create a manifest & push it after pushing images with the new names. Signed-off-by: Giedrius Statkevičius <[email protected]>
Signed-off-by: Loong <[email protected]>
Signed-off-by: Loong <[email protected]>
Signed-off-by: Loong <[email protected]>
Signed-off-by: Giedrius Statkevičius <[email protected]>
Signed-off-by: Giedrius Statkevičius <[email protected]>
Signed-off-by: Giedrius Statkevičius <[email protected]>
Signed-off-by: Loong <[email protected]>
Signed-off-by: Giedrius Statkevičius <[email protected]>
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.
💪 hope this works
Had to use a different emulator to make it work: 9faf940. Now |
Signed-off-by: Xiang Dai [email protected]
Changes
Fix #1851