oak icon indicating copy to clipboard operation
oak copied to clipboard

Move building of proto generated files to its own crate.

Open ernoc opened this issue 1 year ago • 7 comments

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.

ernoc avatar Dec 28 '23 12:12 ernoc

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?

tiziano88 avatar Dec 29 '23 14:12 tiziano88

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 avatar Jan 02 '24 10:01 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?

ernoc avatar Jan 02 '24 10:01 ernoc

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?

tiziano88 avatar Jan 02 '24 15:01 tiziano88

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.

ernoc avatar Jan 03 '24 09:01 ernoc

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?

tiziano88 avatar Jan 03 '24 09:01 tiziano88

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.

ernoc avatar Jan 04 '24 18:01 ernoc