retain the OutputStore when include scanning

[buchgr]

this avoids information loss about injected output
metadata. this change resolves the TODO.

Progress towards #6862

[ericfelly]

this seems like a good change. any idea why it wasn't implemented like this to begin with? any tests we can add?

[justinhorvitz]

I think that TODO predated 6e9af75#diff-3f905f1f4fdd9e7e4c941d2e860c304a, and therefore wasn't trivial because OutputStore was not yet separated from ActionMetadataHandler. I didn't catch that we could resolve this TODO when I implemented the OutputStore. This looks like a good change to me, but I'm not as familiar with when this code path is triggered.

[lberki]

I'm afraid I'm out of my depth here. Given that there are three engineers that feel that way other than the author of the change, how about declaring thta @buchgr did due diligence and just submitting it, then seeing what breaks?

What about the same code in lines 574 and 652?

https://github.com/bazelbuild/bazel/blob/master/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java#L652
https://github.com/bazelbuild/bazel/blob/master/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java#L574

[buchgr]

@lberki

https://github.com/bazelbuild/bazel/blob/master/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java#L652

this code path is triggered during include scanning which happens before any execution / metadata injection and so I believe re-creating the output store here is benign but should probably also be fixed for easier understanding of the code.

https://github.com/bazelbuild/bazel/blob/master/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java#L574

This creates the metadatahandler with an outputstore for the first time. so this code path is the initialization logic and legit.

@ericfelly I don't have a good way of testing this. This change is part of a list of changes making it possible to not download remote outputs and I ll be adding integration tests for this change along the way.

[ericfelly]

@janakdr FYI

[lberki]

@buchgr , you're indeed right re: line 574, I didn't read the code thoroughly enough.

[jin]

can this be imported?

[buchgr]

yep. I am re-running CI after resolving merge conflicts. will import then.