-
Notifications
You must be signed in to change notification settings - Fork 691
Forward architecture from language rules to layers #2150
Conversation
go/image.bzl
Outdated
@@ -89,7 +89,7 @@ STATIC_DEFAULT_BASE = select({ | |||
"//conditions:default": "@go_image_static//image", | |||
}) | |||
|
|||
def go_image(name, base = None, deps = [], layers = [], env = {}, binary = None, **kwargs): | |||
def go_image(name, base = None, deps = [], layers = [], env = {}, binary = None, architecture = "x86_64", **kwargs): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The existing default is amd64 (set in container/image.bzl). Rather than setting a new default here and changing the behavior for existing users, set architecture = None
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, changed for all three.
java/image.bzl
Outdated
@@ -275,6 +275,7 @@ def java_image( | |||
env = {}, | |||
jvm_flags = [], | |||
classpath_as_file = None, | |||
architecture = "x86_64", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, leave the default unset
python3/image.bzl
Outdated
@@ -73,7 +73,7 @@ DEFAULT_BASE = select({ | |||
"//conditions:default": "@py3_image_base//image", | |||
}) | |||
|
|||
def py3_image(name, base = None, deps = [], layers = [], env = {}, **kwargs): | |||
def py3_image(name, base = None, deps = [], layers = [], env = {}, architecture = "x86_64", **kwargs): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, leave the default unset
This change looks great. architecture is defaulted to amd64 in container/image.bzl with no way to control it. Thanks for sending this PR |
I need this change. I was banging my head trying to figure out why I couldn't use go_image to build an arm64 container. I've demonstrated the fix in this repo: https://github.com/shadanan/go-bazel-gazelle-docker-template Agree with Kyle. We should leave the default for architecture unset. |
Respond to feedback from PR review.
Updated, please take a look. |
Looking forward to this change, because I too was caught off guard about only being able to build amd64 images! This would make an already amazing library even better 😍 |
There's a pretty simple workaround for Go that ought to apply to other languages as well - Inline the implementation of Here's a completely untested example:
|
While I'm waiting for this to merge, I basically did what @sfc-gh-kleonhard suggested. I create a file called
And in my BUILD.bazel files, I have this load statement:
That way, when this PR gets merged, I can just delete the |
This Pull Request has been automatically marked as stale because it has not had any activity for 180 days. It will be closed if no further activity occurs in 30 days. |
This PR was automatically closed because it went 30 days without a reply since it was labeled "Can Close?" |
Ideally, this would be able to be inferred from the base image but I don't think it is possible to propagate that information into the build transition. Explicitly setting the architecture seems like the simplest way to trigger the build transition and support non-x86_64 target platforms.
Opening for feedback first.
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Non-x86 target platforms aren't supported.
What is the new behavior?
Non-x86 target platforms are supported.
Does this PR introduce a breaking change?
Other information
Commits (oldest to newest)
8d69206 Forward architecture for py3_image
a9ee0fc Forward architecture for go_image
466a8de Forward architecture for java_image
130dea5 squash! don't set a default architecture
Respond to feedback from PR review.