Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

declared but never created output directories #40

Closed
buchgr opened this issue Dec 5, 2018 · 12 comments
Closed

declared but never created output directories #40

buchgr opened this issue Dec 5, 2018 · 12 comments
Assignees

Comments

@buchgr
Copy link
Contributor

buchgr commented Dec 5, 2018

Hi,

when debugging a change with rules_kotlin I found that some actions declare output directories that they never create. This special case works well in Bazel local execution because the local strategies will create all output (and input) directories before even running the action.

I ran rules_kotlin against several remote execution backends and found that they behave differently. Some will include the never created directories as empty directories in the ActionResult and others will only include in the ActionResult what the action actually created.

The API isn't very specific on how to behave in this situation. It says

A list of the output directories that the client expects to retrieve from the action

I think we should specify this better and remove the ambiguity. I believe there are three options:

  1. Actions that don't create all declared outputs should fail.
  2. The remote execution system should declare never created directories (by the action) as empty directories in the action result.
  3. It's fine to only return a subset of the declared outputs directories.

At this point, I would argue for (3) to be the most sane behavior because this would also work for output files and not break existing clients (who are free to enforce this by themselves). I think this behavior is a bit unsatisfying though because the names of the expected outputs are part of the action key computation.

Thoughts?

P.S.: Somewhat related but not the same issue: bazelbuild/bazel#6393

@agoulti
Copy link

agoulti commented Dec 5, 2018

Actions that don't create all declared outputs should fail.

That's a difference between "actions" and "spawns". Remote execution receives spawns, and it's possible for those to have "optional" outputs. (I think once Ulf's changes completely land, this is how test.xml will be handled - as optional output in the first spawn)

expected outputs are part of the action key computation

Why is that a problem? For example, the Spawn Log distinguishes "declared outputs" and "actual outputs".
It only seems problematic if we try to reconstruct the action from actionResult, but I don't think we have enough information for that anyway?

@buchgr
Copy link
Contributor Author

buchgr commented Dec 5, 2018

That's a difference between "actions" and "spawns".

No it's not. This issue is independent of Bazel and only affects the remote execution API. Also, in this particular issue no optional outputs where involved and output directories cannot be declared as optional in Bazel - only files.

Why is that a problem?

I didn't say that it's a problem, but only that it's unsatisfying because two actions seeing the same inputs and creating the same outputs can have different action keys.

@agoulti
Copy link

agoulti commented Dec 5, 2018

No it's not. This issue is independent of Bazel and only affects the remote execution API.

Sorry, let me rephrase.

What I meant is, Bazel does not enforce a condition "all outputs must be generated" on spawns, only on actions. "test.xml" is going to be an example of that.
If we enforce "1. Actions that don't create all declared outputs should fail.", that means Bazel cannot use it for spawns (and e.g., the new testing set-up wouldn't work).

@buchgr
Copy link
Contributor Author

buchgr commented Dec 5, 2018

Again, this issue is independent of Bazel and I don't want to make a decision just because Bazel does it that way. We can certainly find solutions to make Bazel compliant.

@bergsieker
Copy link
Collaborator

bergsieker commented Dec 5, 2018 via email

@ola-rozenfeld
Copy link
Contributor

As a result of the discussion on bazelbuild/bazel#6393 I changed the wording to try and make it as clear as possible that what we do is option 3:

All declared action outputs are optional. This applies to both files and directories.

To me, the wording now seems unambiguous on this, but I'll gladly make it even clearer. Suggestions?

@buchgr
Copy link
Contributor Author

buchgr commented Dec 5, 2018

An unambiguous wording would be for the API spec to say exactly what you just wrote All declared action outputs are optional :-).

@ola-rozenfeld
Copy link
Contributor

You're right. How about the following paragraph here: https://github.com/bazelbuild/remote-apis/blob/master/build/bazel/remote/execution/v2/remote_execution.proto#L435

Clarification: the API does not require an action to actually produce any or all of its expected outputs. Any listed outputs not generated by the action will be omitted from the ActionResult. In particular: if an action has a listed directory output, but does not create the directory, then the directory will be omitted from the ActionResult.

@buchgr
Copy link
Contributor Author

buchgr commented Dec 5, 2018

Works for me :-). However, let's wait what others have to say e.g. @finnball @edbaunton @EdSchouten :-)

@EdSchouten
Copy link
Collaborator

Sounds good!

Not that I want to hijack this discussion: one thing that's also still a bit vague to me is:

  • Whether parent directories of output files/directories are created automatically prior to executing the build action. I noticed Bazel does so for local builds. Buildbarn does the same to remain compatible. Is this also what is desired?
  • What the permissions on input files/directories ought to be. Can files in the input root be changed? If not, may they be unlinked and replaced? Can new files/directories/... be created in any directory, or only in ones containing one or more output files?

@cjhopman
Copy link

Since as far as I can remember, this has been pretty clearly specified in the documentation of the fields in ActionResult. Buck has always depended on this behavior because it doesn't know if outputs are going to be files or directories (and so just specifies them in both lists).

@bergsieker
Copy link
Collaborator

I think we have a consensus on the original issue ("are outputs required or optional"), so I'll assign to @ola-rozenfeld to clarify the wording of the documentation.

@EdSchouten those are both good questions. Can you open separate issues for each of them, so we won't confuse the discussion threads here? Thanks!

EdSchouten pushed a commit to EdSchouten/remote-apis that referenced this issue Dec 23, 2018
When implementing Buildbarn's worker, I noticed that there exists the
assumption in build rules that directories under bazel-out/ are already
created prior to execution, even if they are not part of the input root.

As I suspect that getting rid of this assumption is both unrealistic and
undesirable, let's document this instead.

Mentioned in: bazelbuild#40
ola-rozenfeld pushed a commit that referenced this issue Jan 7, 2019
)

When implementing Buildbarn's worker, I noticed that there exists the
assumption in build rules that directories under bazel-out/ are already
created prior to execution, even if they are not part of the input root.

As I suspect that getting rid of this assumption is both unrealistic and
undesirable, let's document this instead.

Mentioned in: #40
santigl pushed a commit to santigl/remote-apis that referenced this issue Aug 26, 2020
santigl pushed a commit to santigl/remote-apis that referenced this issue Aug 26, 2020
…lbuild#106)

I wanted to do this in parts, but it is apparently impossible, since
pkg/tree relies on go/digest, and switching it alone to rely on
pkg/digest will break internal users anyway. So I will just manually
merge this commit internally while fixing all internal references.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants