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.

Sign up for GitHub

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

Jump to bottom

Add support for arm64 #3681

Closed
wants to merge 1 commit into from
Closed

Add support for arm64 #3681

wants to merge 1 commit into from

Conversation

lubinsz
Copy link
Contributor

@lubinsz lubinsz commented Jun 21, 2018 •
edited
Loading

Signed-off-by: Bin Lu [email protected]

LuaJIT2.0 does not support for Arm64

Luajit 2.0.5 doesn't support Arm64, please see logs as reference.
So I need to update it into the latest version.
Logs:
bazel fetch //source/...
….
==== Building LuaJIT 2.0.5 ====
make -C src
make[2]: Entering directory '/root/.cache/bazel/_bazel_root/38a3de7f29554e17d452de52137c58b9/external/envoy_deps_cache_2c744dffd279d7e9e0910ce594eb4f4f/luajit.dep.build/LuaJIT-2.0.5/src'
lj_arch.h:55:2: error: #error "No support for this architecture (yet)"
#error "No support for this architecture (yet)"
^

Risk Level: Medium

Testing: unit test,integration

Docs Changes: None

Release Notes: None

@lubinsz lubinsz force-pushed the master branch 3 times, most recently from 7929099 to 6ddc282 Compare June 21, 2018 09:45
@moderation
Copy link
Contributor

moderation commented Jun 21, 2018 •
edited
Loading

See #1861 (comment) for details on how to compile out Luajit for arm64 builds. The bigger issue is that rules_go does not yet support arm64 / aarch64. See https://github.com/moderation/rules_go/commit/b6ce43d0e7fd58eba01d992140a685cc2775cde4 for a possible fix. Lastly the protoc-gen-validate (PGV) dependency won't compile on arm64 / aarch64 either - bufbuild/protoc-gen-validate#75

With your changes @lubinsz, are you able to successfully compile on arm64?

mattklein123
mattklein123 reviewed Jun 21, 2018
View reviewed changes
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution. Please update the PR title and description to be clear this is about LuaJIT. Also please clean up the description template. Thanks!

ci/do_ci.sh Outdated
@@ -55,6 +55,7 @@ if [[ "$1" == "bazel.release" ]]; then
echo "bazel release build with tests..."
bazel_release_binary_build
echo "Testing..."
bazel clean
Copy link
Member

Choose a reason for hiding this comment

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

Why are these cleans necessary?

ci/build_container/build_recipes/luajit.sh
@@ -2,7 +2,7 @@

set -e

VERSION=2.0.5
VERSION=2.1.0-beta3
Copy link
Member

Choose a reason for hiding this comment

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

I don't personally have an objection to upgrading to 2.1, but worth a discussion if anyone else cares about this.

@lubinsz
Copy link
Contributor Author

lubinsz commented Jun 22, 2018

@moderation
The compilation can be done on Arm64 platform with my internal patch for bazel(bazel, rules_go, bazel-gazelle).
But unfortunately, my pr for bazel/rules_go/bazel-gazelle were delayed by our internal legal review.
I will deliver them as soon as possible.

Please see logs with my bazel@arm64 as reference:

Log 1:
file bazel-bin/source/exe/envoy-static

bazel-bin/source/exe/envoy-static: ELF 64-bit LSB executable, ARM aarch64, version 1 (SYSV), dynamically linked, interpreter /lib/ld-linux-aarch64.so.1, for GNU/Linux 3.7.0, BuildID[md5/uuid]=99c6fad8c346d5c71439f57b0208de26, not stripped

Log 2,
root@bin:/go/src/github.com/envoyproxy/envoy# uname -p
aarch64

Log 3,
root@bin:/go/src/github.com/envoyproxy/envoy# bazel fetch //source/...
…

root@bin:/go/src/github.com/envoyproxy/envoy# bazel build //source/exe:envoy-static
….
….
Target //source/exe:envoy-static up-to-date:
bazel-bin/source/exe/envoy-static
INFO: Elapsed time: 80.411s, Critical Path: 58.40s
INFO: 118 processes, linux-sandbox.
INFO: Build completed successfully, 120 total actions

@moderation
Copy link
Contributor

