genrule: cmd doesn't run even though dependency was correctly rebuilt

[davido]

Consider the following rule:

genrule(
  name = 'broken',
  cmd = ' '.join(['$(exe //tools:pack_me)', '-o', '$OUT']),
  deps = ['//:plugin-lib'],
  out = 'broken.out',
)

Where plugin-lib is:

java_library(
  name = 'plugin-lib',
  srcs = glob(['java/**/*.java']),
  provided_deps = ['//:commons-io'],
)

and //tools:pack_me in tools/BUCK:

python_binary(
  name = 'pack_me',
  main = 'pack_me.py',
  visibility = ['PUBLIC'],
)

and the pack_me.py is in tools:

#!/usr/bin/env python

from __future__ import print_function
from optparse import OptionParser
import sys

opts = OptionParser()
opts.add_option('-o', help='path to write WAR to')

args, ctx = opts.parse_args()

print('calling pack_me.py', file=sys.stderr)

f = open(args.o, "w")
f.write("I am a test rule.")
f.close()

Now I'm changing Java file, and rebuilding with:

  buck build broken

I can see in the log, that library was rebuilt:

[2015-10-16 22:48:05.164copy_resources of //:plugin-lib
rm -r -f /home/davido/projects/buck_test/buck-out/gen/lib__plugin-lib__output && mkdir -p /home/davido/projects/buck_test/buck-out/gen/lib__plugin-lib__output
mkdir -p /home/davido/projects/buck_test/buck-out/gen/lib__plugin-lib__output
jar cf buck-out/gen/lib__plugin-lib__output/plugin-lib.jar  buck-out/bin/lib__plugin-lib__classes
calculate_abi buck-out/gen/lib__plugin-lib__output/plugin-lib.jar
mkdir -p /home/davido/projects/buck_test/buck-out/gen
get_class_names buck-out/gen/lib__plugin-lib__output/plugin-lib.jar > buck-out/gen/plugin-lib.classes.txt
BUILT //:broken (4/5 JOBS)
BUILT //:plugin-lib (5/5 JOBS)
[-] BUILDING...FINISHED 0,4s [100%]

However, pack_me.py isn't executed, and thus we have stale output:

$ ls -all buck-out/gen/lib__plugin-lib__output/plugin-lib.jar
-rw-r--r-- 1 davido users 1632 Oct 16 22:50 buck-out/gen/lib__plugin-lib__output/plugin-lib.jar
davido@linux-ucwl:~/projects/buck_test (master *%=)$ ls -all ./java/org/ostrovsky/buck/Main.java
-rw-r--r-- 1 davido users 1360 Oct 16 22:50 ./java/org/ostrovsky/buck/Main.java
davido@linux-ucwl:~/projects/buck_test (master *%=)$ ls -all ./buck-out/gen/broken/broken.out
-rw-r--r-- 1 davido users 17 Oct 16 22:47 ./buck-out/gen/broken/broken.out

And pack_me.py wasn't (erroneously) executed. I would expect, that in this case 'pack_me.py' is called.

In real Gerrit Code Review build toolchain pack_war.py is used to produce final artifact (gerrit.war), however it's not invoked.

[davido]

Probably #427 may be related.

Moving the dependency from deps to $(location :foo) seems to fixed that:

genrule(
  name = 'broken',
  cmd = ' '.join(['$(exe //tools:pack_me)', '-d', '$(location //:plugin-lib)', '-o', '$OUT']),
  out = 'broken.out',
)

[davido]

Actually it is even worse as I thought it is: it's only working as expected for first order dependencies. When transitive dependencies of foo have changed, the genrule()'s command isn't invoked, even when the dependency foo is moved from deps to $(location :foo) macro in command part of genrule().

Consider this dependency graph: foo depends transitively on bar, and the broken defined as:

genrule(
  name = 'broken',
  cmd = ' '.join(['$(exe //tools:pack_me)', '-d', '$(location //:foo)', '-o', '$OUT']),
  out = 'broken.out',
)

and invoking:

  buck build :broken

wouldn't call pack_me.py, when some sources from bar were changed. To make the disaster complete: bar is actually rebuilt, but not :broken!

I think that replacing $(location :foo) macro with $(classpath :foo) macro fixes that:

genrule(
  name = 'broken',
  cmd = ' '.join(['$(exe //tools:pack_me)', '-d', '$(classpath //:foo)', '-o', '$OUT']),
  out = 'broken.out',
)

See: [1].

[1] https://gerrit-review.googlesource.com/71650

[Coneko]

Is plugin-lib.jar itself changing? If it's the same as before broken will not be run even though the dependencies of plugin-lib changed.

[davido]

Is plugin-lib.jar itself changing?

No, it's not. But that worked that way for years. Was this changed since bottom-up to top-down graph traversing? I also noticed, that running:

 buck build --deep gerrit

doesn't solve that: pack_war.py isnt' executed, even though, some deps in the chain were rebuilt. So the only way to make it work again (on Buck master) is to use $(classpath :plugin-lib), so that broken will always be executed, when any dependency in transitive dependency chain of plugin-lib was changed?

[davido]

So, i checked, since when it was broken.

It worked as expected on: 8204fddf60b25a3c2090f3ef0742fca5d466d562.

By working as expected I mean, that when something changed in the transitive dependency chain, and pack-war.py depends among other on gerrit-pgm:pgm:

$ buck targets --json :gerrit
Using buckd.
Adding watchman root: .
[
{
  "bash" : null,
  "buck.base_path" : "",
  "buck.direct_dependencies" : [ "//gerrit-pgm:pgm", "//gerrit-war:init", "//gerrit-war:log4j-config", "//gerrit-war:version", "//lib/log:impl_log4j", "//lib:postgresql", "//gerrit-gwtui:ui_optdbg", "//gerrit-main:main_bin", "//gerrit-war:webapp_assets", "//tools:pack_war" ],
  "buck.type" : "genrule",
  "cmd" : "$(exe //tools:pack_war) -o $OUT --tmp $TMP --lib //gerrit-war:log4j-config --lib //gerrit-war:init --lib //lib:postgresql --lib //lib/log:impl_log4j --lib //gerrit-war:version --pgmlib //gerrit-pgm:pgm $(location //gerrit-main:main_bin) $(location //gerrit-war:webapp_assets) $(location //gerrit-gwtui:ui_optdbg)",
  "cmdExe" : null,
  "deps" : [ "//gerrit-war:log4j-config", "//gerrit-war:init", "//lib:postgresql", "//lib/log:impl_log4j", "//gerrit-war:version", "//gerrit-pgm:pgm" ],
  "name" : "gerrit",
  "out" : "gerrit.war",
  "srcs" : [ ],
  "visibility" : [ ]
}
]

and it depends transitively on many other dependencies:

$ buck targets --json "//gerrit-pgm:pgm"
Using buckd.
Adding watchman root: .
[
{
  "annotationProcessorDeps" : [ "//lib:velocity", "//lib/auto:auto-value", "//lib/commons:collections", "//lib/commons:lang", "//lib/commons:oro" ],
  "annotationProcessorOnly" : null,
  "annotationProcessorParams" : [ ],
  "annotationProcessors" : [ "com.google.auto.value.processor.AutoAnnotationProcessor", "com.google.auto.value.processor.AutoValueProcessor" ],
  "buck.base_path" : "gerrit-pgm",
  "buck.direct_dependencies" : [ "//gerrit-cache-h2:cache-h2", "//gerrit-common:server", "//gerrit-extension-api:api", "//gerrit-gpg:gpg", "//gerrit-gwtexpui:linker_server", "//gerrit-gwtexpui:server", "//gerrit-httpd:httpd", "//gerrit-lucene:lucene", "//gerrit-oauth:oauth", "//gerrit-openid:openid", "//gerrit-pgm:daemon", "//gerrit-pgm:http", "//gerrit-pgm:init", "//gerrit-pgm:init-api", "//gerrit-pgm:util", "//gerrit-reviewdb:server", "//gerrit-server:server", "//gerrit-sshd:sshd", "//lib/auto:auto-value", "//lib/guice:guice", "//lib/guice:guice-assistedinject", "//lib/guice:guice-servlet", "//lib/jgit:jgit", "//lib/log:api", "//lib/log:jsonevent-layout", "//lib/log:log4j", "//lib/prolog:cafeteria", "//lib/prolog:compiler", "//lib/prolog:runtime", "//lib:args4j", "//lib:guava", "//lib:gwtorm", "//lib:protobuf", "//lib:servlet-api-3_1", "//lib/commons:collections", "//lib/commons:lang", "//lib/commons:oro", "//lib:velocity" ],
  "buck.type" : "java_library",
  "compiler" : null,
  "deps" : [ "//gerrit-common:server", "//gerrit-extension-api:api", "//gerrit-gwtexpui:linker_server", "//gerrit-gwtexpui:server", "//gerrit-httpd:httpd", "//gerrit-server:server", "//gerrit-sshd:sshd", "//gerrit-reviewdb:server", "//lib:guava", "//lib/guice:guice", "//lib/guice:guice-assistedinject", "//lib/guice:guice-servlet", "//lib/jgit:jgit", "//lib/log:api", "//lib/log:jsonevent-layout", "//lib/log:log4j", ":http", ":init", ":init-api", ":util", "//gerrit-cache-h2:cache-h2", "//gerrit-gpg:gpg", "//gerrit-lucene:lucene", "//gerrit-oauth:oauth", "//gerrit-openid:openid", "//lib:args4j", "//lib:gwtorm", "//lib:protobuf", "//lib:servlet-api-3_1", "//lib/auto:auto-value", "//lib/prolog:cafeteria", "//lib/prolog:compiler", "//lib/prolog:runtime", ":daemon" ],
  "exportedDeps" : [ ],
  "extraArguments" : [ "-encoding", "UTF-8" ],
  "javaVersion" : null,
  "javac" : null,
  "javacJar" : null,
  "mavenCoords" : null,
  "name" : "pgm",
  "postprocessClassesCommands" : [ ],
  "proguardConfig" : null,
  "providedDeps" : [ ],
  "resources" : [ "src/main/resources/com/google/gerrit/pgm/ProtoGenHeader.txt", "src/main/resources/com/google/gerrit/pgm/Startup.py" ],
  "resourcesRoot" : null,
  "source" : null,
  "srcs" : [ ],
  "target" : null,
  "tests" : [ ],
  "visibility" : [ "//:", "//gerrit-acceptance-tests/...", "//gerrit-gwtdebug:gwtdebug", "//tools/eclipse:classpath", "//Documentation:licenses.txt" ]
}
]

And one of those dependencies were changed, say //gerrit-server:server, then gerrit.war is rebuilt (pack_war.py was invoked).

It was broken since we upgraded to: d1be554f51fb9b2f090a85fcdbcef3b4dbbef8d7 with this diff: https://github.com/gerrit-review/gerrit/commit/f377aa96debec109671efc07102e2850445be62f.

To reproduce: change one file, say gerrit-server/src/main/java/com/google/gerrit/server/WebLinks.java

then run buck buid gerrit, Java is compiled, lib was rebuilt, but not gerrit.war.

$ ls -all buck-out/gen/gerrit-server/lib__server__output/server.jar
-rw-r--r-- 1 davido users 2799815 Oct 17 13:51 buck-out/gen/gerrit-server/lib__server__output/server.jar
$ ls -all buck-out/gen/gerrit/gerrit.war 
-rw-r--r-- 1 davido users 38332505 Oct 17 13:50 buck-out/gen/gerrit/gerrit.war

Since this upgrade our build toolchain is unreliable and the build results are inaccurate.

[1] https://gerrit-review.googlesource.com/70887

[davido]

Created a reproducer (upload in a moment to GH in its own repository) and bisected the problem:

143646f4ccd8737f73a2667a244c4c38dc18cb89 is the first bad commit

[Coneko]

If plugin-lib.jar in your example isn't changing, the genrule will not be executed because its inputs have been recomputed, but turned out to be same as before, so even if you were to execute the genrule, the result would be the same as far as Buck can tell.

If all the inputs to the genrule are the same, why would the result of the genrule be different? How are you reading in files from the genrule's command?

I'll have a look at the repro if you don't mind. It sounds like you're trying to use genrule as a way to inject side effects into the build, which we've never supported, and as we optimise Buck further we'll end up breaking a lot of workarounds that would make it possible to do that in the past.

[Coneko]

I saw the repro you sent to the mailing list: https://github.com/davido/buck_genrule_changed_caching_behaviour_143646f and how you fixed the issue in gerrit: https://gerrit-review.googlesource.com/#/c/71650/ .

I think this is working as intended, but it's certainly too easy to create a genrule that uses inputs Buck isn't tracking by using the buck audit or query command.

We really need to sandbox better, probably starting by preventing recursive buck invocation of any kind, and adding macros that would provide the functionality you need.

For example you call buck audit classpath libs in your genrule's script, but you could pass the same information in with the $(classpath ) macro I think right?

That would let Buck know that your genrule is using the inputs exported in the classpath, and it should be rerun if any of those change.

[davido]

I think this is working as intended, but it's certainly too easy to create a genrule that uses inputs Buck isn't tracking by using the buck audit or query command.

Ack.

For example you call buck audit classpath libs in your genrule's script, but you could pass the same information in with the $(classpath ) macro I think right?

You are right, of course. Thanks for pointing this out. I amended the fix correspondingly.