Add a note about ordering of Tree protos

[peterebden]

We recently discovered some nondeterminism in outputs because the Tree children were coming out in different orders. It looks like that specific case got fixed by bazelbuild/remote-apis-sdks#411, but it seems worth noting for any future implementations.

[EricBurnett]

LGTM. I considered whether the API should take a stronger stance on what the ordering should be, but

1. There is no trivial ordering recommendation, as Directory nodes aren't obviously sortable, and directory traversal order to compose this Tree could be specified but seems like it'd constrain the implementation too much.
2. I actually want to relax the ordering and uniqueness constraints on various other parts of the merkle tree specs to something similar for V3 (V3 idea: Loosen the restriction of action input as Merkle tree #141), so trying to retroactively specify a specific ordering for Tree now feels backwards, if it's not certain its the direction we want to go in the future.

With all that in mind, what you have here seems like the right balance for the present API.

[EdSchouten]

1. There is no trivial ordering recommendation, as Directory nodes aren't obviously sortable, and directory traversal order to compose this Tree could be specified but seems like it'd constrain the implementation too much.

Pieces of code that create Tree objects need to hash individual Directory objects anyway, so they can be referenced by their parent(s). It's trivial for an implementation to also use these hashes for sorting/deduplication purposes.

This is exactly what Buildbarn does, by the way. Child directories are sorted in increasing order by hash, and identical subtrees are merged together.

https://github.com/buildbarn/bb-remote-execution/blob/master/pkg/builder/output_hierarchy.go#L271-L282

[peterebden]

Pieces of code that create Tree objects need to hash individual Directory objects anyway, so they can be referenced by their parent(s). It's trivial for an implementation to also use these hashes for sorting/deduplication purposes.

Yep, agreed, that was the first thing that sprung to my mind too :) Although I could also imagine an implementation that always generates them in a deterministic order and so felt that sorting it was unnecessary.

Anyway, I agree with Eric that it's not really necessary for REAPI to specify how the ordering should be done here; the ordering only matters in terms of determinism of outputs, and as long as an implementation does it the same way each time that should be sufficient (I'm sure nobody has a need for determinism between different implementations...).