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

Upgrading to gazelle v0.18.1 requires a root BUILD file #609

Closed
nlopezgi opened this issue Aug 1, 2019 · 12 comments
Closed

Upgrading to gazelle v0.18.1 requires a root BUILD file #609

nlopezgi opened this issue Aug 1, 2019 · 12 comments

Comments

@nlopezgi
Copy link

nlopezgi commented Aug 1, 2019

Hi,
Were updating our dependency on bazel-gazelle from rules_docker in this PR.
The update is failing as one of the projects we test with on CI does not have a BUILD file at the same level as the WORKSPACE file. .
The error is as follows:

ERROR: .../.cache/bazel/_bazel_ngiraldo/c10a80dcc9221dc198fec52968bd1a2f/external/io_bazel_rules_docker/container/go/cmd/digester/BUILD:19:1: @io_bazel_rules_docker//container/go/cmd/digester:go_default_library depends on @com_github_pkg_errors//:go_default_library in repository @com_github_pkg_errors which failed to fetch. no such package '@com_github_pkg_errors//': Unable to load package for //:WORKSPACE: BUILD file not found in any of the following directories.
 - /.../rules_docker/rules_docker_fork/testing/java_image_no_root

Note the bit: Unable to load package for //:WORKSPACE: BUILD file not found in any of the following directories.

I am able to fix this error by simply adding an empty BUILD file at the same level as the WORKSPACE file.

I think that this PR is the one that added the dependency on //:WORKSPACE.

While having this empty BUILD file is not problematic, the error messaging is quite confusing.
Sample PR with failing test: bazelbuild/rules_docker#1013
Sample PR with fix that adds empty BUILD file: bazelbuild/rules_docker#1045

@jayconrod
Copy link
Contributor

This is admittedly a really confusing error message. Unfortunately, I can't think of a way to work around this without removing the whole feature.

The issue is that go_repository now reads a configuration file, which, by default is WORKSPACE in the main workspace. This helps Gazelle resolve dependencies in external repos (WORKSPACE is treated as a map from Go import path prefixes to repository names). It's no longer necessary for Gazelle to go out to the network to resolve these at build time, which was slow and error-prone.

The default configuration file is @//:WORKSPACE. It can be set explicitly as the build_config attribute of go_repository. We resolve the label using repository_ctx.path. That function simply fails with this message if it can't resolve the label. We can't handle the failure or improve the message in Starlark.

Maybe we could require users to set the label explicitly, opting into the new feature? I'm guessing most people won't though, so I'd prefer to keep this behavior the default if possible. Not having a root build file seems like a relatively rare case.

@nlopezgi
Copy link
Author

nlopezgi commented Sep 6, 2019

sgtm, if its not easy to resolve then lets close. Thanks for the clarifications!

@nlopezgi
Copy link
Author

this is causing a bit of friction as now all dependent projects need to add a BUILD file. A better error message would be awesome as it did take a couple of minutes for my head to jolt and remember the error message corresponded to a missing build file. We're updating our README in rules_docker to make this explicit, but I can imagine other repos will run into the same kind of friction.
Example issues:
bazelbuild/rules_docker#1139
bazelbuild/rules_docker#1126

@laurentlb Do you think there is a better way to do this with Starlark so that the error message is clearer?

@nlopezgi nlopezgi reopened this Sep 10, 2019
@laurentlb
Copy link
Contributor

Can you file a bug? We can look at it.

@nlopezgi
Copy link
Author

created bazelbuild/bazel#9364, thanks! Closing here.

@jayconrod
Copy link
Contributor

@laurentlb If there were a way for a repository rule to resolve a file in the root of another workspace without requiring a build file to be present, that would avoid the error entirely. I don't think build files are completely loaded at the point where these repository rules are evaluated, so the content of the build files shouldn't matter.

Is that a change that could be made in Bazel? I'd be happy to file an issue if it would be useful.

@laurentlb
Copy link
Contributor

Just to clarify, you use the label @//:WORKSPACE, hence it requires a BUILD file?

Reading the documentation of repository_ctx.path, it says it can take a relative path argument. Does it work if you use just WORKSPACE?

@jayconrod
Copy link
Contributor

It needs to read the WORKSPACE file in the main workspace though, not the repository's own WORKSPACE file.

We have an internal repository rule, go_repository_config, which is used to synthesize the bazel_gazelle_go_repository_config repo. It reads the main WORKSPACE file (unless another file is explicitly specified) and produces a configuration file, which is referenced by other repository rules. This is mainly used to map Go module names to repository names. That speeds up dependency resolution, since we don't need to go out to the network to look up module roots in most cases.

@laurentlb
Copy link
Contributor

laurentlb commented Sep 10, 2019

cc @aehlig, any idea?

@//:WORKSPACE doesn't behave like a real label, e.g. it doesn't respect visibility (no need for exports_files here). I wonder if we really need the top-level BUILD file here.

@aehlig
Copy link

aehlig commented Sep 12, 2019

cc @aehlig, any idea?

@//:WORKSPACE doesn't behave like a real label, e.g. it doesn't respect visibility (no need for exports_files here). I wonder if we really need the top-level BUILD file here.

@laurentlb To be honest, I'm a bit surprised by that question, given that you recently handled an incompatible change that forces loads to be even more aligned with package boundaries. In other words, this is not a technical question, but more a question of what we want; if we want labels for loading files to be aligned with package boundaries, then this is working as intended; if we don't want to insist on this property, then why did we even do the quoted incompatible change?

@vymao
Copy link

vymao commented Jul 5, 2022

Has this been resolved? I think I'm running into the same issue now on a recent version of bazel-gazelle.

@mollyibot
Copy link

I also run into the same issue, any updates?

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

No branches or pull requests

6 participants