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

[ISSUE #194] Fix build error on aarch64/arm platform #338

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

yg0x01
Copy link

@yg0x01 yg0x01 commented Dec 26, 2020

What is the purpose of the change

Fix build error on aarch64 platform

Brief changelog

The build.sh runs failed on arm64. I checked and fix some error, add some judgement logic on the point which could failed on arm64.

  1. In CmakeList.txt there is -m32 and -m64 option for compile, but for arm64 this params should not exist. I add an option named BUILD_FOR_ARM and when it is ON there will no -m32/64 parameter which lead build error on arm. In build.sh we should add this option when detect arm platform.
  2. Old openssl-1.1.1d build failed on arm, but 1.1.1i version fix the error, so we should update openssl version for arm.
  3. Jsoncpp copy .a from x86_64-linux-gnu folder, but on arm it should be aarch64-linux-gnu.
  4. Boost b2 tool have a option architecture for build arm. When we build from arm we should add it.

Verifying this change

XXXX

Follow this checklist to help us incorporate your contribution quickly and easily. Notice, it would be helpful if you could finish the following 5 checklist(the last one is not necessary)before request the community to review your PR.

  • Make sure there is a Github issue filed for the change (usually before you start working on it). Trivial changes like typos do not require a Github issue. Your pull request should address just this issue, without pulling in other changes - one PR resolves one issue.
  • Format the pull request title like [ISSUE #123] Fix UnknownException when host config not exist. Each commit in the pull request should have a meaningful subject line and body.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Write necessary unit-test(over 80% coverage) to verify your logic correction, more mock a little better when a cross-module dependency exists.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

@@ -29,7 +29,7 @@ declare fname_openssl="openssl*.tar.gz"
declare fname_libevent="libevent*.zip"
declare fname_jsoncpp="jsoncpp*.zip"
declare fname_boost="boost*.tar.gz"
declare fname_openssl_down="openssl-1.1.1d.tar.gz"
declare fname_openssl_down="openssl-1.1.1i.tar.gz"
Copy link
Member

Choose a reason for hiding this comment

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

Do you have sufficient test with 1.1i?

Copy link
Author

Choose a reason for hiding this comment

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

Actually no. I don't have any idea to make sure 1.1.1i make everything right but it passed build.sh ExecutionTesting process and I use the .so/.a with python client in some simple scenarios (push/pull consumer and producer) on arm/x86_64. It all works fine. Maybe some more tests should be done but I can't find any documents or resource.

Copy link
Member

Choose a reason for hiding this comment

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

Thank you for your honesty, this pr will be followed up by the community for a long time until a convincible security test. I'm happy to follow up on it. If you have compilation problems, you could use the patch to bypass software defects :-)

Copy link
Author

Choose a reason for hiding this comment

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

Thanks. That's the best way to ensure stability.

Copy link

Choose a reason for hiding this comment

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

There are many existing cves in 1.1.1d

Copy link
Contributor

Choose a reason for hiding this comment

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

OpenSSL is supposed to be dynamically linked. SDK should not care about the version of OpenSSL that is eventually used as long as they share compatible ABI.

@vongosling vongosling added disscuss Something undering disscussing refactor refactor functions labels Dec 29, 2020
CMakeLists.txt Outdated
list(APPEND CXX_FLAGS "-m64")
endif ()
option(BUILD_FOR_ARM "Build for arm64 platform" ON)
if (NOT ${BUILD_FOR_ARM} MATCHES "ON")
Copy link
Contributor

Choose a reason for hiding this comment

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

no “matches” is ok

Copy link
Author

Choose a reason for hiding this comment

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

Ok.

build.sh Outdated
Comment on lines 347 to 351
if [ "$(uname -m)" = "aarch64" ]; then
cp ${install_lib_dir}/lib/aarch64-linux-gnu/libjsoncpp.a ${install_lib_dir}/lib/
else
cp ${install_lib_dir}/lib/x86_64-linux-gnu/libjsoncpp.a ${install_lib_dir}/lib/
fi
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems "$(uname -m)-linux-gnu" is better.

Copy link
Author

Choose a reason for hiding this comment

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

You are right. I will fix it.

@@ -29,7 +29,7 @@ declare fname_openssl="openssl*.tar.gz"
declare fname_libevent="libevent*.zip"
declare fname_jsoncpp="jsoncpp*.zip"
declare fname_boost="boost*.tar.gz"
declare fname_openssl_down="openssl-1.1.1d.tar.gz"
declare fname_openssl_down="openssl-1.1.1i.tar.gz"
Copy link

Choose a reason for hiding this comment

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

There are many existing cves in 1.1.1d

else () #not-condition
list(APPEND CXX_FLAGS "-m64")
endif ()
option(BUILD_FOR_ARM "Build for arm64 platform" OFF)
Copy link

Choose a reason for hiding this comment

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

Why not detect the Arm platform and if it's Arm then skip this code instead of having a build option that means people building for Arm need to know to set this?

else
./b2 -j$cpu_num cflags=-fPIC cxxflags=-fPIC --with-atomic --with-thread --with-system --with-chrono --with-date_time --with-log --with-regex --with-serialization --with-filesystem --with-locale --with-iostreams threading=multi link=static release install --prefix=${install_lib_dir}
if [ "$(uname -m)" = "aarch64" ]; then
./b2 -j$cpu_num cflags=-fPIC cxxflags=-fPIC --with-atomic --with-thread --with-system --with-chrono --with-date_time --with-log --with-regex --with-serialization --with-filesystem --with-locale --with-iostreams threading=multi link=static release install --prefix=${install_lib_dir} architecture=arm
Copy link

Choose a reason for hiding this comment

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

why is it necessary to pass architecture=arm when building boost natively?

@leizhiyuan
Copy link

What other problems are there at the moment? It is still necessary for some users to use it on a Mac arm.

@leizhiyuan
Copy link

now arm may be arm64
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disscuss Something undering disscussing refactor refactor functions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants