bazelbuild / rules_docker

Rules for building and handling Docker images with Bazel
Apache License 2.0
1.07k stars 694 forks source link

Issue compiling go_image on macOS #690

Closed obeattie closed 2 years ago

obeattie commented 5 years ago

I am trying to compile a go_image target on macOS, and this is failing with an error about missing imports:

$ bazel build //service.api.ping:image
(20:41:03) INFO: Invocation ID: f27684c3-962f-4aba-a5d7-57245f4da640
(20:41:03) INFO: Current date is 2019-02-12
(20:41:03) INFO: Analysed target //service.api.ping:image (0 packages loaded, 0 targets configured).
(20:41:03) INFO: Found 1 target...
(20:41:04) ERROR: /private/var/tmp/_bazel_oliver/40333f6e8ec32396acff18e2ad143eea/external/org_golang_google_grpc/internal/syscall/BUILD.bazel:3:1: GoCompile external/org_golang_google_grpc/internal/syscall/linux_amd64_pure_stripped/go_default_library%/google.golang.org/grpc/internal/syscall.a failed (Exit 1) compile failed: error executing command bazel-out/host/bin/external/io_bazel_rules_go/go/tools/builders/darwin_amd64_stripped/compile -sdk external/go_sdk -installsuffix linux_amd64 -src ... (remaining 14 argument(s) skipped)

Use --sandbox_debug to see verbose messages from the sandbox
GoCompile: missing strict dependencies:
    /private/var/tmp/_bazel_oliver/40333f6e8ec32396acff18e2ad143eea/sandbox/darwin-sandbox/1/execroot/wearedev/external/org_golang_google_grpc/internal/syscall/syscall_linux.go: import of "golang.org/x/sys/unix"
Known dependencies are:
    google.golang.org/grpc/grpclog
Check that imports in Go sources match importpath attributes in deps.
Target //service.api.ping:image failed to build
Use --verbose_failures to see the command lines of failed build steps.
(20:41:04) INFO: Elapsed time: 0.600s, Critical Path: 0.20s
(20:41:04) INFO: 2 processes: 2 darwin-sandbox.
(20:41:04) FAILED: Build did NOT complete successfully

The BUILD file that I am building this from specifies goarch and goos, which I think is as you recommend it:

load("@io_bazel_rules_docker//go:image.bzl", "go_image")
load("@io_bazel_rules_go//go:def.bzl", "go_binary", "go_library")

go_library(
    deps = [
        "//bedrock:go_default_library",
        "//bedrock/filter:go_default_library",
        "//service.api.ping/handler:go_default_library",
    ],
    importpath = "github.com/monzo/wearedev/service.api.ping",
    name = "go_default_library",
    srcs = ["main.go"],
    visibility = ["//service.api.ping:__subpackages__"],
)

go_binary(
    embed = [":go_default_library"],
    name = "service.api.ping",
    out = "service.api.ping",
    visibility = ["//visibility:public"],
)

go_image(
    embed = [":go_default_library"],
    goarch = "amd64",
    goos = "linux",
    name = "image",
    pure = "on",
    visibility = ["//visibility:public"],
)

However, if I specify the platform explicitly like so, it does work: $ bazel build --platforms=@io_bazel_rules_go//go/toolchain:linux_amd64 //service.api.ping:image

It's a little difficult to unpick precisely what's happening, but the failing compilation rule looks to be defined in rules_go, and it does look like the dependency on @org_golang_x_sys//unix:go_default_library is properly specified for the Linux platform:

https://github.com/bazelbuild/rules_go/blob/f5cfc31d4e8de28bf19d0fb1da2ab8f4be0d2cde/third_party/org_golang_google_grpc-gazelle.patch#L1532-L1535

I am pretty sure that this worked without needing to explicitly specify --platforms in a previous release, so I wonder:

Thanks 🙏


nlopezgi commented 5 years ago

hi @obeattie, thanks for creating this issue. I'm surprised this used to work before for you as running go_image on a Mac was not possible before platforms. to answer your questions:

obeattie commented 5 years ago

Lightning fast! ⚡️

That's interesting that it shouldn't have worked before – I might be mistaken but I am 90% sure. In any case I don't think it matters too much; it sounds like plumbing through exec_compatible_with is the right thing to do.

If I can be of any help, I'm very happy to try and help make that a reality.

nlopezgi commented 5 years ago

