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

Add ppc64le support #156

Merged
merged 3 commits into from
Jan 13, 2024
Merged

Add ppc64le support #156

merged 3 commits into from
Jan 13, 2024

Conversation

Jenkins-J
Copy link
Contributor

Add ppc64le support to Rust bullseye and bookworm images. The Debian buster container image does not currently support the ppc64le architecture. This builds on the work in pull request #101 and addresses issue #54.

@mkumatag
Copy link

@sfackler can you please review this PR?

@clnperez
Copy link

@Jenkins-J looks like this needs a rebase

@Jenkins-J
Copy link
Contributor Author

@sfackler I've rebased the changes for this pull request. Is there anything else I need to do to have this reviewed and merged?

@Jenkins-J
Copy link
Contributor Author

@sfackler there are projects such as https://github.com/IBM/text-generation-inference that aim to support the ppc64le architecture and would benefit from ppc64le support in the official Rust Docker image. Are there any other blockers to adding ppc64le support?

Add ppc64le support to Debian bullseye and bookworm. Debian buster does
not support the ppc64le architecture.
@Jenkins-J
Copy link
Contributor Author

@sfackler @Muscraft I have rebased the changes on the master branch again. Is there anything else holding this PR back from being merged?

Copy link
Member

@Muscraft Muscraft left a comment

Choose a reason for hiding this comment

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

These lines will also need to be changed:

docker-rust/x.py

Lines 189 to 192 in 743e6f4

library += single_library(
tags,
map(lambda a: a.bashbrew, debian_arches),
os.path.join(rust_version, variant))

        arches = debian_arches
        if variant != "buster":
            arches += debian_non_buster_arches

        library += single_library(
                tags,
                map(lambda a: a.bashbrew, arches),
                os.path.join(rust_version, variant)) 

docker-rust/x.py

Lines 203 to 206 in 743e6f4

library += single_library(
tags,
map(lambda a: a.bashbrew, debian_arches),
os.path.join(rust_version, variant, "slim"))

        library += single_library(
                tags,
                map(lambda a: a.bashbrew, arches),
                os.path.join(rust_version, variant, "slim"))

x.py Outdated Show resolved Hide resolved
x.py Outdated Show resolved Hide resolved
x.py Outdated Show resolved Hide resolved
x.py Outdated Show resolved Hide resolved
x.py Outdated Show resolved Hide resolved
@Muscraft
Copy link
Member

The reason this has been hard to decide on is there is currently no policy on what architectures we support similar to rustc's Platform Support.

After talking with @sfackler, we should support architecture that meets the following criteria:

One caveat is that I don't have any hardware to test images for architectures other than amd64 and arm64v8 so other architectures are best effort.

@@ -18,6 +18,10 @@
DebianArch("i386", "i386", "i686-unknown-linux-gnu"),
]

debian_non_buster_arches = [
DebianArch("ppc64le", "ppc64el", "powerpc64le-unknown-linux-gnu"),
Copy link
Member

Choose a reason for hiding this comment

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

These two are meant to be different correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that is correct. The value for bashbrew is ppc64le while the dpkg architecture value needs to be ppc64el.

@clnperez
Copy link

One caveat is that I don't have any hardware to test images for architectures other than amd64 and arm64v8 so other architectures are best effort.

@Muscraft - We have an agreement with OSU that you can, as a maintainer of an open source project, get an OpenStack-managed Power VM there pretty easily: https://osuosl.org/services/powerdev/request_hosting. They'll spin you up a VM, add your SSH key to it, and it's yours. If you're interested let me know. But we're fine with the best effort approach and are happy to be pinged to fix things as well.

Thanks for the review!

x.py Outdated
@@ -170,9 +183,13 @@ def generate_stackbrew_library():
tags.append(version_tag)
tags.append("latest")

arches = debian_arches
Copy link
Member

@Muscraft Muscraft Jan 12, 2024

Choose a reason for hiding this comment

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

When I suggested this line, I forgot it wasn't copying debian_arches, only creating a reference and then mutating it (I deeply miss Rust when I am not using it). It caused ppc64le to be added twice. To fix this, we just need to copy it.

Suggested change
arches = debian_arches
arches = debian_arches[:]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you, it is fixed now.

Follow requested changes, improving names of debian architecture lists.
Follow requested changes, updated variable names and reorganized logic for
improved readability.
@Muscraft Muscraft merged commit b1268c2 into rust-lang:master Jan 13, 2024
8 checks passed
@Muscraft
Copy link
Member

docker-library/official-images#16055

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.

4 participants