commit 4784d735f5c13ab7ad0c4c1068c50a3a6a23f855
author ajurkowski <ajurkowski@google.com> Tue Jul 07 11:33:14 2020 -0700
committer Copybara-Service <copybara-worker@google.com> Tue Jul 07 11:34:23 2020 -0700
tree e6b94cec5db868416f5f2ad316a5825f9feff8bf
parent 1437f34aa30fc199d36da4aafe9d2794432ad39f

Expand `ActionExecutionMetadata.getKey()` method to accept an ArtifactExpander.

Computing a key which uniquely identifies whether 2 actions are equal is
generally impossible without the ability to expand tree artifacts. The source of
the problem happens for Starlark-defined actions which use `Args.add_all` with
a custom `map_each` function and `expand_directories` option enabled. For such
functions, we cannot know the real output of the execution until we can expand
the directory arguments.

Please consider an example:

```
def f(file):
  if file.is_directory():
    return file.path
  return file.path + '_suffix'

directory = ... # input directory
args = ctx.actions.args()
args.add_all([directory], map_each=f, expand_directories=True)
ctx.actions.run(..., inputs=[directory], arguments=args)
```

If we make a change to `f`, which only affects the behavior for files, we
cannot detect that without actually expanding the directory:

```
def f(file):
  if file.is_directory():
    return file.path
  return file.path + '_different_suffix'
```

Technically, we could have a digest identifying Starlark code and base the key
on that, but that is a feature which would limit the ability to expand Starlark
in the future and it does not fit the overall design of it. Additionally to
that, we could theoretically change the function in a way which does not affect
the output for the contents of a given directory, which would lead to
false-positive changes to the key. Due to those concerns, the solution to this
problem is to provide the artifact expander so that we can actually run the
Starlark function with the same parameters as if we were to construct the
command line.

This is a pure refactoring change in preparation to provide a fix for the bug.
We are adding an optional `ArtifactExpander` parameter which will only be used
in the future and, until then, is always null.

PiperOrigin-RevId: 320025538

src/main/java/com/google/devtools/build/lib/analysis/BUILD

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BUILD b/src/main/java/com/google/devtools/build/lib/analysis/BUILD
index 4a976bc..02925d8 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/analysis/BUILD
@@ -1224,6 +1224,7 @@
         "//src/main/java/com/google/devtools/build/lib/concurrent",
         "//src/main/java/com/google/devtools/build/lib/util",
         "//third_party:guava",
+        "//third_party:jsr305",
     ],
 )
 
@@ -1300,6 +1301,7 @@
         "//src/main/java/com/google/devtools/build/lib/actions",
         "//src/main/java/com/google/devtools/build/lib/collect/nestedset",
         "//src/main/java/com/google/devtools/build/lib/util",
+        "//third_party:jsr305",
     ],
 )
 
@@ -1313,6 +1315,7 @@
         "//src/main/java/com/google/devtools/build/lib/collect/nestedset",
         "//src/main/java/com/google/devtools/build/lib/util",
         "//third_party:guava",
+        "//third_party:jsr305",
     ],
 )
 
@@ -1330,6 +1333,7 @@
         "//src/main/java/com/google/devtools/build/lib/util",
         "//src/main/protobuf:failure_details_java_proto",
         "//third_party:guava",
+        "//third_party:jsr305",
     ],
 )
 
@@ -1354,6 +1358,7 @@
         "//src/main/java/com/google/devtools/build/lib/util",
         "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
         "//third_party:guava",
+        "//third_party:jsr305",
     ],
 )
 
@@ -1984,6 +1989,7 @@
         "//src/main/java/com/google/devtools/build/lib/util",
         "//src/main/protobuf:failure_details_java_proto",
         "//third_party:guava",
+        "//third_party:jsr305",
     ],
 )