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

go rules do not support inter package operation #676

Closed
killerfoxi opened this issue Dec 3, 2015 · 13 comments
Closed

go rules do not support inter package operation #676

killerfoxi opened this issue Dec 3, 2015 · 13 comments
Assignees
Labels
P3 We're not considering working on this, but happy to review a PR. (No assignee) type: feature request

Comments

@killerfoxi
Copy link

I stumpled upon this when trying to bazelfy the go grpc package. But I provided a small setup where you can reproduce the issue.

Start with having a main package and package A as well as package B. Have package A interacting with package B and your main package interacting with both packages. You'll end up with typing issues.

./bar/BUILD

package(default_visibility = ["//visibility:public"])

load("/tools/build_rules/go/def", "go_library")

go_library(
  name = "go_default_library",
  srcs = ["bar.go"],
)

./bar/bar.go

package bar

type Bar struct {
}

./foo/BUILD

package(default_visibility = ["//visibility:public"])

load("/tools/build_rules/go/def", "go_library")

go_library(
  name = "go_default_library",
  srcs = ["foo.go"],
  deps = [
    "//test/bar:go_default_library",
  ]
)

./foo/foo.go

package foo

import "./test/bar"

func FooWithBar() *bar.Bar {
        return &bar.Bar{}
}

./BUILD

load("/tools/build_rules/go/def", "go_library")

go_library(
  name = "go_default_library",
  srcs = ["b0rked.go"],
  deps = [
    "//test/foo:go_default_library",
    "//test/bar:go_default_library",
  ],
)

./b0rked.go

package b0rked

import "./test/foo"
import "./test/bar"

func borked() {
        var b *bar.Bar
        b = foo.FooWithBar()
}

You end up with the following error message:

$ bazel build :go_default_library
INFO: Found 1 target...
INFO: From GoCompile test/go_default_library.a:
./test/b0rked.go:8: cannot use foo.FooWithBar() (type *"/home/killerfoxi/.cache/bazel/_bazel_killerfoxi/a3c45aa3028dd8039c18e7604e2918d1/bazel/bazel-out/local_linux-fastbuild/bin/test/foo/go_default_library.a.dir/test/bar/go_default_library".Bar) as type *"/home/killerfoxi/.cache/bazel/_bazel_killerfoxi/a3c45aa3028dd8039c18e7604e2918d1/bazel/bazel-out/local_linux-fastbuild/bin/test/go_default_library.a.dir/test/bar/go_default_library".Bar in assignment
ERROR: /home/projects/bazel/test/BUILD:3:1: error executing shell command: 'rm -rf bazel-out/local_linux-fastbuild/bin/test/go_default_library.a.dir && mkdir -p bazel-out/local_linux-fastbuild/bin/test/go_default_library.a.dir && mkdir -p bazel-out/local_linux-fastbuild/bi...' failed: bash failed: error executing command /bin/bash -c ... (remaining 1 argument(s) skipped): com.google.devtools.build.lib.shell.BadExitStatusException: Process exited with status 1.
Target //test:go_default_library failed to build
Use --verbose_failures to see the command lines of failed build steps.
INFO: Elapsed time: 0.193s, Critical Path: 0.02s

I know that the go rules are very experimental yet. Yet I'm looking forward to have bazel support for it. Any tipps on how to fix this? I understand how this happens, I'm just not sure how to fix it. Thanks!

Btw. I'm using the latest master sync. There also appears to be an off by one error with the ln -s command (one level too much).

@hanwen
Copy link
Contributor

hanwen commented Dec 4, 2015

strange - looks like absolute rather than relative paths are creeping into the objects files

@hanwen
Copy link
Contributor

hanwen commented Dec 4, 2015

have you tried removing the "./" from the import paths?

@killerfoxi
Copy link
Author

Yes, but then it wouldn't find the package at all.

I use go_prefix('') because I don't care about any prefix when I use bazel.

@hanwen
Copy link
Contributor

hanwen commented Dec 4, 2015

why is there "test" in the import names? (your scenario suggests the file names don't have a test prefix.)

@killerfoxi
Copy link
Author

Because I just created a subdirectory called "test". I've other code in that bazel repo for C++. I tend to use one giant bazel repo so I can reuse libs between projects.

@hanwen
Copy link
Contributor

hanwen commented Dec 4, 2015

I think the off-by-one and import without "./" are related. prefix=="" needs to be special cased.

@killerfoxi
Copy link
Author

So how is this supposed to work then if you don't want a prefix? I did as #79 (comment) comment suggested. Because I think the go way is a little stupid here, and I prefer the way of C++. I don't necessary want all my files structured as I would do in go. However, I can live with that, as long as I'm not forced to have a prefix.

What I want is one monolithic structure where I can put my code in and let bazel figure out how to build and combine them (In my case compile protos and make it available for both, my C++ part and my Go part as well as maybe other libraries; I understand that cgo is not yet supported, that's fine for now). Isn't that what bazel is supposed to do?

@hanwen
Copy link
Contributor

hanwen commented Dec 4, 2015

it is arguably a bug that prefix=="" does not work, and I will fix it, but I strongly suggest to use a prefix anyway, the reason being that you otherwise share the namespace with the Go standard library, meaning that you cannot have a directories crypto, net, os, etc. holding Go code in your bazel repository.

@hanwen
Copy link
Contributor

hanwen commented Dec 4, 2015

(there is no requirement that the prefix be a URL, btw. You could take a single string, eg. "killerfoxi"

@hanwen
Copy link
Contributor

hanwen commented Dec 4, 2015

try this patch - I need to figure why it doesn't fix the test, but it seems to work outside the shell tests.

diff --git a/tools/build_rules/go/def.bzl~ b/tools/build_rules/go/def.bzl
index 44a637f..0fb621c 100644
--- a/tools/build_rules/go/def.bzl~
+++ b/tools/build_rules/go/def.bzl
@@ -49,7 +49,7 @@ def _go_prefix_impl(ctx):
def _go_prefix(ctx):
"""slash terminated go-prefix"""
prefix = ctx.attr.go_prefix.go_prefix

  • if not prefix.endswith("/"):
  • if prefix != "" and not prefix.endswith("/"):
    prefix = prefix + "/"
    return prefix

@killerfoxi
Copy link
Author

Mind using md to format it correctly? :) it's a little hard. Btw. test/ doesn't mean it's a test, I just named it that way because I wanted to test this scenario (e.g. provide you with a very low reproducable setup)

@killerfoxi
Copy link
Author

Yay with a prefix it indeed started working with my test example. Let me see if that is also true for the grpc case.

Btw. is there really no easier way than provide a BUILD file per directory? I mean for the C++ libraries I usually provide one BUILD file and have my code in upstream so I can update that part without changing anything. This seems to be not possible with the go rules, because how the system expects this to be organized. It seems a little illogical as with bazel this clearly is possible otherweise.

@killerfoxi
Copy link
Author

Fixing the prefix also seems to fix the typing issue! This was non obvious to me, thanks for your patience and help.

@dslomov dslomov added P3 We're not considering working on this, but happy to review a PR. (No assignee) cloud / appengine / docker labels Dec 15, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P3 We're not considering working on this, but happy to review a PR. (No assignee) type: feature request
Projects
None yet
Development

No branches or pull requests

5 participants