search

bazelbuild / remote-apis

An API for caching and execution of actions on a remote system.
Apache License 2.0
333 stars 118 forks source link

Directory canonicalization does not require non-empty names #175

Open stuhood opened 4 years ago

stuhood commented 4 years ago

Hey folks! The definition of "canonical" for a Directory does not currently (https://github.com/bazelbuild/remote-apis/blob/f54876595da9f2c2d66c98c318d00b60fd64900b/build/bazel/remote/execution/v2/remote_execution.proto#L634-L653) require that the names of children are non-empty, but in practice this seems like a useful constraint.

As an example, the violation of one constraint on inputs lead to a server implementation returning a non-sensical output with an empty directory name (roughly: components like ["out", "", "etc", "file.txt"]). Including "non-empty" in the definition of canonical would have caught this earlier: immediately after the server's response.

Thoughts?

EdSchouten commented 3 years ago

FYI: These are the restrictions that Buildbarn applies: https://github.com/buildbarn/bb-storage/blob/d02061266d4c2f1bb7ecb52efad56e0a7f28c397/pkg/filesystem/path/component.go#L17

EricBurnett commented 3 years ago

It does say "Every child in the directory must have a path of exactly one segment", and you could argue that "" is zero segments rather than one. But either way, I'm fine with it being made more explicit that the empty string is not valid.

On Sun, Feb 7, 2021 at 7:43 AM Ed Schouten notifications@github.com wrote:

FYI: These are the restrictions that Buildbarn applies: https://github.com/buildbarn/bb-storage/blob/d02061266d4c2f1bb7ecb52efad56e0a7f28c397/pkg/filesystem/path/component.go#L17

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/bazelbuild/remote-apis/issues/175#issuecomment-774668273, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABREW23F3VPUBXYO67ZUXDS52DGNANCNFSM4RXUC67Q .