-
Notifications
You must be signed in to change notification settings - Fork 242
Fix Docker build on ARM #473
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
Conversation
Signed-off-by: Nicko Guyer <nicko.guyer@kaleido.io>
Dockerfile
Outdated
|
|
||
| FROM golang:1.16-alpine3.13 AS fabric-builder | ||
| RUN apk add libc6-compat | ||
| FROM golang@sha256:bab81aadc644eee23bafdf479e9bd60c418d405dbc79370ababbb2a2f88d8034 AS fabric-builder |
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'm really worried about how deep we're burying dependency information that needs to be regularly updated in the Dockerfiles at the moment.
Specifically:
- UI release version
- This pretty obscure tag that links to a particular (I assume ARM friendly) image of Golang
Can these be rolled up to the Manifest?
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's a very good point. I think it's possible to pull these into the manifest if we are okay with moving beyond just a basic docker build ... command. Perhaps we can add a target to the Makefile for docker so the command just becomes make docker. And that script takes care of passing the versions from the manifest to here or wherever else they need to go.
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.
As an aside, this is just the sha for the x86_64 version of this image (which runs just fine on an M1 Mac). It is the same image that this Dockerfile would have picked if it were running on an x86_64 host.
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.
Think make docker sounds like a good approach.
Other option which would move forwards with this PR as is, is to put in the comments a repeatable instruction to find (without owning an M1 Mac) the reverse lookup to "what Go image is this?" and most importantly the next x86_64 image to apply a Go patch release (common security update requirement).
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.
| FROM golang@sha256:bab81aadc644eee23bafdf479e9bd60c418d405dbc79370ababbb2a2f88d8034 AS fabric-builder | |
| FROM --platform=linux/x86_64 golang:1.16-alpine3.13 AS fabric-builder |
This seemed to work for this step for me @nguyer - I think it's doing the same thing as your change
peterbroadhurst
left a comment
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.
... please see the bigger thought, but most important thing needed for a merge here is an explanation in the code somewhere of how this Docker image was chosen and when it needs to be updated going forwards.
|
I note after my PR, I'm still seeing a failure from IPFS: |
|
Workaround listed here: ipfs/kubo#8645 (comment) |
|
Yep I've run into the same thing and found that same thread. This is a firefly-cli issue. Shouldn't be too bad to implement but it's more than one line, since we're currently not writing a custom config file for IPFS. I'm going to look into this more tomorrow though. |
Signed-off-by: Nicko Guyer <nicko.guyer@kaleido.io>
|
I pulled in changes from #484 as well |
Signed-off-by: Nicko Guyer <nicko.guyer@kaleido.io>
Codecov Report
@@ Coverage Diff @@
## main #473 +/- ##
===========================================
+ Coverage 99.92% 100.00% +0.07%
===========================================
Files 266 267 +1
Lines 15134 15236 +102
===========================================
+ Hits 15123 15236 +113
+ Misses 11 0 -11
Continue to review full report at Codecov.
|
Signed-off-by: Nicko Guyer <nicko.guyer@kaleido.io>
|
Think it might be worth the script checking if |
Signed-off-by: Nicko Guyer <nicko.guyer@kaleido.io>
Signed-off-by: Nicko Guyer <nicko.guyer@kaleido.io>
peterbroadhurst
left a comment
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.
Looks great!
Resolves #469