I'll sync up with @katre to ask if this is possible /easy now or if we should wait for some of his platform features to roll out and ping back this issue.

katre commented 5 years ago

A couple of questions: is the problem that you cannot build the go_binary without setting --platforms to a linux platform, or that you cannot build the go_image?

Adding exec_compatible_with filters the set of execution platforms, and won't change the target platform, which seems to be the problem here.

obeattie commented 5 years ago

With the --platforms flag, it does work, so it's more an issue that (it seems to me) like Bazel shouldn't even try to compile go_image targets for a non-Linux platform.

katre commented 5 years ago

I have worded my question poorly.

  1. Does bazel build //service.api.ping:go_default_library build?
  2. Does bazel build --platforms=@io_bazel_rules_go//go/toolchain:linux_amd64 //service.api.ping:go_default_library build?
katre commented 5 years ago

I am trying to determine if the problem is with the go rules, or the docker rules.

obeattie commented 5 years ago

Yep, both of these commands build successfully.

katre commented 5 years ago

In that case, I suspect the problem is simply that the goarch and goos attributes of go_image aren't being correctly converted to a target platform.

@nlopezgi: You should probably talk to @jayconrod about the right way to handle this. Most likely, you want to do a configuration transition to set the platforms flag based on goarch and goos. Alternately, you should remove the attributes, and instruct users of passing appropriate values for the --platforms flag directly.

Using exec_compatible_with isn't going to do anything.

jayconrod commented 5 years ago

I think go_image passes the goos and goarch attributes to an underlying go_binary, which compiles libraries with an aspect. The aspect does not work with select expressions, so if there's anything in the transitive dependency graph that relies on select, you'll get an error like this.

There are plans to reimplement goos, goarch, and other attributes using configuration transitions. Last time I checked (early December), these weren't quite supported yet. I think they might be in Bazel 0.23.0, but I need to confirm with the folks working on it.

For now, the --platforms flag is the best way to cross-compile with rules_go. Avoid using goos and goarch if possible.

evie404 commented 5 years ago

I hacked around this with https://github.com/bazelbuild/rules_go/compare/master...rickypai:rpai/fix_grpc_darwin

--platforms is probably the better way though

nlopezgi commented 5 years ago

update: our plan is to set up some examples that run on CI to exercise this use case and provide better advice / docs as to how to do this properly with the current features. From there we can move on to see effort / viability of restricting this from running with the incorrect platform.

alex1545 commented 5 years ago

Hi @obeattie, I'm unable to reproduce your failure. I'm still able to successfully bazel build a go_image target that specifies goarch and goos attributes on a mac. Can you please provide a minimal example for me to try and reproduce this?

obeattie commented 5 years ago

@alex1545 👋 I've put together a minimal proof of concept in obeattie/poc-bazel-690. If you clone this repository and try to build the //:image target on a Mac, you should reproduce the failure:

$ bazel build //:image
INFO: Invocation ID: b7914b0c-9c00-4ed0-807e-71bb073179b5
INFO: Analysed target //:image (100 packages loaded, 7737 targets configured).
INFO: Found 1 target...

…

ERROR: /private/var/tmp/_bazel_oliver/e46407b6ebc58174591c4f03b4d163aa/external/org_golang_google_grpc/internal/syscall/BUILD.bazel:3:1: GoCompile external/org_golang_google_grpc/internal/syscall/linux_amd64_pure_stripped/go_default_library%/google.golang.org/grpc/internal/syscall.a failed (Exit 1) compile failed: error executing command bazel-out/host/bin/external/io_bazel_rules_go/go/tools/builders/darwin_amd64_stripped/compile -sdk external/go_sdk -installsuffix linux_amd64 -src ... (remaining 14 argument(s) skipped)

Use --sandbox_debug to see verbose messages from the sandbox
GoCompile: missing strict dependencies:
    /private/var/tmp/_bazel_oliver/e46407b6ebc58174591c4f03b4d163aa/sandbox/darwin-sandbox/237/execroot/poc/external/org_golang_google_grpc/internal/syscall/syscall_linux.go: import of "golang.org/x/sys/unix"
Known dependencies are:
    google.golang.org/grpc/grpclog
Check that imports in Go sources match importpath attributes in deps.
Target //:image failed to build
Use --verbose_failures to see the command lines of failed build steps.
INFO: Elapsed time: 71.782s, Critical Path: 16.41s
INFO: 237 processes: 237 darwin-sandbox.
FAILED: Build did NOT complete successfully
alex1545 commented 5 years ago

