TreeArtifacts output directories aren't created with remote execution enabled

[cushon]

Previously: #6262

Repro:

cd $(mktemp -d)
git clone git@github.com:cushon/bazel-i6393.git
cd bazel-i6393/
bazel --bazelrc=/dev/null build --config=remote ...

ERROR: /tmp/tmp.pti7hTUec5/bazel-i6393/BUILD:3:1: error executing shell command: '/bin/bash -c cd bazel-out/k8-fastbuild/bin/a_dir && pwd' failed (Exit 1)
/bin/bash: line 0: cd: bazel-out/k8-fastbuild/bin/a_dir: No such file or directory
Target //:a failed to build

[ola-rozenfeld]

The expected behaviour is for the TreeArtifact output directory to be created before the action is executed.

Wait, why? We've always had the opposite expectation, the action itself should create the output directory, just like it should create the output files. This is what the current tests test for, too.

[cushon]

We've always had the opposite expectation

That isn't how the local, forge, or sandbox (post-9b8b79002dc9e62bc3fa56525dc4b01c3c2fe805) strategies handle this case.

My model is that the outputs are the contents of the directory. What is the advantage of requiring the action to create the directory?

The issue that prompted the original bug was that earlier versions of javac write output to a directory, but expect the directory to be pre-existing. We could work around that by creating a wrapper script for javac that creates the directory, but it seems strictly better to just guarantee the directory exists.

[cushon]

@philwo for 9b8b790

[cushon]

also @lberki who said the behaviour [of not creating the directories] was 'surprising' in cl/214229700

[ola-rozenfeld]

If we want remote execution to create the output directories before running the action, this must be explicitly specified in the remote execution API -- which it isn't, and I don't think it would be a good idea. Not only no existing servers support it, but why would they? This means no distinction between an empty output directory and a non-existing output directory. Why would we want to add this limitation? I mean, you can open a PR on the remote-apis repository for the discussion, but it won't be a short process (I foresee a lot of people to convince).

Currently, this sounds like a bug -- Bazel has an action that doesn't properly produce it's outputs when executed remotely. The wrapper idea sounds like the proper fix to me.

[cushon]

If we want remote execution to create the output directories before running the action, this must be explicitly specified in the remote execution API

The alternative is to "fix" all of the other spawn strategies to stop creating the output directories, is that the proposal? It feels like a regression to me, but it's probably better than the inconsistency.

This means no distinction between an empty output directory and a non-existing output directory. Why would we want to add this limitation?

In what sense is it a limitation? If an action declares an output directory and doesn't create it, currently it succeeds under remote execution and just doesn't produce output corresponding to that directory. If instead an empty directory was created and the action didn't write any output to that directory, wouldn't that have effectively the same result?

The wrapper idea sounds like the proper fix to me.

I mean, we could also have a wrapper that takes tarballs as input and unzips them, runs the tool, and then packages the outputs back up into another tarball. One of the benefits of TreeArtifacts is that they make it convenient to use tools that consume or produce directory contents without having to write wrapper scripts. Requiring those tools to create the output directories limits the generality of the solution.

[ola-rozenfeld]

In what sense is it a limitation? If an action declares an output directory and doesn't create it, currently it succeeds under remote execution and just doesn't produce output corresponding to that directory. If instead an empty directory was created and the action didn't write any output to that directory, wouldn't that have effectively the same result?

Currently, those actions produce different ActionResults by the remote server. In the first case, the output directory would not be returned at all. In the second case, an empty output directory would be returned. These are different behaviors. You propose to eliminate the distinction forever, by spec. It would also be inconsistent with other outputs -- why should an action always be specified to create all its output directories, but not its output files? Just as there is a difference between an empty output file and a missing output file, there should be a difference between an empty output directory and a missing output directory, imo.

The alternative is to "fix" all of the other spawn strategies to stop creating the output directories, is that the proposal?

I see a few solutions:

1. patching the remote strategy only to add a wrapper to create output directories
2. fixing all spawn strategies to use the same wrapper
3. changing the expectation so that an action is expected to always produce its outputs

I agree 2 is better than 1, although we could also start with 1 and move to 2. I'm not sure what would be required for 3 -- perhaps it's not feasible at all, because rule developers rely on the existing Bazel expectation?

[cushon]

why should an action always be specified to create all its output directories, but not its output files?

