Distributed caching: build fails when build inputs include directories

[dfabulich]

Consider this repository https://github.com/dfabulich/hazelcast-input-dir

In its WORKSPACE, it declares a new_http_archive whose output includes directories:

new_http_archive(
    name='babel_6_5_2',
    url='https://registry.npmjs.org/babel/-/babel-6.5.2.tgz',
    build_file_content="filegroup(name='raw', srcs=glob(['*'], exclude_directories=0), visibility=['//visibility:public'])",
)

Its BUILD file includes a simple genrule that depends on those files.

genrule(
    name='x',
    srcs=['@babel_6_5_2//:raw'],
    outs=['x.txt'],
    cmd='echo $(locations @babel_6_5_2//:raw) > $@',
)

Finally, its tools/bazel.rc uses a local hazelcast node.

build --hazelcast_node=127.0.0.1:5701 --genrule_strategy=remote --spawn_strategy=remote

When you bazel clean && bazel build :x the build fails:

$ bazel clean && bazel build :x
INFO: Starting clean (this may take a while). Consider using --expunge_async if the clean takes more than several minutes.
INFO: Found 1 target...
WARNING: /tmp/dgf/hazelcast-input-dir/BUILD:1:1: input 'external/babel_6_5_2/package' to //:x is a directory; dependency checking of directories is unsound.
ERROR: /tmp/dgf/hazelcast-input-dir/BUILD:1:1: Executing genrule //:x failed: Failed to get digest for input.: Input is a directory: external/babel_6_5_2/package.
Target //:x failed to build
Use --verbose_failures to see the command lines of failed build steps.
INFO: Elapsed time: 1.176s, Critical Path: 0.00s

Whereas if you bazel clean && bazel build --genrule_strategy= :x to disable distributed caching, the build succeeds with a warning.

$ bazel clean && bazel build --genrule_strategy= :x
INFO: Starting clean (this may take a while). Consider using --expunge_async if the clean takes more than several minutes.
INFO: Found 1 target...
WARNING: /tmp/dgf/hazelcast-input-dir/BUILD:1:1: input 'external/babel_6_5_2/package' to //:x is a directory; dependency checking of directories is unsound.
Target //:x up-to-date:
  bazel-genfiles/x.txt
INFO: Elapsed time: 1.249s, Critical Path: 0.04s

[dfabulich]

@hhclam

[hermione521]

Also cc @philwo

[philwo]

I think this is "working as intended" - Bazel is not designed to handle directories as input artifacts and much of the code assumes that artifacts are files that can be read, checksummed, transferred, ...

I agree it would be cool if we could support directories as first-class objects, though. The general workaround is to use the contents of the directories as individual files - does that work for you?

e.g. by replacing this:
build_file_content="filegroup(name='raw', srcs=glob(['*'], exclude_directories=0), visibility=['//visibility:public'])",
with
build_file_content="filegroup(name='raw', srcs=glob(['**']), visibility=['//visibility:public'])",

[dfabulich]

No, I can't do that, because of issue #374, under which there are restrictions on the characters that are allowed in input file names. For example, this fails:

new_http_archive(
    name='ua_parser_js_0_7_10',
    url='https://registry.npmjs.org/ua-parser-js/-/ua-parser-js-0.7.10.tgz',
    build_file_content="filegroup(name='raw', srcs=glob(['**']), visibility=['//visibility:public'])",
)

ERROR: /private/var/tmp/_bazel_dfabulich/a99be41be8573ba0e57311bd0155f47e/external/ua_parser_js_0_7_10/BUILD:1:1:
@ua_parser_js_0_7_10//:raw: invalid label 'package/test/browser&mediaplayer-test.json'
in element 12 of attribute 'srcs' in 'filegroup' rule: invalid target name 'package/test/browser
&mediaplayer-test.json': target names may not contain '&'.

The only workaround is to use exclude_directories=0 to include the directories.

http://www.bazel.io/docs/build-ref.html

Unfortunately, there are some scenarios where directory labels must be used. For example, if the testdata directory contains files whose names do not conform to the strict label syntax (e.g. they contain certain punctuation symbols), then explicit enumeration of files, or use of the glob() function will produce an invalid labels error. You must use directory labels in this case, but beware of the concomitant risk of incorrect rebuilds described above.

