Update to zerocopy v0.8
#57
Conversation
|
If I may ask, can you briefly describe some things that zerocopy 0.8 makes possible with generated interfaces? I seem to recall it having new features that maybe covered some cases that were formerly only possible with Hubpack/Ssmarshal. But not sure which of the new features are actually usable for IPC with zerocopy encoding. |
|
|
The support for enums is interesting, since I was starting to come around to removing the zerocopy encoding for IPCs and standardizing on one of the serde representations. Explaining to people why they couldn't have enums was an awful user experience. So I look forward to doing some testing of the new zerocopy version and seeing if it changes my mind! |
There was a problem hiding this comment.
While I don't think anyone is depending on this externally via Cargo, I would suggest that this merits a version bump to the crates. Since users would need to change which zerocopy traits they're deriving. (Hi @kairoswater-jason -- you are likely using a git dep and will be unaffected.)
runtime/Cargo.toml
Outdated
| counters = { git = "https://github.com/oxidecomputer/hubris" } | ||
| userlib = {git = "https://github.com/oxidecomputer/hubris"} | ||
| zerocopy = "0.6.1" | ||
| zerocopy = { version = "0.8.25", features = ["derive"] } |
There was a problem hiding this comment.
Nit:
this is not how the zerocopy folks currently recommend getting the derive macros. From the crate docs:
However, you may experience better compile times if you instead directly depend on both zerocopy and zerocopy-derive in your Cargo.toml, since doing so will allow Rust to compile these crates in parallel.
There was a problem hiding this comment.
Oh, I had overlooked that in their docs, good catch. I'll change it.
There was a problem hiding this comment.
Turns out idol-runtime doesn't actually use the derives, just the traits. So we can just drop the feature here. However, the codegen will need to be updated if we want the rest of Hubris to derive on the two of them separately.
|
Hello again @cbiffle, Yes I use a forked version that reimplements the Pipelined server style that was removed a while back. I've been putting off updating to zerocopy 0.8 - but this looks like a good opportunity to do so |
It turns out that the re-export of zerocopy doesn't actually help that much, as crates embedding `idol`-generated code must still depend on `zerocopy` explicitly. This is because the `zercopy_derive` macros generate implementations for zerocopy's traits that reference them as `zerocopy::<TRAIT>`, which breaks when there isn't a `zerocopy` crate in scope when the generated code is compiled.
This reverts commit 3f97567. Turns out that actually doesn't work either: I think we just *need* to continue having explicit `zerocopy` deps in embedding crates: ``` error[E0433]: failed to resolve: could not find `zerocopy` in the list of imported crates --> /hubris/target/thumbv7em-none-eabihf/release/build/drv-user-leds-api-8a28786152326190/out/client_stub.rs:215:46 | 215 | #[derive(zerocopy_derive::IntoBytes, zerocopy_derive::Immutable)] | ^^^^^^^^^^^^^^^^^^^^^^^^^^ could not find `zerocopy` in the list of imported crates | = note: this error originates in the derive macro `zerocopy_derive::Immutable` (in Nightly builds, run with -Z macro-backtrace for more info) help: consider importing one of these crates --> drv/user-leds-api/src/lib.rs:9:1 | 9 + use crate::zerocopy; --> |drv/user-leds-api/src/lib.rs:9:1 | 9 + use idol_runtime::zerocopy; ```
|
@cbiffle Unfortunately, the idea we previously discussed of having This issue exists even when the codegen emits the |
|
I'm going to merge this now that Hubris builds happily with it. |
now that oxidecomputer/idolatry#57 has merged
This branch updates our dependency on `zerocopy` from v0.6.x to v0.8.x. The primary motivation for this change is that I had wanted to use `zerocopy` v0.8's support for data-bearing `enum`s in the `gateway-ereport-messages` crate I added in oxidecomputer/management-gateway-service#370, and...I hadn't realized how painful taking the `zerocopy` update in Hubris would be. :) But, it's also just good to stay on top of new dependency versions regardless. This is a _very_ large change, since pretty much every place where we derive or use `zerocopy`'s traits needed to be changed slightly to use the new APIs, but for the most part, it's not actually that *interesting*, so reviewing it should be pertty straightforward. The main API changes that are worth noting are: - `AsBytes` is now called `IntoBytes`, which was an easy update. - All the `_from_prefix`/`_from_suffix` APIs now return the rest of the slice, which is a nice improvement --- previously we would manually split off the rest of the slice when using those functions. - Conversions from bytes now return `Result`s instead of `Option`s, which very trivial changes in a few places. - `LayoutVerified` is replaced by a new `Ref` type, but other API changes mean that you now basically never need to use it, which rocks! - Some methods were renamed, which was also a pretty trivial find-and-replace. - `zerocopy` adds new `Immutable` and `KnownLayout` marker traits. `Immutable` is required for `IntoBytes::as_bytes()`; types which do not derive it are now assumed to have interior mutability and only provide `IntoBytes::as_bytes_mut()`. So, basically everything now needs to derive `Immutable`. `KnownLayout` isn't required for APIs we use as commonly but I added it on everything anyway. This was most of what made this update annoying. - `FromBytes::new_zeroed` is now provided by a new `FromZeros` trait, but the `FromBytes` derive implements that trait as well. There may be some places where we can now make better use of new `zerocopy` features. In particular, we can now use `zerocopy` on data-bearing enums, which might allow us to replace `hubpack` with `zerocopy` in several places. I didn't do that in this PR, but it's worth looking into in follow-up changes --- I just wanted to get everything building with the new API and felt that improving our usage of it would be better off done in smaller commits. One other important thing to note is that updating the `gateway-messages` dependency increased stack depth in `control-plane-agent` from 6000B to 6136B for non-sidecar targets, so I bumped them up to 6256B. This, in turn, increases RAM to 65525B for `control-plane-agent`, which exceeds the `max-sizes` config, per @cbiffle's previous advice, I just deleted all the `max-sizes` for `control-plane-agent` tasks. Furthermore, this branch requires the following changes to other crates to pick up the latest `zerocopy`: - oxidecomputer/management-gateway-service#384 - oxidecomputer/idolatry#57 - oxidecomputer/humpty#8 Those ought to merge first so we can point our Git dependencies on those repos back at the `main` branch.
No description provided.