It would be great if you could share your patches. I've been trying and failing to compile with a mixture of Bazel fixes from https://zhiyisun.github.io/2017/02/15/Running-Google-Machine-Learning-Library-Tensorflow-On-ARM-64-bit-Platform.html, my patch to rules_go at https://github.com/moderation/rules_go/commit/b6ce43d0e7fd58eba01d992140a685cc2775cde4

@lubinsz
Copy link
Contributor Author

lubinsz commented Jun 26, 2018

@moderation
Please see my test steps as reference.
1, //get my arm64 patch for bazel
wget https://github.com/lubinsz/bazel/files/2135817/0001-Add-support-for-bazel-arm64.txt --no-check-certificate
2, //download bazel
wget https://github.com/bazelbuild/bazel/releases/download/0.14.1/bazel-0.14.1-dist.zip
3, //apply my arm64 patch, compile it and move bazel into /usr/bin or /usr/local/bin
patch -p1 <0001-Add-support-for-bazel-arm64.txt
./compile.sh
cp output/bazel /usr/bin/
4, //clone envoy project. I tested it several weeks ago. So please checkout it into 4dd49d8
git clone https://github.com/envoyproxy/envoy.git
cd envoy
git checkout 4dd49d8
wget https://github.com/lubinsz/bazel/files/2135923/0001-Add-support-for-envoy.txt
patch -p1 <0001-Add-support-for-envoy.txt
bazel fetch //source/...
bazel build //source/exe:envoy-static
5, plz see my log as reference
-----------------
root@entos-arm-01:/home/bblu/gerrit/test/envoy# git branch

  • (HEAD detached at 4dd49d8)
    master

bazel fetch //source/...
....
bazel build //source/exe:envoy-static
...
Target //source/exe:envoy-static up-to-date:
bazel-bin/source/exe/envoy-static
INFO: Elapsed time: 408.862s, Critical Path: 321.91s
INFO: 1443 processes: 1438 linux-sandbox, 5 local.
INFO: Build completed successfully, 1849 total actions

@moderation
Copy link
Contributor

moderation commented Jun 26, 2018 •
edited
Loading

@lubinsz Amazing! These patches are working for me. I applied the Bazel changes to 0.15.0 that was released this morning. I compiled out Lua altogether and didn't include your patches. I applied the Envoy patches to master. Success! Thanks for this.

It would be great to get the Bazel changes accepted upstream - do you plan on submitting a PR?

Similarly it would be good to get the rules_go changes accepted as well.

@moderation moderation mentioned this pull request Jun 28, 2018
Closed
@stale
Copy link

stale bot commented Jul 3, 2018

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@stale stale bot added the stale stalebot believes this issue/PR has not been touched recently label Jul 3, 2018
@vielmetti
Copy link

The CI is failing in "coverage" at

INFO: From ProtoCompile external/envoy_api/envoy/service/discovery/v2/hds.pb.cc:

and at "asan" at

bazel-out/k8-dbg/genfiles/external/com_google_protobuf/src: warning: directory does not exist.

@mattklein123 can you take a look at the failing CI tests and suggest a next course of action?

@stale stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Jul 3, 2018
@mattklein123
Copy link
Member

Merge master and see if it fixes itself. Beyond that, I'm not sure and I would see if you can repro locally within docker.

@stale
Copy link

stale bot commented Jul 10, 2018

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@stale stale bot added the stale stalebot believes this issue/PR has not been touched recently label Jul 10, 2018
@vielmetti
Copy link

@moderation @lubinsz please take a look

@stale stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Jul 12, 2018
@stale
Copy link

stale bot commented Jul 19, 2018

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@stale stale bot added the stale stalebot believes this issue/PR has not been touched recently label Jul 19, 2018
@stale
Copy link

stale bot commented Jul 26, 2018

This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@stale stale bot closed this Jul 26, 2018
@htuch
Copy link
Member

htuch commented Aug 8, 2018

@lubinsz wondering where we are with ARM support? I've heard from some folks interested in this, any chance we can land the required PRs?

@moderation
Copy link
Contributor

I'm compiling on arm64 and just set up a new Pine64 Rockpro64 (6 core) system. I'm using the latest Bazel 0.16.0. The patches to Bazel have changed a bit since @lubinsz posted the original patch. The discrepancy between Go using the term arm64 and the rest of the world and the chips using aarch64 is a mess. @lubinsz fork of rules_go is still required. I am not compiling Lua and have changed my WORKSPACE with