[ahumesky]

As a workaround, you could replace the glob with something like this:

srcs = [f for f in glob(["**"]) if "&" not in f]

[dslomov]

@philwo, any more information we need here?

[philwo]

@dslomov No, this is a known issue, we know it's important and what needs to be done, but AFAIK unfortunately no one is actively working on this, because everyone is busy with other stuff :(

I think we should create a P1 bug / feature request about "relax label syntax validation" (if there isn't one already) and then mark this as a duplicate of it.

[hhclam]

philwo: Do you mean it's a known issue with directories or label syntax?

Is there a tracking issue for better directories support?

[philwo]

@hhclam Sorry, I meant it's a known issue that the label syntax is too strict.

In a way, it's also a known issue that our support for directories in general is bad - I wouldn't be surprised if sandboxing breaks, remote caching breaks (because how Bazel has no digest for directories), remote execution breaks (because how would you transfer a directory in the protocol?), ...

I remember someone on our team (sorry - I initially pinged the wrong Cory here :)) once making a proposal for how we could improve support for directories, but I'm not sure where that went.

I personally think that it would be cool if we had better support for directories, but I don't see it happening. It's more probable that we eventually fix the label syntax, so that Bazel can address individual files and the need for using directories goes away.

Pinging @ulfjack for a more senior perspective on this whole topic.

[hhclam]

So it sounds like despite the issue with label syntax validation, directories should be better supported with remote caching and execution. This is a valid bug that I should fix.

[ulfjack]

@damienmg just changed labels to allow ' ', '(', ')' and '$'. Maybe we can add '&' to that list as well?

Other than that, I'm still not sure what to do about directory inputs. RemoteSpawnStrategy certainly does not check whether input files are directories and does not upload directories. This is good for correctness - we can't track directories at that level. If we want to support directories, we'd need to expand them at a higher level conceptually.

[damienmg]

'&' is not problematic for any case (except for unquoted shell), we can add it right away.

[ulfjack]

I'm actually not sure what the current state is wrt. directories, since I made some changes recently. I think we're throwing an error if there's a directory input, or maybe it's a warning?

[ulfjack]

In either case, I've prepared a change to allow even more characters in labels, which will hopefully reduce the need to have directory inputs.

[ulfjack]

If I add a directory to data of a test, I get a different error:

INFO: Analysed target //a:test.
INFO: Found 1 test target...
FAIL: //a:test (see /usr/local/google/home/ulfjack/.cache/bazel/_bazel_ulfjack/5c706281d73fa9baac5a257912930c73/execroot/io_bazel/bazel-out/local-fastbuild/testlogs/a/test/test.log)
ERROR: /usr/local/google/home/ulfjack/Projects/bazel/a/BUILD:2:1: output 'a/test/test.log' was not created
ERROR: /usr/local/google/home/ulfjack/Projects/bazel/a/BUILD:2:1: not all outputs were created or valid
Target //a:test up-to-date:
  bazel-bin/a/test
INFO: Elapsed time: 0.225s, Critical Path: 0.01s
FAILED: Build did NOT complete successfully
//a:test                                                                 FAILED in 0.0s

Executed 1 out of 1 test: 1 fails locally.

[buchgr]

This has been fixed in the meantime.

[mboes]

@buchgr to clarify - what has been fixed? The label issue or the original issue, i.e. making directory inputs work with distributed builds. I'm having a hard time finding this in the commit history.

[buchgr]

@mboes making directory inputs work with distributed builds. Are you seeing something else?
We discourage using this feature though i.e. don't use genrules like this

genrule(
  cmd = "mkdir foo",
  outs = ["foo"],
)

or filegroups including a directory instead of globbing it. There are early plans of properly supporting this in Bazel by introducing a "dir/" syntax to declare that a genrule output is a directory

genrule(
  cmd = "mkdir foo",
  outs = ["foo/"],
)

[djmarcin]

@buchgr Has there been movement on this plan, or is the best way to go to declare a custom rule that uses declare_directory?

(found this due to WARNING: Ouput ... is a directory; dependency checking of directories is unsound)

[alexeagle]

https://docs.aspect.build/rules/aspect_bazel_lib/docs/run_binary/
is a genrule-style rule that knows how to output a proper Bazel directory artifact.