search

bazelbuild / bazel

a fast, scalable, multi-language and extensible build system
https://bazel.build
Apache License 2.0
23.21k stars 4.06k forks source link

Remote caching/execution fails to handle source directories in a runfiles tree #11495

Open illicitonion opened 4 years ago

illicitonion commented 4 years ago

Description of the problem / feature request:

Trying to use $(location) of a directory causes a crash in merkle tree building if using remote execution, but works in local execution.

Either this should be allowed in both contexts (and work in both contexts), or it should be actively disallowed in both contexts.

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

See trivial repro at https://github.com/illicitonion/repro-bazel-dir-runfile

What's the output of bazel info release?

release 3.1.0

Any other information, logs, or outputs that you want to share?

If you swallow the exception, the action runs (yay no server crash!), but the contents of the directory (which were included by the other dependency, the filegroup) are absent.

illicitonion commented 4 years ago

I debugged a little, as far as I can tell Runfiles.filterListForObscuringSymlinks filters out the children, and then the MerkleTree builder uses this as the source of truth for what files are necessary.

A simple fix could be to just swap the logic here; keep the most nested rather than least nested things, but I don't know what negative impact that could have. Otherwise, we should probably keep around information about both the original and modified manifest until a point that we know which are directories, and then make a smarter decision when constructing the MerkleTree.

cameron-martin commented 2 years ago

Is having directory dependencies supported? I can't find much information about this.

If it is supported, what is the behaviour? Does it hash the contents of the directory to determine if it has changed?

illicitonion commented 2 years ago

Is having directory dependencies supported? I can't find much information about this.

In a non-remote-execution world, that dependency on a directory causes the empty directory to be added to the sandbox. If you want the contents, you'd need to depend on a filegroup or similar which globs (or otherwise references) the contents.

I would assume remote execution should work similarly.

cameron-martin commented 2 years ago

That's not what I'm seeing for runfile directory dependencies during local execution of tests, which I believe should use the same sandboxing as builds. The whole directory gets symlinked into the sandbox in this case.

fmeum commented 2 years ago

Tracking of source directories is unsound without the BAZEL_TRACK_SOURCE_DIRECTORIES JVM property enabled via startup flags. The recommended alternative is to use globs.

cameron-martin commented 2 years ago

@fmeum Thanks for the info.

Do you know of any proposals to fix this unsound behaviour?

fmeum commented 2 years ago

There is a lot of context on https://github.com/bazelbuild/bazel/pull/15774, but no issue yet - you could create one to track this.

tjgq commented 1 year ago

@coeuvre and I looked into this. It's not actually related to $(location); all that's required to repro it is an action consuming a runfiles tree containing a source directory. The bug (well - at least part of it) is that when the directory contents are expanded into the Merkle tree, we don't rebase the paths under the runfiles directory (somewhere around here: https://cs.opensource.google/bazel/bazel/+/master:src/main/java/com/google/devtools/build/lib/remote/merkletree/DirectoryTreeBuilder.java;l=206;drc=24de276977b44c6135813ced5e113c36da7e9953)