Skip to content

Allow s390x arch #93

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Allow s390x arch #93

wants to merge 1 commit into from

Conversation

gongsu832
Copy link

Allow rules to be used on s390x arch.

Signed-off-by: Gong Su <gong_su@hotmail.com>
@CLAassistant
Copy link

CLAassistant commented Dec 27, 2024

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@srikrsna-buf srikrsna-buf left a comment

Choose a reason for hiding this comment

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

This should not work as there is no binary built for s390x architecture. Were you able to test this on that architecture?

@gongsu832
Copy link
Author

Since this is a dependency of protobuf, the patch allows me to build protobuf on s390x successfully which I couldn't otherwise.

@jayakasadev
Copy link

@srikrsna-buf what else is needed to get this merged?

@srikrsna-buf
Copy link
Member

There is no binary for s390x available, the change in this PR will not be able to download buf

@gongsu832
Copy link
Author

There is no binary for s390x available, the change in this PR will not be able to download buf

Sorry can you be a bit more specific? Why is having a s390x binary for buf a prereq for this change?

@nicksnyder
Copy link
Member

The purpose of the _buf_download_releases_impl function you are editing is to download a version of buf.

There is no version of buf available for s390x: https://github.com/bufbuild/buf/releases/tag/v1.50.1

This PR would change the behavior from failing with an error message Unsupported operating system or cpu architecture to silently not downloaded a version of buf at all when cpu is s390x.

  • L139 won't match any known versions of buf when cpu is s390x
  • L148 will never execute when cpu is s390x

@gongsu832
Copy link
Author

gongsu832 commented Mar 18, 2025

Thank you @nicksnyder for the explanation. Would it be acceptable to add a check to fail the build if the binaries are not found in the sha list, like the following:

diff --git a/buf/internal/toolchain.bzl b/buf/internal/toolchain.bzl
index b2bcc40..03e69be 100644
--- a/buf/internal/toolchain.bzl
+++ b/buf/internal/toolchain.bzl
@@ -130,6 +130,7 @@ def _buf_download_releases_impl(ctx):
     ctx.file("WORKSPACE", "workspace(name = \"{name}\")".format(name = ctx.name))
     ctx.file("toolchain.bzl", _TOOLCHAIN_FILE)
     sha_list = ctx.read("sha256.txt").splitlines()
+    sha_found = False
     for sha_line in sha_list:
         if sha_line.strip(" ").endswith(".tar.gz"):
             continue
@@ -139,6 +140,7 @@ def _buf_download_releases_impl(ctx):
         if lower_case_bin.find(os) == -1 or lower_case_bin.find(cpu) == -1:
             continue

+        sha_found = True
         output = lower_case_bin[:lower_case_bin.find(os) - 1]
         if os == "windows":
             output += ".exe"
@@ -153,6 +155,9 @@ def _buf_download_releases_impl(ctx):
             output = output,
         )

+    if not sha_found:
+        fail("buf binaries not found in the release")
+
     if os == "darwin":
         os = "osx"

Ahh but that would make the build fail on s390x now. Hmmm...looks like we have to get bufbuild to release s390x binaries first.

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.

5 participants