envoy_dependencies(
    skip_targets=['luajit']
)

With Cloudflare moving a lot of their stack to ARM and reducing their use of NGINX (https://twitter.com/eastdakota/status/1024515150546493440), you'd think they would be a perfect candidate.

@moderation
Copy link
Contributor

There is a PR to support arm64 for rules_go at bazel-contrib/rules_go#1550. Has a dependency on a massive closed Bazel PR. Looks like they are targeting for Bazel 0.17.0

@ptone
Copy link

ptone commented Aug 8, 2018

Thanks for these updates @moderation I was trying to get a build done with the steps in #3681 (comment) but did not work.

Any chance you have a gist of the steps you are using to get a working build? Since Bazel 0.16 was just released, maybe it will be a little while till that other work is available.

@moderation
Copy link
Contributor

@ptone take a look at https://gitlab.com/snippets/1742074. The delay was testing backing our the arm64 terminology which seems to be Go specific and standardizing on aarch64 for the Bazel changes. You'll see how this is handled in the rules_go fork. Let me know if this makes sense or if you have questions.

@ptone
Copy link

ptone commented Aug 9, 2018

Thanks @moderation I have now gotten farther, with less patched lines, but still haven't won in this latest round.

I had a couple other issues I may have solved (which may have broken something else)

I had to remove luajit as external dep
https://gist.github.com/ptone/3524e93b51502cbef526eea34b7c2ca4#file-envoy-diff-L37

I was getting bizarre errors about a output fcolor-diagnostics flag:
https://gist.github.com/ptone/3524e93b51502cbef526eea34b7c2ca4#file-bazel-diff-L47

In the end, the fatal error seemed to related to files that were on disk, but had something wrong about the way they were referenced.

My build context is on a roc-rk3328 system running armbian distro

@moderation
Copy link
Contributor

moderation commented Aug 9, 2018 •
edited
Loading

@ptone I mentioned compiling out Lua above but omitted it from the snippet. I've updated the snippet now to include that detail. I also link to my minimal extensions_build_config.bzl file that only compiles in what I use and shortens compilation time. I also include my build command. Also, last night I upgrade the Bazel on my Rock64 to 0.16.0. Considering the small amount of edits I tried compiling Bazel without any modifications and it compiled, and with my rules_go fork it successfully compiled Envoy!

@lubinsz
Copy link
Contributor Author

lubinsz commented Oct 22, 2018

@moderation @ptone @vielmetti @mattklein123
Hi all,
I have rebase the code , also fix a bug in source/exe/signal_action.cc.
Now, envoy can be build on Arm64 platform.
Please see #4809

@lubinsz
Copy link
Contributor Author

lubinsz commented Oct 22, 2018

@moderation @ptone @vielmetti @mattklein123
Hi all,
I have rebased the code, also fix a bug in source/exe/signal_action.cc.
Now, envoy can be built on Arm64.
Please see #4809

4 similar comments
@lubinsz
Copy link
Contributor Author

lubinsz commented Oct 22, 2018

@moderation @ptone @vielmetti @mattklein123
Hi all,
I have rebased the code, also fix a bug in source/exe/signal_action.cc.
Now, envoy can be built on Arm64.
Please see #4809

@lubinsz
Copy link
Contributor Author

lubinsz commented Oct 22, 2018

@moderation @ptone @vielmetti @mattklein123
Hi all,
I have rebased the code, also fix a bug in source/exe/signal_action.cc.
Now, envoy can be built on Arm64.
Please see #4809

@lubinsz
Copy link
Contributor Author

lubinsz commented Oct 22, 2018

@moderation @ptone @vielmetti @mattklein123
Hi all,
I have rebased the code, also fix a bug in source/exe/signal_action.cc.
Now, envoy can be built on Arm64.
Please see #4809

@lubinsz
Copy link
Contributor Author

lubinsz commented Oct 22, 2018

@moderation @ptone @vielmetti @mattklein123
Hi all,
I have rebased the code, also fix a bug in source/exe/signal_action.cc.
Now, envoy can be built on Arm64.
Please see #4809

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattklein123 mattklein123 mattklein123 left review comments

Assignees

@zuercher zuercher

@mattklein123 mattklein123

Labels
stale stalebot believes this issue/PR has not been touched recently
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@lubinsz @moderation @vielmetti @mattklein123 @htuch @ptone @zuercher