Bazel: proto_library: warning: directory does not exist

Created on 17 Jan 2019 · 29Comments · Source: bazelbuild/bazel

Description of the problem / feature request:

Building proto_library targets for Well Known Types in @com_google_protobuf emits warnings like the one below. The build succeeds, but it would be nice if the warning can be fixed or removed.

$ bazel build @com_google_protobuf//:any_proto
...
INFO: From Generating Descriptor Set proto_library @com_google_protobuf//:any_proto:
external/com_google_protobuf: warning: directory does not exist.
...

Bugs: what's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

# WORKSPACE
load("@bazel_tools//tools/build_defs/repo:git.bzl", "git_repository")

git_repository(
    name = "com_google_protobuf",
    remote = "https://github.com/protocolbuffers/protobuf",
    tag = "v3.6.1.3",
)

git_repository(
    name = "bazel_skylib",
    remote = "https://github.com/bazelbuild/bazel-skylib",
    tag = "0.6.0",
)

bazel build @com_google_protobuf//:any_proto

What operating system are you running Bazel on?

Linux amd64

What's the output of `bazel info release`?

release 0.21.0

This does not happen in 0.20.0, but it does happen in 0.22.0rc2.

P2 team-Rules-Server

Source

jayconrod

👍12

Most helpful comment

@joshburkart

Can confirm the error still exists in Bazel 3.x.x

MegaPanchamZ on 27 Sep 2020

👍3

All 29 comments

@lberki I wonder if this is related to recent source root changes?

jayconrod on 17 Jan 2019

/cc @haberman

Probably, I suppose? We did reshuffle the way protoc is called a bit. I'm traveling this week and next, though, so I probably won't be able to look at it closely until I get back home.

@dbabkin , can you help out?

lberki on 21 Jan 2019

Friendly ping. This still happens in 0.22.0.

thaidn on 3 Mar 2019

Hi,
There is no ETA when that will be fixed.
Priority of this task is low, as it is warning only.

dbabkin on 8 Mar 2019

Small ping, still happening in 0.25.3

steeve on 6 Jun 2019

Ping again, this is extremely annoying and spammy.

dapirian on 28 Jun 2019

👍2

Update: I have a change under review that will fix this issue. Let's see how the review goes. It's not complicated, so it shouldn't be problematic.

lberki on 2 Jul 2019

👍3

...and apologies for the delay. I know it was very annoying (in fact, the reason why I fixed it was because it annoyed me! :) )

lberki on 3 Jul 2019

Well, that was optimistic:

https://buildkite.com/bazel/bazel-at-head-plus-downstream/builds/1067#24a01f98-1ed2-4423-8d94-bf11cef557e4

The change will have to be rolled back. I'll take a look the next time I have some spare time.

lberki on 3 Jul 2019

Ugh. This is awful. Hear me out.

This warning messages happen when all the source files of a proto_library in an external repository are generated and therefore --proto_path=<root of external repository> is superfluous. However, the list of proto source roots comes from the same place that we use to determine the prefix we should cut off the exec path to obtain the proto import path.

Of course, we have to cut off different parts depending on whether the proto file is source or generated, but that we determine by... guesswork in guessProtoPathUnderRoot:

https://github.com/bazelbuild/bazel/blob/master/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoCompileActionBuilder.java#L659

That algorithm broke when we removed the directory from the set of proto paths. The reason why only Go broke is that this line trying to cater for legacy use cases saves us, but I don't want to rely on it:

https://github.com/bazelbuild/bazel/blob/master/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoCompileActionBuilder.java#L671

, but go_proto_library sensibly doesn't have its equivalent.

So that change was indeed wrong. Now, the question is, how to make it right -- eliminating the warning requires that we remove a repository root from the set of proto paths if there are only generated files, but if we remove it, the algorithm to generate -I clauses suddenly stops working.

Furthermore, it's not trivial to decouple --proto_path from -I because they are both derived from ProtoInfo, which is exposed to Starlark, so we can't just change it willy-nilly. In particular, it only has room for one direct proto source root, which is not enough: the principled fix is to make it so that ProtoInfo.proto_source_root is an execpath and not a root-relative path and if a proto_library has both generated and source .proto files, one would require both.

A saving grace is that ProtoInfo cannot currently be created from Starlark, so we may get away with adding a Java field that's not exposed to Starlark and leaving ProtoInfo.proto_source_root as a polite lie.

However, it's far from a trivial change :(

lberki on 3 Jul 2019

Shouldn't we remove .proto_source_root by https://github.com/bazelbuild/bazel/issues/7153?

hlopko on 3 Jul 2019

This is not about the attribute, but the eponymous field of ProtoInfo.

lberki on 3 Jul 2019

Yup, what I'm asking is if the field makes sense when the attribute is gone.

hlopko on 3 Jul 2019

It does. That field tells what the _effective_ proto source root is. That's a concept that makes sense even when the attribute will be gone.

lberki on 3 Jul 2019

So, I got a bit nerd sniped and I have another solution: simply create a virtual proto import directory when the proto_library rule has a generated .proto file just like we do when strip_import_prefix or import_prefix is set.

Granted, it's not _that_ simple because we still need to make sure that our internal repository keeps chugging along, but I'd gladly fork the behavior of proto_library in this respect (they are different already, so no big change) if that's what I have to do to make proto_library saner for everyone else.

That's _still_ not a tiny amount of work and it is _still_ possible that some odd downstream project breaks, but this is certainly much simpler than the previous alternative. And it doesn't even require lying to Starlark!

lberki on 3 Jul 2019

Update: I have another fix. Learning from the previous one, I have to run in through all sorts of presubmits to make sure that it doesn't break anything. It introduces a slight incompatibility which I hope no one relies on, but there is only one way to be sure...

If that still doesn't work, I have another option (not generating virtual import directories for rules with proto_source_root= set, only for those with generated sources), which might still fail, but with a much lower probability. The downstream presubmit will tell.

lberki on 8 Jul 2019

Update: collateral damage is minimal. Unfortunately, it includes rules_rust and Bazel itself.

I'll send out changes to both then try to submit this.

lberki on 8 Jul 2019

/cc @scentini

Update: this will have to be rolled back due to some obscure semantic differences within Google.

The fix is trivial and expect it to land tomorrow, but I'd much rather not cowboy code up a forward fix and instead work without time presure.

lberki on 9 Jul 2019

...and there is also https://github.com/bazelbuild/bazel/issues/8832, but that's fixed by https://github.com/bazelbuild/bazel-buildfarm/pull/272 . Once that's submitted and I can find some breathing room, I'll try to roll this forward again.

lberki on 9 Jul 2019

Update: I have a third fix. To verify, I:

Fixed the Google-internal usage and checked that everything that was broken is now OK
Updated the fix to grpc-java (https://github.com/grpc/grpc-java/pull/5967)
Updated the patch to grpc-java in buildfarm (https://github.com/bazelbuild/bazel-buildfarm/pull/275)
Ran a Bazel global presubmit (https://buildkite.com/bazel/bazel-at-head-plus-downstream/builds/1084)
Verified that bazel-buildfarm works with all four combinations of (last Bazel release, Bazel at HEAD + this fix) x (bazel-buildfarm at HEAD, bazel-buildfarm with the above pull request) (no link, I did it manually)

Third time is the charm!

lberki on 10 Jul 2019

🎉2

Has this been fixed in bazel 0.28.1 or do we have to wait for 0.29.0?