-
Notifications
You must be signed in to change notification settings - Fork 113
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
Add bazel rule for converting OCI images to OCI runtime bundles #4619
Conversation
I filed #4620 to track fixing the build issues. The problem is that Nix uses non-standard paths, so the hermetic GCC toolchain can't run in Nix + docker. (It does run locally when using direnv because it can use the system's dynamic linker.) |
Changing the compiler toolchain is for some reason causing //cc/transport:grpc_streaming_transport_test to fail with an invalid clock type. I have no idea why this is happening; I'll need to look more tomorrow. |
Tests are now fixed. It turns out that changing the compiler exacerbated an existing use of uninitialized memory in grpc_streaming_transport_test. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
licenses = ["notice"], | ||
) | ||
|
||
proto_library( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please double-check that the internal script that copies the code from Github will not remove this file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently it only copies "oak_containers/proto/*.proto" so I believe it will be ignored.
@@ -0,0 +1,44 @@ | |||
# |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are currently trying to onsolidate all protos under the same proto
directory: https://github.com/project-oak/oak/tree/main/proto
I think we could add the /proto/application/hello_world/
directory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opened #4663 to consolidate some of the existing protos. I personally don't think we should move any existing protos as part of this pr.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds good to me. Thanks, @jul-sh!
@@ -30,6 +32,18 @@ proto_library( | |||
], | |||
) | |||
|
|||
cc_proto_library( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We were currently automatically generating these rules during code-copy from Github
So current script needs to be updated to not fail during download
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how to avoid a brief failure -- let's talk more offline about how to sequence the necessary changes.
oak_containers_hello_world_untrusted_app/tests/integration_test.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this is cool.
This rule only supports x86_64 linux because it uses a pre-compiled umoci binary. If other build environments are required later, additional toolchains can be added.
This matches the Rust implementation and is useful for (a) demonstrating the C++ Oak Containers APIs and (b) demonstrating in a follow-up change how to build OCI runtime bundles with bazel.
The integration test uses the same setup as the existing oak_containers_hello_world_untrusted_app test, except with a different container bundle.
env_logger::init(); | ||
}); | ||
} | ||
|
||
#[tokio::test(flavor = "multi_thread", worker_threads = 4)] | ||
async fn hello_world() { | ||
static INIT_DEPENDENCIES: Lazy<anyhow::Result<()>> = Lazy::new(|| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure this is working as intended. At least on my machine (cargo nextest run --package=oak_containers_hello_world_untrusted_app
), if I run the tests in this package, the dependencies get built twice in parallel (which fails because of some race condition in the output files). Not sure why though, maybe the tests are running on different processes and not sharing the lock?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indeed, nextest runs them separately, which means each of them will still try to rebuild all the dependencies: https://nexte.st/book/usage.html?highlight=process#limitations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this rule (+ rules_oci), it's possible to build deterministic/reproducible runtime bundles entirely using bazel. The new rule only supports x86_64 Linux because it uses a pre-compiled umoci binary; if other build environments are required later, additional toolchains can be added.
To demonstrate the new rule, a C++ implementation of the Oak Containers Hello World trusted app has been added. This implementation matches the existing Rust implementation and is verified by the existing Rust Hello World integration test. Note that
oak_containers_launcher::Args
member visibility needed to be changed so that the unit test could specify a container bundle path.