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

Better ppc64le support and minor improvements #184

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

mgiessing
Copy link

This is PR helps in improving the ppc64le build container (and minor improvement for the other archs), however the bazelisk part is not yet supported, so the correct bazel version needs to be downloaded manually.

Signed-off-by: Marvin Giessing <marvin.giessing@gmail.com>
Signed-off-by: Marvin Giessing <marvin.giessing@gmail.com>
@mgiessing mgiessing requested a review from a team as a code owner August 25, 2022 21:28
Signed-off-by: Marvin Giessing <marvin.giessing@gmail.com>
Signed-off-by: Marvin Giessing <marvin.giessing@gmail.com>
Signed-off-by: Marvin Giessing <marvin.giessing@gmail.com>
Signed-off-by: Marvin Giessing <marvin.giessing@gmail.com>
Copy link
Member

@lizan lizan left a comment

Choose a reason for hiding this comment

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

Nice work!

;;
'x86_64' )
LLVM_DISTRO=x86_64-linux-gnu-ubuntu-18.04
LLVM_SHA256SUM=61582215dafafb7b576ea30cc136be92c877ba1f1c31ddbbd372d6d65622fef5
apt-get install -y --no-install-recommends \
google-cloud-sdk \
Copy link
Member

Choose a reason for hiding this comment

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

why do we need google-cloud-sdk here? you can add this only if x86_64 and aarch64 to PACKAGES in above.

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure whether there is a more elegant solution but I'll append the google-cloud-sdk if ARCH is x86 or arm ok?
Will look like this:

PACKAGES=(
    [...]
)

if [[ "${ARCH}" == "x86_64" || "${ARCH}" == "aarch64" ]]; then
  PACKAGES+=("google-cloud-sdk")
fi

I'll also add libtinfo5 in PACKAGES, then we can remove the complete apt-get install ... in the LLVM part.

Would that be fine with you?

@@ -86,24 +75,31 @@ case $ARCH in
'ppc64le' )
LLVM_DISTRO=powerpc64le-linux-ubuntu-18.04
LLVM_SHA256SUM=2d504c4920885c86b306358846178bc2232dfac83b47c3b1d05861a8162980e6
apt-get install -y --no-install-recommends \
Copy link
Member

Choose a reason for hiding this comment

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

this can be outside of the case statement once google-cloud-sdk handling is moved up.

Copy link
Author

Choose a reason for hiding this comment

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

As described above I think we can remove all apt-get ... in all the LLVM part because that is then handled via PACKAGES already.

Would look like this

# Set LLVM version for each cpu architecture.
LLVM_VERSION=14.0.0
case $ARCH in
    'ppc64le' )
        LLVM_DISTRO=powerpc64le-linux-ubuntu-18.04
        LLVM_SHA256SUM=2d504c4920885c86b306358846178bc2232dfac83b47c3b1d05861a8162980e6
        ;;
    'x86_64' )
        LLVM_DISTRO=x86_64-linux-gnu-ubuntu-18.04
        LLVM_SHA256SUM=61582215dafafb7b576ea30cc136be92c877ba1f1c31ddbbd372d6d65622fef5
        ;;
    'aarch64' )
        LLVM_DISTRO=aarch64-linux-gnu
        LLVM_SHA256SUM=1792badcd44066c79148ffeb1746058422cc9d838462be07e3cb19a4b724a1ee
        ;;
esac

Would that be fine with you?

@@ -18,7 +18,7 @@ config_env() {

if [[ -z "${BUILD_TOOLS_PLATFORMS}" ]]; then
if [[ "${OS_DISTRO}" == "ubuntu" ]]; then
export BUILD_TOOLS_PLATFORMS=linux/arm64,linux/amd64
export BUILD_TOOLS_PLATFORMS=linux/arm64,linux/amd64,linux/ppc64le
Copy link
Member

Choose a reason for hiding this comment

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

let's hold off this for now as it drastically increases image build time in CI with emulation + building tools.

Copy link
Author

Choose a reason for hiding this comment

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

Sure - I can remove this with the next commit I'm just wondering if you don't want to have it built at all or just while this PR is not finally approved.

Thanks!

Signed-off-by: Marvin Giessing <marvin.giessing@gmail.com>
@mgiessing
Copy link
Author

@lizan I've updated the PR accordingly - can you have another check? :)
Thanks!

@lehrig
Copy link

lehrig commented Jan 3, 2023

I'm also interested in simplifying the ppc build - is there any update on this @lizan?

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.

3 participants