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

Gazelle broken when go_prefix is "" #870

Closed
kgreenek opened this issue Oct 3, 2017 · 4 comments · Fixed by #912
Closed

Gazelle broken when go_prefix is "" #870

kgreenek opened this issue Oct 3, 2017 · 4 comments · Fixed by #912

Comments

@kgreenek
Copy link

kgreenek commented Oct 3, 2017

I have a monorepo, where my go_prefix is set to "". The repository is structured like this:

  • WORKSPACE
  • BUILD.bazel
  • my_prefix
    • foo
      • lib.go
    • bar
      • lib.go

If //my_prefix/foo/lib.go imports //my_prefix/bar, it does so as follows:

import "my_prefix/bar"

In the root BUILD.bazel file, we don't need a go_prefix, because it's already implicit in the directory structure of the monorepo. So we set go_prefix to an empty string:

load("@io_bazel_rules_go//go:def.bzl", "go_prefix")
go_prefix("")

I used to be able to generate BUILD files using gazelle. However, in a recent update, it stopped working.

When I run gazelle, I would expect the generated //my_prefix/foo/BUILD.bazel file to look something like:

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

However, deps for any modules in my WORKSPACE are not set. In other words, the dependency on "//my_prefix/bar:go_default_library" is not found. External dependencies are added correctly though.

Also, my_prefix is not a URL, it is just a string that is literally similar to "my_prefix".

@jayconrod
Copy link
Contributor

The direct cause of this is logic in Resolver.ResolveGo that decides whether an import path is local or not. It checks whether an import path equals the prefix or starts with the prefix followed by a slash. This is so that "foobar/baz" is not considered local under the prefix "foo". That doesn't work if your prefix is "" though.

#859 will fix this, but that's a ways off. I'll put together a fix today that should mitigate this.

@jayconrod
Copy link
Contributor

Sorry this isn't fixed yet. I'm at GothamGo, and that's taking most of my time.

One other complication with this is that Gazelle will view these import paths as being part of the standard library. It doesn't keep a full list of standard packages (we don't want to hard code that), so it uses a simple heuristic to check if something is standard: does not contain a dot, and not under the prefix.

A possible workaround for the short term: use a non-empty prefix. So importing "/my_repo/go_subtree/foo" would get you the label "//go_subtree/foo:go_default_library". Google does something like this with internal Go code.

@kgreenek
Copy link
Author

kgreenek commented Oct 6, 2017

Is there a recommended directory structure for a mono-repo? Is it best practice to push all go code under a top-level "//go" sub-folder?

It's kind of nice not having a go-prefix in the mono-repo because then non-go code can live with their associated service without being in a separate folder hierarchy. E.g. //go/my_service, //py/my_service, and //proto/my_service all just becomes //my_service, and the import paths in all languages are easy (for Go, python, and C++, at least). There may be drawbacks to doing this though in the long run.

We're just winging it! Not sure what the "correct" approach is.

@jayconrod
Copy link
Contributor

@kgreenek At the moment, there isn't a recommended approach because go_prefix and Gazelle's import-path-to-dependency translation breaks a lot of things that would be intuitive. Putting everything under //my_service seems very reasonable to me, as opposed to breaking things up by language.

We plan to deprecate go_prefix eventually (#721). Indexing import paths (#859) is a prerequisite for that, and I plan to work on that soon. Gazelle will still have some concept of prefix for the purpose of setting import paths on rules that don't specify one explicitly. However, I'd like Gazelle to handle empty prefixes and multiple prefixes in different subtrees.

jayconrod pushed a commit to jayconrod/rules_go that referenced this issue Oct 13, 2017
Gazelle now allows go_prefix to be empty. It must still be set either
on the command line or in a go_prefix rule. When the prefix is empty,
all imports not in the standard library are resolved
locally. Dependencies will not point to the vendor directory or to
external repositories.

* Gazelle now hard-codes a list of standard packages from the SDK that
  was used to build it.
* All imports (including standard imports) are now added to
  packages.GoTarget. The resolve package filters out standard
  libraries.

Fixes bazel-contrib#870
jayconrod added a commit that referenced this issue Oct 13, 2017
Gazelle now allows go_prefix to be empty. It must still be set either
on the command line or in a go_prefix rule. When the prefix is empty,
all imports not in the standard library are resolved
locally. Dependencies will not point to the vendor directory or to
external repositories.

* Gazelle now hard-codes a list of standard packages from the SDK that
  was used to build it.
* All imports (including standard imports) are now added to
  packages.GoTarget. The resolve package filters out standard
  libraries.

Fixes #870
skyl added a commit to skyl/pango that referenced this issue Jun 11, 2018
…weird stuff with the paths - manually editing the build files makes both go cmdline tool and bazel work with the import paths I want ... will revisit, see bazel-contrib/rules_go#870
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants