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 docker cross-platform builds #10827

Merged
merged 32 commits into from
Jul 19, 2024
Merged

Conversation

bitwiseguy
Copy link
Contributor

@bitwiseguy bitwiseguy commented Jun 14, 2024

The docker images being pushed to docker registry by our CI do not work on arm64 machines. This PR attempts to fix that.

Fixes #11081

Key change to fix cross compiling issues

Removed TARGETOS and TARGETARCH declarations from the top of the dockerfile. These args both get set automatically by the docker build kit, and the global declarations in the dockerfile caused an issue where the values were not set or passed to the underlying make commands which invoke go build.

Additional context

Also added CGO_ENABLED=0 to docker images that are being built for cross-platform. This will cause the build to fail if C code is included in the go source code. This seems better than allowing the build to pass and only catching the failure further downstream when the image is pulled and attempted to run on an arm64 machine. If we want the cross-platform builds to work with C code included, we will have to spend some more effort adding appropriate CC and CXX packages to our docker builder, then allow CGO_ENABLED=1. I'm open to removing this part if reviewers feel this is a bad idea

Tests

Added check-cross-platform ci jobs for every image published during scheduled-docker-publish that is built for multiple platforms (i.e. platforms: "linux/amd64,linux/arm64" for the docker-build job)

@fanbsb
Copy link

fanbsb commented Jun 20, 2024

Please give priority to this PR, we need the arm64 image before the hardfork on op-node 1.7.7

@fanbsb
Copy link

fanbsb commented Jul 1, 2024

This PR works on M1 Mac, but not on AWS Graviton instance.

Error Message
/ # op-node --help
/usr/local/bin/op-node: line 2: syntax error: unexpected "(

Copy link
Contributor

semgrep-app bot commented Jul 7, 2024

Semgrep found 1 sol-style-malformed-revert finding:

  • packages/contracts-bedrock/src/cannon/MIPS.sol

Malformed revert statement style.

Ignore this finding from sol-style-malformed-revert.

Semgrep found 28 golang_fmt_errorf_no_params findings:

No fmt.Errorf invocations without fmt arguments allowed

Ignore this finding from golang_fmt_errorf_no_params.

Semgrep found 1 nil-check-after-call finding:

Potential p2pmetrics nil dereference when NewBandwidthCounter is called

Ignore this finding from nil-check-after-call.

Semgrep found 1 math-random-used finding:

Do not use math/rand. Use crypto/rand instead.

Ignore this finding from math-random-used.

Semgrep found 2 todos_require_linear findings:

Please create a GitHub ticket for this TODO.

Ignore this finding from todos_require_linear.

Semgrep found 1 os-error-is-not-exist finding:

  • op-chain-ops/cmd/registry-data/main.go

New code should use errors.Is with the appropriate error type

Ignore this finding from os-error-is-not-exist.

Semgrep found 2 marshal-json-pointer-receiver findings:

MarshalJSON with a pointer receiver has surprising results: golang/go#22967

Ignore this finding from marshal-json-pointer-receiver.

Copy link

codecov bot commented Jul 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.69%. Comparing base (3eb9861) to head (a231f98).
Report is 17 commits behind head on develop.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop   #10827   +/-   ##
========================================
  Coverage    60.69%   60.69%           
========================================
  Files           20       20           
  Lines         1781     1781           
  Branches        71       71           
========================================
  Hits          1081     1081           
  Misses         667      667           
  Partials        33       33           
Flag Coverage Δ
cannon-go-tests 79.69% <ø> (ø)
chain-mon-tests 27.14% <ø> (ø)
sdk-tests 16.44% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@bitwiseguy bitwiseguy force-pushed the ss/fix-docker-cross-platform branch from 798b85b to 7362544 Compare July 8, 2024 02:52
op-node/Makefile Outdated Show resolved Hide resolved
.circleci/config.yml Outdated Show resolved Hide resolved
@bitwiseguy bitwiseguy marked this pull request as ready for review July 9, 2024 20:14
@bitwiseguy bitwiseguy requested review from a team, 0x00101010, zhwrd and mslipper as code owners July 9, 2024 20:14
.circleci/config.yml Outdated Show resolved Hide resolved
.circleci/config.yml Show resolved Hide resolved
op-batcher/Makefile Outdated Show resolved Hide resolved
op-batcher/Makefile Outdated Show resolved Hide resolved
ops/docker/op-stack-go/Dockerfile Show resolved Hide resolved
op-program/Makefile Outdated Show resolved Hide resolved
Copy link
Contributor

@protolambda protolambda left a comment

Choose a reason for hiding this comment

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

Regarding op-program: the host part is not subject to the same reproduction and platform target restrictions as the client part

@protolambda protolambda added this pull request to the merge queue Jul 19, 2024
Merged via the queue into develop with commit 26eac67 Jul 19, 2024
63 checks passed
@protolambda protolambda deleted the ss/fix-docker-cross-platform branch July 19, 2024 19:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

arm64 docker images contain x86 executables
5 participants