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

feat: publish multi-arch images #199

Merged
merged 11 commits into from
Mar 25, 2024
Merged

Conversation

alexeagle
Copy link
Contributor

Allows bb-storage users to deploy on arm64 architecture, e.g. AWS Graviton. Fixes #198

cmd/bb_copy/BUILD.bazel Outdated Show resolved Hide resolved
@alexeagle
Copy link
Contributor Author

I just learned about #167 , sorry this is replacing that I think

cmd/bb_copy/BUILD.bazel Outdated Show resolved Hide resolved
cmd/bb_copy/BUILD.bazel Outdated Show resolved Hide resolved
tools/container.bzl Outdated Show resolved Hide resolved
tools/container.bzl Outdated Show resolved Hide resolved
@alexeagle
Copy link
Contributor Author

Do you think my change is responsible for the gazelle failure on CI?

@EdSchouten
Copy link
Member

EdSchouten commented Mar 16, 2024

Do you think my change is responsible for the gazelle failure on CI?

Yes. I can reproduce it by running the commands run by the CI pipeline on my local system. It looks like I can address it by calling gazelle_dependencies() earlier on:

diff --git a/WORKSPACE b/WORKSPACE
index 8806a54..9747956 100644
--- a/WORKSPACE
+++ b/WORKSPACE
@@ -73,6 +73,8 @@ go_rules_dependencies()
 
 go_register_toolchains(version = "1.21.5")
 
+gazelle_dependencies()
+
 load("@rules_oci//oci:pull.bzl", "oci_pull")
 load("@rules_oci//oci:repositories.bzl", "LATEST_CRANE_VERSION", "oci_register_toolchains")
 
@@ -101,8 +103,6 @@ oci_pull(
     ],
 )
 
-gazelle_dependencies()
-
 http_archive(
     name = "com_github_bazelbuild_buildtools",
     sha256 = "09a94213ea0d4a844e991374511fb0d44650e9c321799ec5d5dd28b250d82ca3",

@alexeagle
Copy link
Contributor Author

PTAL - I have not integration-tested this by pushing a container and running a service from it.

tools/container.bzl Show resolved Hide resolved
tools/container.bzl Show resolved Hide resolved
WORKSPACE Show resolved Hide resolved
@moroten
Copy link
Contributor

moroten commented Mar 19, 2024

I created bazel-contrib/rules_oci#531 to make oci_image_index perform the target platform transition instead.

EdSchouten added a commit that referenced this pull request Mar 20, 2024
We should at some point upgrade to Bazel 7.x, but that requires more
bzlmod trickery. Let's first jump to 6.4.0, so that Alex can use
native.package_relative_label() in PR #199.
EdSchouten added a commit that referenced this pull request Mar 20, 2024
We should at some point upgrade to Bazel 7.x, but that requires more
bzlmod trickery. Let's first jump to 6.4.0, so that Alex can use
native.package_relative_label() in PR #199.
EdSchouten added a commit that referenced this pull request Mar 20, 2024
We should at some point upgrade to Bazel 7.x, but that requires more
bzlmod trickery. Let's first jump to 6.4.0, so that Alex can use
native.package_relative_label() in PR #199.
@EdSchouten
Copy link
Member

I saw that that PR build of yours failed with some protoc code generation issues. I thought that would be caused by the Bazel 6.1.0 upgrade, so that's why I just did an upgrade to Bazel 6.4.0. Be sure to rebase your PR on top of master to see if it goes through now.

arch_image_target = "{}_{}_image".format(name, arch)
target_platform = "@io_bazel_rules_go//go/toolchain:linux_{}".format(arch)
images.append(arch_image_target)
platform_transition_filegroup(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note, if we accept bazel-contrib/rules_oci#531 I'll cut a release there and come back in a follow-up PR to simplify this code to use it.

WORKSPACE Outdated Show resolved Hide resolved
@alexeagle
Copy link
Contributor Author

@moroten FYI that I tried patching your rules_oci PR but it still fails on linux/i386 in the same way as this PR does.

https://github.com/buildbarn/bb-storage/actions/runs/8397776269/job/23006173303?pr=201

@alexeagle
Copy link
Contributor Author

@EdSchouten I'm hoping my last commit made it green - do you have something like a label you could apply so the workflow doesn't require a manual approval when I push commits? 🙏🏻

@EdSchouten
Copy link
Member

Github auto approves workflows if you have made at least 1 commit to this repo. So after I merge this change, any followup PR made by you should automatically run the build.

@alexeagle
Copy link
Contributor Author

Hooray it's green, anything else you'd like me to change?

@EdSchouten EdSchouten merged commit ffa88a0 into buildbarn:master Mar 25, 2024
1 check passed
@alexeagle alexeagle deleted the multiarch branch March 25, 2024 21:25
@moroten
Copy link
Contributor

moroten commented Mar 26, 2024

Thank you for merging this. The ideas with bazel-contrib/rules_oci#531 for multi arch images will take some time to get ready.

alexeagle added a commit to alexeagle/bb-remote-execution that referenced this pull request Mar 26, 2024
Lets us re-use the multi-arch image setup from buildbarn/bb-storage#199
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.

Publish images with arm64
4 participants