@obeattie, I was now able to reproduce your failure. Thank you for the provided sample.

To answer @katre very first question

A couple of questions: is the problem that you cannot build the go_binary without setting --platforms to a linux platform, or that you cannot build the go_image?

It's both. The problem is that I cannot build the go_binary without setting --platforms to a linux platform and hence the build of the go_image target also fails with the same error. So when I modify this target to be as shown below, I get that failure:

go_binary(
    embed = [":go_default_library"],
    name = "bazel-poc-690",
    out = "bazel-poc-690",
    visibility = ["//visibility:public"],
    goarch = "amd64",
    goos = "linux",
    pure = "on",
)

@jayconrod, I also tried it with Bazel 0.23.2 and I still get the same failure. So it seems that goos and goarch still haven't been reimplemented.

As @jayconrod mentioned, since go_image passes the goos and goarch attributes to an underlying go_binary, our docs for go_image should be consistent with go_binary on how the goos and goarch attributes should be used. You may want to take a look at the goos and goarch notes here.

I'll update our docs to advice that goos and goarch attributes should only be used in situations when the --platforms flag doesn't work.

c16a commented 5 years ago

My issue is similar.

go_binary(
    name = "sample_binary",
    embed = [":go_default_library"],
    visibility = ["//visibility:public"],
)

go_image(
    name = "sample_image",
    srcs = ["main.go"],
    importpath = "com.something.sample",
)

I'm able to run bazel build //:sample_binary and generate the Golang binary. Binary even runs as expected on a Linux host.

But running bazel build //:sample_image throws the missing dependencies error. I've tried the --platforms=@io_bazel_rules_go//go/toolchain:linux_amd64 but no luck.

I'm able to generate the image via a Dockerfile, but I'm eager to do it the Bazel way.

My setup is as below

Prop Info
Bazel version 0.23.1
Bazel build time Mon Mar 4 10:40:32 2019
Golang version go1.12.1 darwin/amd64
OS macOS Mojave 10.14.3
Processor 2.5 Ghz Intel Core i7
Memory 16GB 1600 MHz DDR3

Can someone help?

alex1545 commented 5 years ago

Hi @munukutla, can you please provide a minimal example for which I can reproduce your issue? From your comment I can only assume that you use the same main.go file in your go_library target as you do in your go_image target.

apesternikov commented 5 years ago

We are experiencing the same problem. Anything with dependency on grpc built with goarch = "amd64" goos = "linux" on mac is failing with similar error. I wanted to add that --platforms=@io_bazel_rules_go//go/toolchain:linux_amd64 is not a good solution because we are building both local and linux binaries (for containers) on mac in a single build run bazel build //... looks like this issue belongs to rules_go, I would boil this down to simpler form without rules_docker and crosspost to rules_go.

nlopezgi commented 5 years ago

I don't think its possible at all to do what you are trying to do: build with a single command both local (for mac) and linux (for container) targets. You need to do this in two commands.

@katre for comments on status of doing a multi platform build that produces outputs for 2 platforms in a single bazel command.

apesternikov commented 5 years ago

@nlopezgi It is certainly possible and we are currently using this in production.

katre commented 5 years ago

Right now we don't support multi-platform builds in the same Bazel command. This is on our roadmap (https://bazel.build/roadmaps/configuration.html) but not being worked on currently.

apesternikov commented 5 years ago

@katre I would like to correct myself. It is working at least for go and java - two languages that cross compile natively (mostly) without special support in a build system, so the stated goal is every day reality for us and it is beautiful. You are talking about solving in in general for all languages, which is completely different story. It would be fantastic to see it working.

nlopezgi commented 5 years ago

thanks for clarifying @katre, @apesternikov, good to know that for some languages you are able to leverage their cross-platform / platform-agnostic capabilities to use build outputs produced from a single build command in multiple platforms.

From the perspective of the original ask, iiuc, until multi platform builds in Bazel are possible, we will not be able to support building (from a mac) a go_image target and also building some other targets that produce outputs for mac as part of the same command.

excavador commented 5 years ago

I don't think its possible at all to do what you are trying to do: build with a single command both local (for mac) and linux (for container) targets. You need to do this in two commands.

@nlopezgi but what about host tools? We have a lot of DSL tools in our project. In our case when we are building linux image we also need to build some OSX binaries as stage of the build final binary.

How it supposed to work?

nlopezgi commented 5 years ago

@excavador I'm not sure I understand your question or how I can answer. Are you talking about something that is already working when you say "our case when we are building linux image we also need to build some OSX binaries" or are you saying this is what you want to do and asking me how to achieve it?

joneshf commented 3 years ago

Now that transitions have been out for a bit, it seems like one of the linked PRs: https://github.com/Yolk-HQ/rules_docker/pull/1 might be worth revisiting. With bazel 4.0.0 and rules_docker 0.17.0, the diff these days could look something like:

diff --git a/lang/image.bzl b/lang/image.bzl
index c2db8f4..3d53bd1 100644
--- a/lang/image.bzl
+++ b/lang/image.bzl
@@ -34,8 +34,8 @@ def _binary_name(ctx):
     # /app/foo/bar/baz/blah
     return "/".join([
         ctx.attr.directory,
-        ctx.attr.binary.label.package,
-        ctx.attr.binary.label.name,
+        ctx.attr.binary[0].label.package,
+        ctx.attr.binary[0].label.name,
     ])

 def _runfiles_dir(ctx):
@@ -148,7 +148,7 @@ def _app_layer_impl(ctx, runfiles = None, emptyfiles = None):
     parent_parts = _get_layers(ctx, ctx.attr.name, ctx.attr.base)
     filepath = _final_file_path if ctx.attr.binary else layer_file_path
     emptyfilepath = _final_emptyfile_path if ctx.attr.binary else _layer_emptyfile_path
-    dep = ctx.attr.dep or ctx.attr.binary
+    dep = (ctx.attr.dep or ctx.attr.binary)[0]
     top_layer = ctx.attr.binary and not ctx.attr.dep

     if ctx.attr.create_empty_workspace_dir:
@@ -239,6 +239,20 @@ def _app_layer_impl(ctx, runfiles = None, emptyfiles = None):
         null_cmd = args == [],
     )

+def _container_transition_impl(settings, attr):
+    _ignore = (settings, attr)
+    return {
+        "//command_line_option:platforms": "@io_bazel_rules_go//go/toolchain:linux_amd64",
+    }
+
+container_transition = transition(
+    implementation = _container_transition_impl,
+    inputs = [],
+    outputs = [
+        "//command_line_option:platforms",
+    ],
+)
+
 image = struct(
     attrs = dicts.add(_container.image.attrs, {
         # The base image on which to overlay the dependency layers.
@@ -250,7 +264,7 @@ image = struct(
         # the runfiles dir.
         "binary": attr.label(
             executable = True,
-            cfg = "target",
+            cfg = container_transition,
         ),
         # Set this to true to create an empty workspace directory under the
         # app directory specified as the 'directory' attribute.
@@ -263,11 +277,16 @@ image = struct(
         # The dependency whose runfiles we're appending.
         # If not specified, then the layer will be treated as the top layer,
         # and all remaining deps of "binary" will be added under runfiles.
-        "dep": attr.label(),
+        "dep": attr.label(
+            cfg = container_transition,
+        ),
         "directory": attr.string(default = "/app"),
         "entrypoint": attr.string_list(default = []),
         "legacy_run_behavior": attr.bool(default = False),
         "workdir": attr.string(default = ""),
+        "_allowlist_function_transition": attr.label(
+            default = "@bazel_tools//tools/allowlists/function_transition_allowlist",
+        ),
     }),
     outputs = _container.image.outputs,
     toolchains = ["@io_bazel_rules_docker//toolchains/docker:toolchain_type"],

I've tried this out on a go_image being built on macOS Catalina, and it seems to work without requiring the --platforms flag.

Would a PR to make this change be the way to move forward, or is a transition not the thing to do here?

ajbouh commented 3 years ago

I'm excited for transitions to be part of rules_docker. It will simplify the creation of docker images and continue to strengthen the community.

I've been using a patch like this locally along with an experimental cross compiler toolchain to make it much easier to do hermetic builds for docker images. (https://github.com/ajbouh/bazel-zig-cc for the curious.)

I'm also using this approach to get proper cgo support for my go_image targets.

gravypod commented 2 years ago

Hey all. I don't currently have access to a mac to test these fixes so I wouldn't really feel comfortable putting together a PR for this. If someone sends one in I'd review it. We do have MacOS CI runners that could test this so if the suggestion in this comment works we should go for it.