remote strategy doesn't incorporate runfiles as inputs

[dfabulich]

Consider this repository https://github.com/dfabulich/remote-cache-key-runfiles

The repository has an empty WORKSPACE and a trivial sh_binary, cp.sh, designed to copy its runfile (called runfile) to a parameterized destination.

cp runfile $1

The runfile is just a text file containing the number 0.

The BUILD file declares cp.sh as a sh_binary with the runfile in its data, and declares a genrule called x that runs cp.sh to generate an output file, out.txt.

sh_binary(
    name='cp',
    srcs=['cp.sh'],
    data=['runfile'],
)

genrule(
    name='x',
    tools=['cp'],
    outs=['out.txt'],
    cmd='$(location cp) $@',
)

If you run bazel build :x && more bazel-genfiles/out.txt you'll see that it has copied the runfile to bazel-genfiles/out.txt and the output is 0. If you change the runfile to contain the number 1 the out.txt file will change when you re-run the build as expected.

Now set the runfile back to 0, launch a Hazelcast server, and run the build like this:

bazel clean && bazel build --hazelcast_node=localhost:5701 \
  --genrule_strategy=remote --spawn_strategy=remote :x && more bazel-genfiles/out.txt

You'll correctly see the output is 0. Now change the runfile contents to 1 and run the build again.

Expected: When the runfile contents change, the cache should be invalidated and the output should be regenerated as 1
Actual: The old cached output is reused and the output remains 0

full console output

dfab: /tp/remote-cache-key-runfiles $ bazel clean && bazel build --hazelcast_node=localhost:5701 --genrule_strategy=remote --spawn_strategy=remote :x && more bazel-genfiles/out.txt
.
INFO: Starting clean (this may take a while). Consider using --expunge_async if the clean takes more than several minutes.
INFO: Found 1 target...
Target //:x up-to-date:
  bazel-genfiles/out.txt
INFO: Elapsed time: 2.304s, Critical Path: 0.07s
0
dfab: /tp/remote-cache-key-runfiles $ echo 1 > runfile
dfab: /tp/remote-cache-key-runfiles $ bazel clean && bazel build --hazelcast_node=localhost:5701 --genrule_strategy=remote --spawn_strategy=remote :x && more bazel-genfiles/out.txt
INFO: Starting clean (this may take a while). Consider using --expunge_async if the clean takes more than several minutes.
INFO: Found 1 target...
Target //:x up-to-date:
  bazel-genfiles/out.txt
INFO: Elapsed time: 1.622s, Critical Path: 0.02s
0
dfab: /tp/remote-cache-key-runfiles $ bazel version
Build label: 0.3.1
Build target: bazel-out/local-fastbuild/bin/src/main/java/com/google/devtools/build/lib/bazel/BazelServer_deploy.jar
Build time: Fri Jul 29 09:05:18 2016 (1469783118)
Build timestamp: 1469783118
Build timestamp as int: 1469783118

[dfabulich]

Inspecting in a debugger, the runfiles are attached to the action as a "middleman" artifact. RemoteSpawnStrategy dutifully calls ActionInputHelper.expandArtifacts to expand the runfiles, but the actionExecutionContext's ArtifactExpanderImpl has an empty expandedInputs map; thus the middleman's inputs are not expanded and are omitted from the cache key.

This, in turn, is happening because ActionExecutionFunction.checkInputs, which (I think) is responsible for populating the expandedInputs, doesn't know how to handle middleman artifacts. It treats middleman artifacts as ordinary FileArtifactValues.

[dfabulich]

Looks like there was an incriminating TODO in 0.3.1

bazel/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java

Line 578 in 740b768

// TODO(bazel-team): Make sure middleman "virtual" artifact data is properly processed.

// TODO(bazel-team): Make sure middleman "virtual" artifact data is properly processed.

That TODO was eliminated but apparently not addressed in ad77f97 because this test fails on the latest master, too.

[dfabulich]

Wait a minute. We're just computing a hash; we don't need to expand the middlemen. We can use their original value. I'll pull together a PR

[dfabulich]

https://bazel-review.googlesource.com/4230

[meteorcloudy]

//cc @dslomov Can you review this change?

[ulfjack]

Closing because there's a commit that references this bug. Please reopen if this isn't fixed.

[benjaminp]

I don't think this is fixed. I can't find the commit that references this bug. Moreover, the latest master still expands the input file artifacts before computing the cumulative input digest and consequently fails to consider the runfiles middleman.

[ulfjack]

The commit is linked just above my comment, hash 6734f43.

[benjaminp]

That's a fork of Bazel, not the canonical repo.

…

On Tue, Jan 17, 2017, at 07:56, Ulf Adams wrote: The commit is linked just above my comment, hash 6734f43. -- You are receiving this because you commented. Reply to this email directly or view it on GitHub: #1593 (comment)

[ulfjack]

Doh!

[dslomov]

@ulfjack is this the right category?

[ulfjack]

Let me put it under service APIs, which is marginally better.

[ulfjack]

I have some changes in flight to prepare for open sourcing the code we're using internally to handle runfiles.