Move building of proto generated files to its own crate.

[ernoc]

I'm moving the generating Rust code for our protos into its own proto crate (#4565) . [EDIT 4 Jan 2024: two different crates, see comments]

oak_attestation_verification provides conversion from its own result type (anyhow::Result<DiceChainResult>) into AttestationResult proto.

It used to do that by implementing From. Problem: we can no longer do that because now both AttestationResult and Result are foreign structs now.

For now, I've created an arbitrary conversion function (option 3 below), but opening this for discussion.

Here are some options:

1 - Make verifier.rs use AttestationResult for its public interface, rather than anyhow::Result<DiceChainResult>
2 - Bake the result cases into DiceChainResult. So verifier.rs's public interface would use DiceChainResult, and DiceChainResult would have 2 cases: Ok and Err.
3 - Just not use std::convert, simply supply a public function to convert
4 - Supply an impl From<DiceChainResult> in oak_attestation_verification crate and an impl From<Error> in the proto crate. proto crate would now have both generated and hand-written Rust code. Still not super useful as client code needs to do the matching before converting.
5 - [Just for completeness, no-go] implement From<&anyhow::Result<DiceChainResult>> in the proto crate, making it depend on oak_attestation_verification crate.

[tiziano88]

At high level, option 3 makes sense to me too. I am not convinced that we want all the protos in one crate though. This means that a change in any proto will require rebuilding almost all crates (since I think every single of our crates transitively depends on some proto). This is not a huge issue, but it definitely imposes an ongoing cost in terms of development and CI feedback loops.

As a side question: what will this look like if / when we migrate to bazel? I don't even know what the crate granularity is there. Is each bazel target its own crate? How do submodules work?

[ernoc]

I think in generic terms, it would make sense to group protos into cohesive packages based on what other packages depend on them. In Rust this would be having multiple crates. Is there a split that you have in mind currently, or for the time being should we go with 1 single crate, later split it?

In Bazel, this will be just 1 package (one directory), but multiple independent build targets in it.

With possible use of Bazel in mind, it might make sense to keep 1 directory (therefore 1 crate) for now.

[ernoc]

On the other hand, we don't change protos all that often: https://github.com/project-oak/oak/commits/main/proto - so the rebuilding everything issue would be infrequent?

[tiziano88]

SG to go with one package for now.

I am still not clear what the mapping between cargo crates and bazel targets is though. AFAICT there are no actual crates in bazel, just individual targets -- or, similarly, each target is equivalent to its own crate? Or are there crates and mods also in bazel?

[ernoc]

In Rust (IIUC), a package can have many crates. As someone new to Rust, it seems like it may be a good idea to have only 1 crate per package though.

In Bazel a package can have many targets. In Bazel+Proto, for example, you can have 1 package for protos, and several build targets for proto_java_library, proto_cc_library, libraries that derive RPC/service functionality from protos, etc.

[tiziano88]

IIUC in Bazel, a package is not really a concrete unit, is it? In other words, the only unit of compilation (and dependency) is a target. I.e. if there are several targets in a folder, they are completely independent (apart from visibility and other explicitly package-level attributes). Or do packages do more that I am not aware of?

[ernoc]

True, packages are only used for organising visibility, etc, - they're not buildable.

Re proto package dependencies: I've now declared 2 independent crates:

• oak_attestation_proto
• oak_functions_proto

So changes in one will not trigger rebuilding the other's dependencies.

[tiziano88]

(nit) remove all empty lines in between use statements.

[tiziano88]

This makes sense, but I am not sure we want the rust crates under the proto tree though. Could we define the oak_attestation_proto crate at top level, and just refer to the protos in this folder?

[ernoc]

I'm ok to do that but here's a different point of view: the only reason we have these Rust files under proto is because of limitations in prost that it won't output a "ready to use" Rust crate - it will instead output a file that needs to be included somewhere and this is that somewhere. The non-generated code should only exist to include that file and give it module names. Moving it to a "normal" crate invites the temptation of writing more non-generated code, making it then harder to port to Bazel? With Bazel I expect we'll need even less (or hopefully, maybe, none) of this code - just the proto files.

[tiziano88]

I think leaving a comment in the lib.rs file of the crate should be enough to clarify that we don't expect to write any additional code there -- we don't need to bury the crate into another directory.

[ernoc]

Ok, in that case would you be OK with moving the proto files themselves to those respective crates too? Otherwise we are still in a situation where we have a Rust crate depending on a proto file that's in another directory tree, in a way that's opaque to the build system (paths resolved by arbitrary Rust code in build.rs), which is why we wanted this PR initially?

[tiziano88]

I forget -- if the crate is next to the proto, does change tracking work? I thought it still doesn't

[tiziano88]

Also it may be worth trying to rebuild the proto crates across workspaces now that #4597 and similar other changes are in. Maybe we don't need anything more? But I may be missing some other issue that we are trying to address

[tiziano88]

Do you have a command that you are trying to run that currently does not work, but your PR is meant to address? e.g. depending on the protos across workspaces, or something like that? So we know when we are "done"

[ernoc]

No, nothing is broken that I know right now, so this isn't really needed. It would only serve as a pre-step to Bazel building but strictly not needed.