The language in Command.output_directories currently includes "only the contents of the indicated directories (recursively including the contents of their subdirectories) will be returned", which kind of sounds the directory itself isn't returned, which could suggest it isn't the actions' responsibility to create it. Regardless of what the outcome of this bug is, there might be an opportunity to tighten up the language in the spec.

I'm not sure what would be required for 3 -- perhaps it's not feasible at all, because rule developers rely on the existing Bazel expectation?

It'd be possible to rely on either behaviour, but naively it seems more likely for tools to assume the directory exists than to require that is doesn't exist. (Some tools might assert that it's non-empty, but that's not a problem.)

[ola-rozenfeld]

We thought this is not ambiguous because what we actually return in the ActionResult is a digest of the Tree message, which encodes the entire directory structure -- there is no way to confuse it with just a list of files (and there's no way to not return the directory itself). But yes, you're right that we should phrase this better -- I think I'll just remove the word "contents" altogether for extra clarity.

[bergsieker]

We had a similar discussion internally a few months ago, asking the question about intermediate directory nodes, and concluded that there wasn't a clear best path forward. :)

I'll note two things:

• We expected RBE to have other clients aside from Bazel, so we should value clear, unambiguous semantics.
• We also value uniform behavior between local and remote builds.

I'm inclined to say that at the API layer, we should NOT create output directories. That leaves the semantics clear, unambiguous, and simple. Bazel can then either use a wrapper to create the directories, or precreate them and declare them as inputs. (Do we support empty directories as inputs yet?)

[EricBurnett]

Before we go down the road of requiring a wrapper for all actions (which seems ugly/clunky enough to force that an API change is potentially better): can bazel specify the desired directory in the input tree? I seem to recall that when we first discussed this we thought that bazel could/should specify the directories it wants pre-created as part of the inputs, in which case it can directly apply the "directories should be pre-created" logic itself fairly easily.

Assuming that's correct, I still prefer not creating directories, but ensuring the caller has the necessary tools to request them be created as desired.

[bergsieker]

Can someone clarify how Bazel handles this locally? Are the output directories created as part of the spawn, or before the spawn actually executes?

[ola-rozenfeld]

We do support empty directories as inputs in the API (although not yet in RBE, IIRC, but that's something we need to fix regardless). That's a really good idea for a workaround! A patch in the remote runner only that will add all output directories to the input list as empty directories, ensuring they are created before action is executed. It is simpler than (1) above (although still kinda hacky). One problem is that I'm not sure the RBE side fix can be quick.

…

On Wed, Oct 17, 2018 at 10:48 AM Eric Burnett ***@***.***> wrote: Before we go down the road of requiring a wrapper for all actions (which seems ugly/clunky enough to force that an API change is potentially better): can bazel specify the desired directory in the *input* tree? I seem to recall that when we first discussed this we thought that bazel could/should specify the directories it wants pre-created as part of the inputs, in which case it can directly apply the "directories should be pre-created" logic itself fairly easily. Assuming that's correct, I still prefer not creating directories, but ensuring the caller has the necessary tools to *request* them be created as desired. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#6393 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AYoKuFoDNRKtMHE_xCqYhiRPMjer9MFYks5ul0NSgaJpZM4XdCX8> .

[philwo]

Bazel's local sandbox strategies create all output directories before running the spawn inside the sandbox: https://source.bazel.build/bazel/+/master:src/main/java/com/google/devtools/build/lib/sandbox/AbstractContainerizingSandboxedSpawn.java;l=102

I can't find it for the local strategies (I think it's somewhere happening in Skyframe), but @cushon recently looked into this and might remember where that was.

[benjaminp]

I can't find it for the local strategies (I think it's somewhere happening in Skyframe), but @cushon recently looked into this and might remember where that was.

It's handled by SkyframeActionExecutor. (Though with the caveat this is for actions not spawns, which only usually overlap.)

[bergsieker]

This sample fails in the same way when I run it locally (with or without the linux-sandbox). Is it just me?

If you change the command to "pwd && ls -R && cd %s && pwd" you can see that ./bazel-out/k8-fastbuild/bin is created automatically, but ./bazel-out/k8-fastbuild/bin/a_dir is not, regardless of spawn strategy.

[bergsieker]

@cushon What is the priority of this issue? I believe it was originally filed as a result of a regression related to updates in javabase handling, and I don't want remote execution to be blindsided by future updates.

