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.

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

fix: push docker image action. #580

Merged
merged 4 commits into from
Jul 7, 2022

Conversation

zemyblue
Copy link
Member

@zemyblue zemyblue commented Jul 5, 2022

Signed-off-by: zemyblue [email protected]

Description

The registered docker image of docker hub is not executed in Apple M1 Macbook. Because the docker image was not built with the platforms information, so I fix this problem in this PR.

And libstdc++ library is required for linux/amd64 because bls-eth-go-binary of ostracon. So I add it in the Dockerfile.

Motivation and context

How has this been tested?

Screenshots (if appropriate):

Checklist:

  • I followed the contributing guidelines and code of conduct.
  • I have added a relevant changelog to CHANGELOG.md
  • I have added tests to cover my changes.
  • I have updated the documentation accordingly.
  • I have updated API documentation client/docs/swagger-ui/swagger.yaml

@zemyblue zemyblue requested review from 0Tech, da1suk8, dudong2, tnasu and ulbqb July 5, 2022 09:53
@zemyblue zemyblue self-assigned this Jul 5, 2022
@zemyblue zemyblue added A: CI A: bug Something isn't working labels Jul 5, 2022
@codecov
Copy link

codecov bot commented Jul 5, 2022

Codecov Report

Merging #580 (c9ca556) into main (3743fd6) will decrease coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #580      +/-   ##
==========================================
- Coverage   57.68%   57.68%   -0.01%     
==========================================
  Files         793      793              
  Lines       87013    87013              
==========================================
- Hits        50193    50190       -3     
- Misses      33662    33664       +2     
- Partials     3158     3159       +1     
Impacted Files Coverage Δ
x/wasm/keeper/keeper.go 84.85% <0.00%> (-0.37%) ⬇️

@@ -30,7 +30,7 @@ RUN make build-linux
FROM alpine:edge

# Install ca-certificates
RUN apk add --update ca-certificates
RUN apk add --update ca-certificates libstdc++
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this change specific to M1? Then, it would be nice to state it in the comment or description of the PR.

Copy link
Member

Choose a reason for hiding this comment

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

I know it is for using BLS of line/ostracon.

Copy link
Member

Choose a reason for hiding this comment

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

I tried it on M1 macbook docker linux/arm64 and linux/amd64 environment, but the latest github.com/herumi/bls-eth-go-binary seems to require libstdc++.

Copy link
Member Author

@zemyblue zemyblue Jul 6, 2022

Choose a reason for hiding this comment

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

No, libstdc++ library is required for linux/amd64 because bls-eth-go-binary of ostracon.
I add the description about this chagens.

go.mod Outdated
@@ -62,5 +62,6 @@ require (
replace (
github.com/99designs/keyring => github.com/cosmos/keyring v1.1.7-0.20210622111912-ef00f8ac3d76
github.com/gogo/protobuf => github.com/regen-network/protobuf v1.3.3-alpha.regen.1
github.com/herumi/bls-eth-go-binary => github.com/herumi/bls-eth-go-binary v0.0.0-20220509081320-2d8ab06de53c
Copy link
Member

@tnasu tnasu Jul 6, 2022

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

How about using line/ostracon-v1.0.6 instead of using replace?

I think so, too.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you. I'll change line/ostracon version from v1.0.6-0.20220614053335-f9d9fa2cc779 to v1.0.6. However this change would not have been necessary in the original. :)

Copy link
Member

@ulbqb ulbqb left a comment

Choose a reason for hiding this comment

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

I leave comments.

Copy link
Member

@tnasu tnasu left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@0Tech 0Tech left a comment

Choose a reason for hiding this comment

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

LGTM

@zemyblue zemyblue merged commit 2e12c78 into Finschia:main Jul 7, 2022
@zemyblue zemyblue deleted the fix_docker_image_setting branch August 2, 2022 11:45
@zemyblue zemyblue mentioned this pull request Oct 27, 2022
5 tasks
@zemyblue zemyblue mentioned this pull request Nov 28, 2022
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: bug Something isn't working A: CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants