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

Dockerfile: arm64 support #4858

Merged
merged 14 commits into from
Mar 31, 2022
Merged

Dockerfile: arm64 support #4858

merged 14 commits into from
Mar 31, 2022

Conversation

thepwagner
Copy link
Contributor

@thepwagner thepwagner commented Mar 15, 2022

This PR amends the Dockerfile and bin/docker-dev-shell commands to support native builds on arm64, as provided by an M1 Mac.

  • Initial commits walk the Dockerfile and provide arm64-native alternatives when binaries are fetched.
    • There is no Elm, sorry.
    • There are no Terraform Debian packages, so I switched to fetching zips.
    • In places where the digest of downloaded files is verified, you have two digests to maintain now.
  • No attempt is made to build arm64 in CI, or publish arm64 releases.

The resulting build stands up OK to light testing: I could dry-run a golang, node and gradle repository.

I hit an error in the bundler that I didn't try to debug:

	 1: from /home/dependabot/dependabot-core/bundler/lib/dependabot/bundler/update_checker/shared_bundler_helpers.rb:54:in `block in in_a_native_bundler_context'
/home/dependabot/dependabot-core/bundler/lib/dependabot/bundler/update_checker/conflicting_dependency_resolver.rb:44:in `block in conflicting_dependencies': undefined method `name' for nil:NilClass (NoMethodError)

Per https://www.terraform.io/cli/install/apt , there is no support for
`arm64`. Switching to downloading a .zip for both architectures is
preferable to having multiple installation paths.
Flutter includes an amd64 dart-sdk, and doesn't provide an arm64
package.

I naively swapped the system dart-sdk into flutter: if it works it will
also save some space :crossed-fingers:.
This avoids dealing with the exception:
```
Traceback (most recent call last):
	3: from bin/dry-run.rb:76:in `<main>'
	2: from bin/dry-run.rb:76:in `require'
	1: from /home/dependabot/dependabot-core/omnibus/.bundle/ruby/2.7.0/gems/stackprof-0.2.19/lib/stackprof.rb:1:in `<top (required)>'
/home/dependabot/dependabot-core/omnibus/.bundle/ruby/2.7.0/gems/stackprof-0.2.19/lib/stackprof.rb:1:in `require': /home/dependabot/dependabot-core/omnibus/.bundle/ruby/2.7.0/gems/stackprof-0.2.19/lib/stackprof/stackprof.so: undefined symbol: pthread_atfork - /home/dependabot/dependabot-core/omnibus/.bundle/ruby/2.7.0/gems/stackprof-0.2.19/lib/stackprof/stackprof.so (LoadError)
```
This is an easy fix with big implications.

It's intending to workaround this error:

```
Traceback (most recent call last):
	11: from bin/dry-run.rb:96:in `<main>'
	10: from bin/dry-run.rb:96:in `require'
	 9: from /home/dependabot/dependabot-core/go_modules/lib/dependabot/go_modules.rb:6:in `<top (required)>'
	 8: from /home/dependabot/dependabot-core/go_modules/lib/dependabot/go_modules.rb:6:in `require'
	 7: from /home/dependabot/dependabot-core/go_modules/lib/dependabot/go_modules/file_parser.rb:6:in `<top (required)>'
	 6: from /home/dependabot/dependabot-core/go_modules/lib/dependabot/go_modules/file_parser.rb:6:in `require'
	 5: from /home/dependabot/dependabot-core/go_modules/lib/dependabot/go_modules/path_converter.rb:4:in `<top (required)>'
	 4: from /home/dependabot/dependabot-core/go_modules/lib/dependabot/go_modules/path_converter.rb:4:in `require'
	 3: from /home/dependabot/dependabot-core/omnibus/.bundle/ruby/2.7.0/gems/nokogiri-1.13.3-aarch64-linux/lib/nokogiri.rb:10:in `<top (required)>'
	 2: from /home/dependabot/dependabot-core/omnibus/.bundle/ruby/2.7.0/gems/nokogiri-1.13.3-aarch64-linux/lib/nokogiri.rb:10:in `require_relative'
	 1: from /home/dependabot/dependabot-core/omnibus/.bundle/ruby/2.7.0/gems/nokogiri-1.13.3-aarch64-linux/lib/nokogiri/extension.rb:7:in `<top (required)>'
/home/dependabot/dependabot-core/omnibus/.bundle/ruby/2.7.0/gems/nokogiri-1.13.3-aarch64-linux/lib/nokogiri/extension.rb:7:in `require_relative': /lib/aarch64-linux-gnu/libm.so.6: version `GLIBC_2.29' not found (required by /home/dependabot/dependabot-core/omnibus/.bundle/ruby/2.7.0/gems/nokogiri-1.13.3-aarch64-linux/lib/nokogiri/2.7/nokogiri.so) - /home/dependabot/dependabot-core/omnibus/.bundle/ruby/2.7.0/gems/nokogiri-1.13.3-aarch64-linux/lib/nokogiri/2.7/nokogiri.so (LoadError)
```
@thepwagner thepwagner mentioned this pull request Mar 15, 2022
@jeffwidman
Copy link
Member

I opened a PR a while back for the focal upgrade: #4394

Although at this point jammy gets released next month, so if it's hard for the core team to pull in due to scheduling, they may want to jump straight to that--better to test one release well than to test two releases less rigorously.

@jurre
Copy link
Member

jurre commented Mar 16, 2022

Thanks @thepwagner! Bumping ubuntu seems reasonable to me, although I'd like to roll that out separately, which I might as well do now, and so will.

@jeffwidman yeah it's not a bad call, although I kind of don't want to wait even longer, and we should just prioritize rolling that out when it's released. We have a little bit more slack on the team now so we should be able to prioritize this type of work quicker

@pavera pavera marked this pull request as ready for review March 25, 2022 18:17
@pavera pavera requested a review from a team as a code owner March 25, 2022 18:17
Dockerfile Outdated
@@ -250,6 +260,8 @@ RUN curl --connect-timeout 15 --retry 5 "https://storage.googleapis.com/flutter_
&& tar xf "/tmp/flutter.xz" -C /opt/dart \
&& rm "/tmp/flutter.xz" \
&& chmod -R o+rx "/opt/dart/flutter" \
&& rm -Rf /opt/dart/flutter/bin/cache/dart-sdk \
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious about these changes since they seem unrelated to arm support? I think this ends up getting deleted anyway below in the find ... -delete command?

Copy link
Contributor

@pavera pavera Mar 25, 2022

Choose a reason for hiding this comment

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

Not an expert, but it appears flutter ships with a dart-sdk, these 2 lines replace the x86 dart sdk with the arm sdk downloaded in the previous step. I say this because without these 2 lines I get the following error during docker build:
#26 47.08 qemu-x86_64: Could not open '/lib64/ld-linux-x86-64.so.2': No such file or directory

We could maybe try to conditionally do this substitution only for arm64?

Copy link
Contributor Author

@thepwagner thepwagner Mar 25, 2022

Choose a reason for hiding this comment

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

Everyone is correct 🎉 :

The flutter SDK (even after https://github.com/dependabot/dependabot-core/pull/4904/files), embeds an x86_64 dart sdk:

$ file /opt/flutter/new/flutter/bin/cache/dart-sdk/bin/dart 
/opt/flutter/new/flutter/bin/cache/dart-sdk/bin/dart: ELF 64-bit LSB pie executable, x86-64, version 1 (SYSV), dynamically linked, interpreter /lib64/ld-linux-x86-64.so.2, for GNU/Linux 2.6.32, stripped

And the files are deleted by the find, so they aren't part of the final image.

$ docker run --rm dependabot/dependabot-core:latest bash -c "find /opt/dart/flutter/bin/cache/dart-sdk"
find: ‘/opt/dart/flutter/bin/cache/dart-sdk’: No such file or directory

IIUC the issue is L266 where flutter --version is called before the delete happens - that uses the bundled dart-sdk and fails, so I ninja-patched the architecture-appropriate dart-sdk in.

If flutter --version is just meant to "hello world"/verify the installation, removing that step is an alternative to this hack.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, got it, thanks! I don't think we need to make it conditional, I was just curious why it was necessary. This is worth adding a comment about?

cc: @jonassf @sigurdm to see if you have any feedback here.

Copy link
Contributor

Choose a reason for hiding this comment

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

If flutter --version is just meant to "hello world"/verify the installation, removing that step is an alternative to this hack.

I think so! When you clone flutter from github the first invocation will create the version file (that pub relies on) - that is why we did this step, but the packaged flutter contains it already - so I think it can just be removed!

pavera added 2 commits March 29, 2022 15:09
This also allows us to remove the symlink to the arch appropriate dart sdk.
Copy link
Contributor

@mctofu mctofu left a comment

Choose a reason for hiding this comment

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

Thanks!

@jurre
Copy link
Member

jurre commented Mar 30, 2022

Just running it on my old, old non-arm64 laptop to make sure that still works, but assume it will after which I'll merge this in. Thanks @thepwagner!

@jurre
Copy link
Member

jurre commented Mar 30, 2022

Hmm, running into a little snag:

 => ERROR [stage-0 13/38] RUN cd /tmp   && curl --http1.1 -o go-x86_64.tar.gz https://dl.google.com/go/go1.18.linux-x86_64.tar.gz   && printf "e85278e98f57cdb150fe8409e6e5df5343ecb13cebf03a5d5ff12bd55a80264f go-amd64.tar.gz\n7ac7b396a691e588c5fb57  0.6s
------
 > [stage-0 13/38] RUN cd /tmp   && curl --http1.1 -o go-x86_64.tar.gz https://dl.google.com/go/go1.18.linux-x86_64.tar.gz   && printf "e85278e98f57cdb150fe8409e6e5df5343ecb13cebf03a5d5ff12bd55a80264f go-amd64.tar.gz\n7ac7b396a691e588c5fb57687759e6c4db84a2a3bbebb0765f4b38e5b1c5b00e go-arm64.tar.gz\n" | sha256sum -c --ignore-missing -   && tar -xzf go-x86_64.tar.gz -C /opt   && rm go-x86_64.tar.gz:
#19 0.344   % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
#19 0.344                                  Dload  Upload   Total   Spent    Left  Speed
100  1449  100  1449    0     0   7748      0 --:--:-- --:--:-- --:--:--  7748
#19 0.537 sha256sum: 'standard input': no file was verified
------

I can dig into that later

Edit: @pavera noticed this might be a google thing, getting a similar message on arm

@thepwagner
Copy link
Contributor Author

thepwagner commented Mar 30, 2022

@jurre I think the issue is with how bin/docker-dev-shell is setting TARGET_ARCH.

From -o go-x86_64.tar.gz in the output, we can reason TARGETARCH=x86_64 for you, but from printf "... go-amd64.tar.gz", I expected TARGETARCH=amd64.

In CI, the bin/docker-dev-shell script isn't executed - so it uses the built-in default of amd64 and works great.
If uname -m returns x86_64 for you, then we probably just need a little extra script in docker-dev-shell to map that to amd64.

I'm confident that TARGET_ARCH=arm64 works as expected on M1:

$ uname -a
Darwin trolololol.local 21.4.0 Darwin Kernel Version 21.4.0: Mon Feb 21 20:35:58 PST 2022; root:xnu-8020.101.4~2/RELEASE_ARM64_T6000 arm64
$ uname -m
arm64

I chose arm64/amd64 as the architecture names based on the Go ecosystem.
aarch64 and/or x64_64 would work just as well, with a little string manipulation in the style of how DART_ARCH is remapped from x86_64 to x64.

💡 I thought maybe docker info -f '{{.Architecture}}' could be used to get arm64/amd64 consistently, since the docker run --platform=linux/${arch} flag accepts those strings. Unfortunately it returns aarch64/x86_64.


Edit: confirmed and pushed a patch for normalizing uname.

Because I broke down and ran the build on two machines, let's go racing! With a clean cache, I clocked:

  • M1Pro+16GB - 479.3s core + 710.4 dev shell = 1189.7s total
  • Hex-i7-MBP+32GB - 618.0s + 928.6 = 1546.6s total

Bundler still did not work on arm64, with a trace similar to the issue description.
I get the same error on amd64, so I may just be using a bad test case? (dependabot-fixtures/business)

@pavera
Copy link
Contributor

pavera commented Mar 31, 2022

Yes @thepwagner I also have confirmed that the same error occurs with bundler on amd64 and arm64. The dry run script for bundler expects/requires a lock file in the repo used for testing. I've also confirmed that bundler works on both amd64 and arm64 as long as the repo has a lock file.

@jurre
Copy link
Member

jurre commented Mar 31, 2022

image

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.

None yet

6 participants