Propagate the full workspace status info to cc_library linkstamp.

[ccciudatu]

Minimal patch to fix #1992 and #4863.

Includes both predefined and custom keys as build information
for C++ stamping, treating all unknown keys as strings.

Makes it so that the build-info will be redacted iif both
volatile and non-volatile info is requested in a single header,
since the inputs are now always passed in to figure out the
available keys.

Closes #1992.
Closes #4863.

[ccciudatu]

https://bazelbuild.slack.com/archives/CGA9QFQ8H/p1629163152051500

[oquenchil]

LGTM

[ccciudatu]

@oquenchil Thank you for your review.
Do you happen to know when this might be merged?

[oquenchil]

Sorry, this causes 2 internal tests to fail. When I have time I will fix them and merge.

[oquenchil]

Let me copy paste what the errors are and the tests themselves, perhaps you can take a look:

java.lang.IllegalStateException
	at com.google.common.base.Preconditions.checkState([Preconditions.java:493](https://cs.corp.google.com/#search/&q=f:com/google/common/base/Preconditions.java:493&ws=tap-prod-presubmit/171985774&snapshot=2))
	at com.google.devtools.build.lib.rules.cpp.WriteBuildInfoHeaderAction.<init>(WriteBuildInfoHeaderAction.java:78)
	at com.google.devtools.build.lib.rules.cpp.WriteBuildInfoHeaderActionTest.writeHeader([WriteBuildInfoHeaderActionTest.java:332](https://cs.corp.google.com/#search/&q=f:com/google/devtools/build/lib/rules/cpp/WriteBuildInfoHeaderActionTest.java:332&ws=tap-prod-presubmit/171985774&snapshot=2))
	at com.google.devtools.build.lib.rules.cpp.WriteBuildInfoHeaderActionTest.testDeterminism([WriteBuildInfoHeaderActionTest.java:182](https://cs.corp.google.com/#search/&q=f:com/google/devtools/build/lib/rules/cpp/WriteBuildInfoHeaderActionTest.java:182&ws=tap-prod-presubmit/171985774&snapshot=2))
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke([Method.java:566](https://cs.corp.google.com/#search/&sq=package:%5Epiper$%20project:openjdk&q=java/lang/reflect/Method.java:566))
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall([FrameworkMethod.java:59](https://cs.corp.google.com/piper///depot/google3/third_party/java_src/junit/v4_12/main/java/org/junit/runners/model/FrameworkMethod.java?q=invoke&l=59&ws=tap-prod-presubmit/171985774&snapshot=2))
	at org.junit.internal.runners.model.ReflectiveCallable.run([ReflectiveCallable.java:12](https://cs.corp.google.com/piper///depot/google3/third_party/java_src/junit/v4_12/main/java/org/junit/internal/runners/model/ReflectiveCallable.java?q=runReflectiveCall&l=12&ws=tap-prod-presubmit/171985774&snapshot=2))
	at org.junit.runners.model.FrameworkMethod.invokeExplosively([FrameworkMethod.java:61](https://cs.corp.google.com/piper///depot/google3/third_party/java_src/junit/v4_12/main/java/org/junit/runners/model/FrameworkMethod.java?q=run&l=61&ws=tap-prod-presubmit/171985774&snapshot=2))
	at org.junit.internal.runners.statements.InvokeMethod.evaluate([InvokeMethod.java:17](https://cs.corp.google.com/piper///depot/google3/third_party/java_src/junit/v4_12/main/java/org/junit/internal/runners/statements/InvokeMethod.java?q=invokeExplosively&l=17&ws=tap-prod-presubmit/171985774&snapshot=2))
	at org.junit.runners.ParentRunner$3.evaluate([ParentRunner.java:308](https://cs.corp.google.com/piper///depot/google3/third_party/java_src/junit/v4_12/main/java/org/junit/runners/ParentRunner.java?q=evaluate&l=308&ws=tap-prod-presubmit/171985774&snapshot=2))
	at org.junit.runners.BlockJUnit4ClassRunner$1.evaluate([BlockJUnit4ClassRunner.java:100](https://cs.corp.google.com/piper///depot/google3/third_party/java_src/junit/v4_12/main/java/org/junit/runners/BlockJUnit4ClassRunner.java?q=evaluate&l=100&ws=tap-prod-presubmit/171985774&snapshot=2))
	at org.junit.runners.ParentRunner.runLeaf([ParentRunner.java:368](https://cs.corp.google.com/piper///depot/google3/third_party/java_src/junit/v4_12/main/java/org/junit/runners/ParentRunner.java?q=evaluate&l=368&ws=tap-prod-presubmit/171985774&snapshot=2))
	at org.junit.runners.BlockJUnit4ClassRunner.runChild([BlockJUnit4ClassRunner.java:103](https://cs.corp.google.com/piper///depot/google3/third_party/java_src/junit/v4_12/main/java/org/junit/runners/BlockJUnit4ClassRunner.java?q=runLeaf&l=103&ws=tap-prod-presubmit/171985774&snapshot=2))
	at org.junit.runners.BlockJUnit4ClassRunner.runChild([BlockJUnit4ClassRunner.java:63](https://cs.corp.google.com/piper///depot/google3/third_party/java_src/junit/v4_12/main/java/org/junit/runners/BlockJUnit4ClassRunner.java?q=runChild&l=63&ws=tap-prod-presubmit/171985774&snapshot=2))
	at org.junit.runners.ParentRunner$4.run([ParentRunner.java:333](https://cs.corp.google.com/piper///depot/google3/third_party/java_src/junit/v4_12/main/java/org/junit/runners/ParentRunner.java?q=runChild&l=333&ws=tap-prod-presubmit/171985774&snapshot=2))
	at org.junit.runners.ParentRunner$1.schedule([ParentRunner.java:81](https://cs.corp.google.com/piper///depot/google3/third_party/java_src/junit/v4_12/main/java/org/junit/runners/ParentRunner.java?q=run&l=81&ws=tap-prod-presubmit/171985774&snapshot=2))
	at org.junit.runners.ParentRunner.runChildren([ParentRunner.java:331](https://cs.corp.google.com/piper///depot/google3/third_party/java_src/junit/v4_12/main/java/org/junit/runners/ParentRunner.java?q=schedule&l=331&ws=tap-prod-presubmit/171985774&snapshot=2))
	at org.junit.runners.ParentRunner.access$100([ParentRunner.java:68](https://cs.corp.google.com/piper///depot/google3/third_party/java_src/junit/v4_12/main/java/org/junit/runners/ParentRunner.java?q=runChildren&l=68&ws=tap-prod-presubmit/171985774&snapshot=2))
	at org.junit.runners.ParentRunner$2.evaluate([ParentRunner.java:295](https://cs.corp.google.com/piper///depot/google3/third_party/java_src/junit/v4_12/main/java/org/junit/runners/ParentRunner.java?q=access$100&l=295&ws=tap-prod-presubmit/171985774&snapshot=2))
	at org.junit.runners.ParentRunner$3.evaluate([ParentRunner.java:308](https://cs.corp.google.com/piper///depot/google3/third_party/java_src/junit/v4_12/main/java/org/junit/runners/ParentRunner.java?q=evaluate&l=308&ws=tap-prod-presubmit/171985774&snapshot=2))
	at org.junit.runners.ParentRunner.run([ParentRunner.java:420](https://cs.corp.google.com/piper///depot/google3/third_party/java_src/junit/v4_12/main/java/org/junit/runners/ParentRunner.java?q=evaluate&l=420&ws=tap-prod-presubmit/171985774&snapshot=2))
	at com.google.testing.junit.runner.internal.junit4.CancellableRequestFactory$CancellableRunner.run([CancellableRequestFactory.java:108](https://cs.corp.google.com/#search/&q=f:com/google/testing/junit/runner/internal/junit4/CancellableRequestFactory.java:108&ws=tap-prod-presubmit/171985774&snapshot=2))
	at org.junit.runner.JUnitCore.run([JUnitCore.java:137](https://cs.corp.google.com/piper///depot/google3/third_party/java_src/junit/v4_12/main/java/org/junit/runner/JUnitCore.java?q=run&l=137&ws=tap-prod-presubmit/171985774&snapshot=2))
	at org.junit.runner.JUnitCore.run([JUnitCore.java:115](https://cs.corp.google.com/piper///depot/google3/third_party/java_src/junit/v4_12/main/java/org/junit/runner/JUnitCore.java?q=run&l=115&ws=tap-prod-presubmit/171985774&snapshot=2))
	at com.google.testing.junit.runner.junit4.JUnit4Runner.run([JUnit4Runner.java:104](https://cs.corp.google.com/#search/&q=f:com/google/testing/junit/runner/junit4/JUnit4Runner.java:104&ws=tap-prod-presubmit/171985774&snapshot=2))
	at com.google.testing.junit.runner.RunnerShell$2.run([RunnerShell.java:34](https://cs.corp.google.com/#search/&q=f:com/google/testing/junit/runner/RunnerShell.java:34&ws=tap-prod-presubmit/171985774&snapshot=2))
	at com.google.testing.junit.runner.GoogleTestRunner.runTestsInSuite([GoogleTestRunner.java:200](https://cs.corp.google.com/#search/&q=f:com/google/testing/junit/runner/GoogleTestRunner.java:200&ws=tap-prod-presubmit/171985774&snapshot=2))
	at com.google.testing.junit.runner.GoogleTestRunner.runTestsInSuite([GoogleTestRunner.java:184](https://cs.corp.google.com/#search/&q=f:com/google/testing/junit/runner/GoogleTestRunner.java:184&ws=tap-prod-presubmit/171985774&snapshot=2))
	at com.google.testing.junit.runner.GoogleTestRunner.main([GoogleTestRunner.java:137](https://cs.corp.google.com/#search/&q=f:com/google/testing/junit/runner/GoogleTestRunner.java:137&ws=tap-prod-presubmit/171985774&snapshot=2))

  @Test
  public void testDeterminism() throws Exception {
    BlazeDirectories directories =
        new BlazeDirectories(
            new ServerDirectories(
                scratch.dir("/install"), scratch.dir("/base"), scratch.dir("/user_root")),
            scratch.dir("/workspace"),
            /* defaultSystemJavabase= */ null,
            "mock-product-name");
    BinTools binTools = BinTools.empty(directories);
    Executor executor =
        new TestExecutorBuilder(fileSystem, directories, binTools)
            .addContext(WorkspaceStatusAction.Context.class, new NondeterministicContext())
            .build();

    List<List<String>> lines = writeHeader(executor, true, true, null, 3);
    assertThat(lines).hasSize(3);

    String lastLine = null;
    for (List<String> lineVal : lines) {
      assertThat(lineVal).hasSize(1);
      String line = Iterables.getOnlyElement(lineVal);
      assertThat(line).contains("#define NONDET");
      if (lastLine != null) {
        assertThat(lastLine).isEqualTo(line);
      }
      lastLine = line;
    }
  }
  @Test
  public void testHeaderMetadata() throws Exception {
    Path execRoot = scratch.dir("/base/root");
    String outSegment = "out";
    execRoot.getChild(outSegment).createDirectoryAndParents();
    ArtifactRoot root = ArtifactRoot.asDerivedRoot(execRoot, RootType.Output, outSegment);
    Path txt = scratch.overwriteFile("/base/root/out/info.txt");
    scratch.overwriteFile("/base/root/out/header.h", "something");

    ArtifactFactory artifactFactory =
        new ArtifactFactory(/* execRootParent= */ execRoot.getParentDirectory(), "blaze-out");

    scratch.overwriteFile("/base/root/out/header_ft.h", "something");
    Artifact buildInfoHeader = artifactFactory.getDerivedArtifact(
        PathFragment.create("header_ft.h"), root, ActionsTestUtil.NULL_ARTIFACT_OWNER);
    assertThat(getMetadata(buildInfoHeader).getDigest()).isNotNull();

    Action action =
        new WriteBuildInfoHeaderAction(
            NestedSetBuilder.emptySet(Order.STABLE_ORDER),
            buildInfoHeader,
            /*writeVolatileInfo=*/ false,
            /* writeStableInfo= */ true);
    assertThat(action.getOutputs()).containsExactly(buildInfoHeader);

    scratch.overwriteFile("/base/root/out/header_tf.h", "something");
    buildInfoHeader = artifactFactory.getDerivedArtifact(
        PathFragment.create("header_tf.h"), root, ActionsTestUtil.NULL_ARTIFACT_OWNER);
    assertThat(getMetadata(buildInfoHeader).getDigest()).isNotNull();

    action =
        new WriteBuildInfoHeaderAction(
            NestedSetBuilder.emptySet(Order.STABLE_ORDER),
            buildInfoHeader,
            /*writeVolatileInfo=*/ true,
            /* writeStableInfo= */ false);
    assertThat(action.getOutputs()).containsExactly(buildInfoHeader);

    scratch.overwriteFile("/base/root/out/header_tf2.h", "something");
    buildInfoHeader = artifactFactory.getConstantMetadataArtifact(
        PathFragment.create("header_tf2.h"), root, ActionsTestUtil.NULL_ARTIFACT_OWNER);
    // Artifact should force constant metadata here.
    assertThat(buildInfoHeader.isConstantMetadata()).isTrue();

    action =
        new WriteBuildInfoHeaderAction(
            NestedSetBuilder.create(Order.STABLE_ORDER, ActionsTestUtil.createArtifact(root, txt)),
            buildInfoHeader,
            /*writeVolatileInfo=*/ true,
            /* writeStableInfo= */ false);
    assertThat(action.getOutputs()).containsExactly(buildInfoHeader);
  }

  @Test
  public void testRedacted() throws Exception {
    Executor executor =
        getExecutor(
            ImmutableMap.of(
                "STRING", Key.of(KeyType.STRING, "default", "redactedString"),
                "INTEGER", Key.of(KeyType.INTEGER, "0", "69")),
            ImmutableMap.of());
    List<String> lines = writeHeader(executor, false, true, null);
    assertThat(lines).containsExactly(
        "#define STRING \"redactedString\"",
        "#define INTEGER 69");
  }

[ccciudatu]

@oquenchil The main issue with these tests is that they rely on the current assumption that passing an empty input set should result in redacted information being generated.

With this patch, this assumption is no longer true, since we now need to account for custom variables as well, so we need access to stable-status.txt and volatile-status.txt to get the actual keys produced by the workspace_status script, instead of only producing values (redacted or not) for the predefined variables.
So I decided to disallow empty input sets to make it clear that build info can't be generated out of thin air anymore (the error should probably be an IAE instead of ISE, but that's a different concern).

As a consequence, to infer whether the build info should be "redacted", we now rely on the request for both volatile and non-volatile info at the same time. Previously, this was only allowed when inputs were empty (which meant "redacted"), now I made it so that it is itself the trigger for "redacted". The downside is that you can't request for stable-only or volatile-only redacted information. This was meant to keep the existing method signature by also preserving an invariant that seemed to hold true throughout the public code, but please let me know if you think we need an explicit "redacted" flag to be passed in or a different way to infer the "redacted" status (rather than hijacking the existing flags).

Assuming we keep the "redacted" inference as per this PR, we can fix the tests by always providing non-empty input artifacts and asserting redacted only when we pass in both writeVolatileInfo and writeStableInfo as true.
If there is a use-case where we actually want to generate redacted information for stable-only or volatile-only, then I'll have to change the code to expect an explicit redacted flag, alongside with the existing params.

Simply allowing empty input sets (as before) is also an option, just to account for these tests. But I find it dangerous/confusing, as it would silently still allow generating just the predefined variables as redacted build info. What do you think?

[buildbreaker2021]

Hey,

IIUC the issues you linked would be fixed if users had their say in generating BuildInfo files.
There is an ongoing effort to do that, so I suggest we wait.
Reason being that this code will most likely be gone from Bazel in the future and merging this PR requires some effort(at least it is not trivial how this will effect things internally, failing unittests are already a bad start to being with).
WDYT?

[a7g4]

Hi, any update on merging this? I just ran into the same issue :(