Repository: bazelbuild/bazel
Issue: Error messaging when BUILD file not present at top level directory is unclear
Labels: P1
more data needed
team-ExternalDeps
untriaged
bad error messaging

### Description of the problem / feature request:

`go_repository` now reads a configuration file, which, by default is WORKSPACE in the main workspace. 
The implementation of this feature is such that the error messaging when the file is not present is as follows: 
```
ERROR: Analysis of target '//src/hack/project:image' failed; build aborted: no such package '@com_github_google_go_containerregistry//pkg/v1': Unable to load package for //:WORKSPACE: BUILD file not found in any of the following directories.
 - /local/path/to/my/git/repository
```
This error message is already causing issues for rules_docker customers, as now this BUILD file is needed in all projects that depend transitively on gazelle. 

Please see https://github.com/bazelbuild/bazel-gazelle/issues/609#issuecomment-530017922

Also related: 
bazelbuild/rules_docker#1139
bazelbuild/rules_docker#1126

### Feature requests: what underlying problem are you trying to solve with this feature?

I would like to have better messaging for this type of errors.

### Bugs: what's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

Depend on gazelle (latest version) from a project that does not have a root BUILD file

fyi @laurentlb 


Issue comments:
What message would you suggest?
Just to clarify @meisterT's question.

The [quoted issue](https://github.com/bazelbuild/bazel-gazelle/issues/609)

> Please see [bazelbuild/bazel-gazelle#609 (comment)](https://github.com/bazelbuild/bazel-gazelle/issues/609#issuecomment-530017922)

[states](https://github.com/bazelbuild/bazel-gazelle/issues/609#issuecomment-529958394)

> 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. 

However, the error message literally states `BUILD file not found`, so I wonder what additional information you would like to have seen.
`BUILD file not found at the top level directory`
would be a good start

Note error message currently includes:
`Analysis of target '//src/hack/project:image' failed` 
and
`failed; build aborted: no such package '@com_github_google_go_containerregistry//pkg/v1'`
and
`Unable to load package for //:WORKSPACE:`

So user has to work out that the two first references above are misleading, and the latter one
`Unable to load package for //:WORKSPACE:` refers to not having a BUILD file at that level.

As evidenced by 2 issues opened by different users in `rules_docker`, I'm not the only one who feels this message is not informative enough.
bazelbuild/rules_docker#1139
bazelbuild/rules_docker#1126

Other error traces from those issues:
```
ERROR: Analysis of target '//dockertest:dockertest' failed; build aborted: no such package '@com_github_pkg_errors//': Unable to load package for //:WORKSPACE: BUILD file not found in any of the following directories.
 - /Users/jeffhodges/src/github.com/jmhodges/bazel_bugs
```
and

```
ERROR: @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
```

I'm sure many would be able to figure out instantly that this means that the BUILD file is missing at the top level, but several others wont.
> `BUILD file not found at the top level directory`
> would be a good start

Such a message without context would give the wrong impression that there were a requirement to have a `BUILD` file next next to the `WORKSPACE` file, which, however, is not a requirement. It is perfectly fine to not have a package for the empty string, as long as no one references the empty string as a package name. In that respect, "top level" is no different than any other package name: only those packages referenced somewhere have to exist.

Therefore, I also wouldn't call ...

> So user has to work out that the two first references above are misleading,

... the first two references misleading; instead they give the needed context on where the offending reference was found (keep in mind that `bazel` has no means of knowing whether your source layout does or does not intend to have a package with name `''`). It simply follows the standard pattern of all `bazel` error messages that a lot of users find useful.
- First it states which of targets `bazel` was asked to build failed; this information is not redundant, as the command line accepts target patters that can, in general, expand to several targets.
- Then it mentions the root cause, in this case a package depended upon that does not exist.
- Finally, it gives information on the root cause. In this case, why that package does not exist: it has a definition, but that definition depends on an invalid label `//:WORKSPACE`.
- It also explains why the label `//:WORKSPACE` is invalid: that label requires the package `''` to exist, but it found no `BUILD` file for that package.
- Finally, `bazel` even lists the candidate directories where a `BUILD` file could be located in order for the package `''` to exist.

This whole chain of reasons is useful to understand why `bazel` even started to look for that package (in this case with the empty string as name).

So I still don't see any actionable suggestion on how to improve this already very detailed error message.
Agreed, thanks anyway
Something that'll help here is an additional educational message along the lines of 

"Unable to load package for //:WORKSPACE. **A BUILD file is required to mark a directory as a Bazel package**. BUILD file not found in any of the following directories.."

Many users understand what packages and targets are, but have no idea BUILD files are use to mark package boundaries. I certainly took a while to learn where to place `:` in target strings.. related https://github.com/bazelbuild/bazel/issues/7834

Made a PR: https://github.com/bazelbuild/bazel/pull/9385

@nlopezgi wdyt?
Hi @jin,
that does look to me like it would be a bit easier to understand,
Thanks for sending that PR
> Just to clarify @meisterT's question.
> 
> The [quoted issue](https://github.com/bazelbuild/bazel-gazelle/issues/609)
> 
> > Please see [bazelbuild/bazel-gazelle#609 (comment)](https://github.com/bazelbuild/bazel-gazelle/issues/609#issuecomment-530017922)
> 
> [states](https://github.com/bazelbuild/bazel-gazelle/issues/609#issuecomment-529958394)
> 
> > 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.
> 
> However, the error message literally states `BUILD file not found`, so I wonder what additional information you would like to have seen.

@aehlig error messages, such as "BUILD file not found...", lack a "call to action". People who are new to Bazel will need to spend more effort to get past the error. This happened to me too (I am new to Bazel)!

In contrast, this message is friendly and makes the most of the end-user's time:

> Add a BUILD file to *DIRECTORY*, to mark it as a Bazel package.

I think that actionable error messages help everyone benefit from using Bazel.
@nicoaragon I have an incoming change incorporating your suggestion. Thanks.

> In contrast, this message is friendly and makes the most of the end-user's time:
> 
> > Add a BUILD file to _DIRECTORY_, to mark it as a Bazel package.

It, however, it not in all cases the right thing to do. Adding a `BUILD` file splits the package into which the `BUILD` file is added, thus rendering other labels incorrect that used to be correct before the addition of the `BUILD` file.

@aehlig 

I think you care for accuracy. Also, you want to prevent people breaking their build.

I care for end user experience and I want to empower Bazel users to be comfortable changing, and evolving their build. That includes breaking and fixing the build.

In this case, I would be happy to make the change and break other labels, as long as Bazel lets me know that I did.

What do you use to measure end user experience?

Pull request review comments:

Patch that resolved the issue:
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PackageLookupFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PackageLookupFunction.java
index 080d1bd392eff1..bafd17aae901f6 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/PackageLookupFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/PackageLookupFunction.java
@@ -113,10 +113,12 @@ public SkyValue compute(SkyKey skyKey, Environment env)
    */
   public static String explainNoBuildFileValue(PackageIdentifier packageKey, Environment env)
       throws InterruptedException {
+    String educationalMessage = "A BUILD file marks a directory as a Bazel package. However, ";
     if (packageKey.getRepository().isMain()) {
       PathPackageLocator pkgLocator = PrecomputedValue.PATH_PACKAGE_LOCATOR.get(env);
       StringBuilder message = new StringBuilder();
-      message.append("BUILD file not found in any of the following directories.");
+      message.append(educationalMessage);
+      message.append("there are no BUILD files found in any of the following directories:");
       for (Root root : pkgLocator.getPathEntries()) {
         message
             .append("\n - ")
@@ -124,7 +126,8 @@ public static String explainNoBuildFileValue(PackageIdentifier packageKey, Envir
       }
       return message.toString();
     } else {
-      return "BUILD file not found in directory '"
+      return educationalMessage
+          + "there is no BUILD file found in the directory '"
           + packageKey.getPackageFragment()
           + "' of external repository "
           + packageKey.getRepository();


Test patch:
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/AnalysisFailureReportingTest.java b/src/test/java/com/google/devtools/build/lib/analysis/AnalysisFailureReportingTest.java
index 6cf030fb119a85..3c81a15c1a626d 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/AnalysisFailureReportingTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/AnalysisFailureReportingTest.java
@@ -92,7 +92,8 @@ public void testMissingDependency() throws Exception {
         .containsExactly(
             new LoadingFailedCause(
                 causeLabel,
-                "no such package 'bar': BUILD file not found in any of the following directories.\n"
+                "no such package 'bar': A BUILD file marks a directory as a Bazel package. "
+                    + "However, there are no BUILD files in any of the following directories:\n"
                     + " - /workspace/bar"));
   }