[cushon]

This sample fails in the same way when I run it locally (with or without the linux-sandbox). Is it just me?

Which version of Bazel are you using? It works for me with 0.18.0 and --spawn_strategy=standalone, and it should work at head with local sandboxing.

What is the priority of this issue?

I'm working around it by not using TreeArtifacts (see 73972b3); it's not blocking.

[ulfjack]

Adding the directories as inputs seems odd, too. I think Buildbarn - at least - wants to create the input tree as read-only. I'd be happy with a solution where the client sends a list of directories in the protocol, but I'm unsure about reusing the inputs for that.

[EdSchouten]

Buildbarn ensures that all directories in the input root are writable, regardless of whether they were there explicitly (in the Merkle tree) or implicitly (as parents of outputs). It’s just that input files are read only, to make it possible to have a file cache with hardlinking.

[EricBurnett]

I think the ship has already sailed on allowing new outputs under the input root somewhere? E.g. with use of working_directory, it's almost certain the outputs will be somewhere other than the root of the action.

[EdSchouten]

Exactly!

[shahms]

There remains a situation in which a workaround is not presently possible: if you want to declare a directory and some subset of files beneath it. Say you know the primary output but not some additional outputs which may also be generated and need to be kept. Currently:

def _impl(ctx):
    resources = ctx.actions.declare_directory("{name}.d".format(name = ctx.label.name))
    primary_output = ctx.actions.declare_file("{name}.d/{name}.html".format(name = ctx.label.name))
    ctx.actions.run_shell(
        command = "touch \"$@\"",
        outputs = [resources, primary_output],
        arguments = [primary_output.path],
    )
    return [DefaultInfo(files = depset([resources]))]

dir_and_file = rule(
    implementation = _impl,
)

Building targets using that rule with RBE fails inside Bazel and no amount of mkdir in the script will fix that. While you can just declare the directory, it is useful for a variety of reasons to be able to have a File nominating both the primary output as well as the containing directory. With the current remote semantics, that's not possible.

[lberki]

I would much prefer not to support this case because the fact that a declare_file file refers to a subset of declare_directory directory makes for very complicated behavior: for example, what if they come from different actions (or even configured targets) and then one action needs to be invalidated and the other does not?

[shahms]

Very reasonable. I should note that Bazel doesn't currently allow separate actions to output a directory and a file within that directory (it gives an error about one artifact being a prefix of the other). It might be nice if this error happened at artifact declaration time instead if this is not intended to be supported.

[fmeum]

I just found this issue while working on a fix for the "other half" over at #15276: Creating the directories corresponding to empty TreeArtifacts, which Bazel's sandbox runners do, but remote execution doesn't (yet).

Does anybody see an issue with Bazel adding the output directories to the input tree? I do see great value in having this issue fixed one way or the other and fixing it in Bazel seems much simpler than amending the spec and waiting for all RBE implementations to adapt. If this approach is generally viewed to be viable, I would be happy to implement it.

[EricBurnett]

Re-reading the discussion above (it's been a few years :)), I believe that should be fine and correct:

1. With the deprecation of output_directories in the spec and the switch to output_paths where it's not defined ahead of time whether an output will be a directory or a file, servers do not have the information to create output directories proactively. (Though they should be creating parent directories, per comment on output_paths).
2. With the existence of working_directory, it's generally expected that outputs may fall somewhere under the input root.
3. And more broadly, nothing has ever specified they should be disjoint.

So I think it's completely reasonable to use directories in the input to specify what should be pre-created for outputs, and in fact, the most reasonable choice given the spec constraints. (Consistent with my opinion back in 2018: #6393 (comment))

Conversely, I think the only place where this could be problematic is if any servers have specific expectations that a directory is not simultaneously listed in the input root and specified as an output. I can't think of a good reason they would have - needing to support (2) means that input directories must sometimes be writable at some level, so it would seem to be a server-side assertion with no clear technical purpose, and not supported by any spec text (3). Seems unlikely to me... though I won't go so far as to say it's definitely safe for all existing implementations :). Other folk can flag if they know of any implementations that might care.

[lberki]

/cc @coeuvre

[fmeum]

I just noticed that Bazel's own remote backend implementation doesn't allow for output directories to be listed in inputs, but that does seem to be against the spec. For example, the spec explicitly mentions that the entire working directory can be declared as an output directory.