Re: [PATCH v5 1/2] crosvm: add recipe for ChromeOS Virtual Machine Monitor (VMM)

Hi Bruce,

> The other VMMs in this layer (libvirt, xen, xen-tools) all live under
> recipes-extended/. recipes-devtools/ in meta-virt is mostly go-mod
> tooling (go, go-distribution, etc.). crosvm fits the VMM category, so
> recipes-extended/crosvm/ would match convention better.

> Not strictly blocking, but worth moving in v2 (or whatever the next
> spin is called).

Ack. Will move it to recipes-extended.

> The comment says cargo.bbclass writes a [patches.crates-io] entry to
> ${CARGO_HOME}/config.toml using the SRC_URI path as-is, without
> honouring the "subdir=" in your minijail SRC_URI entry. So cargo tries
> to build a path that has no Cargo.toml and fails.
>
> This concerns me because meta-virt has a number of other cargo
> recipes — netavark, aardvark-dns, podlet, fuse-overlayfs — and none
> of them carry an equivalent workaround. So either:
>
>   a) they all benefit from the same problem somehow (e.g. their
>      SRC_URI entries don't use subdir=), or
>
>   b) they do trigger the same cargo.bbclass behaviour but happen to
>      not break because their secondary repos do contain a Cargo.toml
>      at the SRC_URI root, or
>
>   c) cargo.bbclass has been fixed in oe-core master since this recipe
>      was written, and the workaround is now stale.
>
> Could you dig into the root cause and confirm which of (a/b/c) applies?
> If (c), we can just drop the workaround. If (a) or (b), the fix
> probably belongs in cargo.bbclass upstream — and we'd carry a
> documented local workaround until that lands, rather than a sed-script
> postfunc with no commentary on why it's needed only for crosvm.

I re-checked this in current trees and the result is: (a)+(b), not (c).

  - cargo_common_do_patch_paths() is still active in oe-core and is run from do_configure (cargo_common.bbclass:207, commit
    684a8af41c5).
  - It only creates Cargo [patch."..."] entries for git/gitsm URLs that have both name and destsuffix (cargo_common.bbclass:149-152,
    commits f871d9d6094 + 684a8af41c5), and writes those entries to config.toml (cargo_common.bbclass:165-167, commit 684a8af41c5).
  - crosvm is exactly in that path: it has an extra git dependency for minijail with name=minijail and destsuffix=...
    (crosvm_0.1.0.bb:18, commit 52c76179d), and therefore gets an auto-generated Cargo patch entry. The local sed filter is removing
    that generated entry (crosvm_0.1.0.bb:36-55, commit 52c76179d).
  - netavark, aardvark-dns, and podlet do not use this problematic secondary-git pattern; they use one top-level git source and crat
    es from crate://... includes:
      - netavark_1.17.2.bb:11-13:11, commits 8e45045c8/8c599baea/498a53037
      - netavark-crates.inc:4-6:4, commits 498a53037/8e45045c8
      - aardvark-dns_1.17.0.bb:11-13:11, commit 508517af3/cd6d4550a
      - aardvark-dns-crates.inc:4-6:4, commits cd6d4550a/c08591fd7
      - podlet_git.bb:9-12:9, commit 7ab697991
      - podlet-crates.inc:4-6:4, commit 7ab697991
  - fuse-overlayfs is not a cargo recipe (inherit autotools pkgconfig) (fuse-overlayfs_1.16.bb:13, commit 19a66be0d).

  Additional conflicting observation (important):

  - oe-core commit e955099a2e added subdir handling by appending it to destsuffix in cargo_common (cargo_common.bbclass:157-159,
    commit e955099a2ee).
  - But BitBake git unpack semantics are destdir = join(destdir, subdir) first, then join(destdir, destsuffix) (git.py:670-681,
    commits 66953f06fe, 3e7f8afeac, 25e0b0bc50).
  - So when both are used, cargo path construction and actual unpack layout still disagree.

  Conclusion: until oe-core lands a proper fix in cargo_common.bbclass, keeping the crosvm sed workaround is justified and should
  stay with explicit rationale in the recipe.

subdir is interpreted differently between the two code paths. In BitBake’s git fetcher, unpack destination is built as <UNPACKDIR>/
<subdir>/<destsuffix> (apply subdir first, then append destsuffix) in git.py:670-681. In cargo_common_do_patch_paths(), the
generated Cargo patch path is built as <UNPACKDIR>/<destsuffix>/<subdir> (destsuffix = os.path.join(destsuffix, subdir)) in
cargo_common.bbclass:157-160.
So the same SRC_URI parameters resolve to different filesystem paths, and Cargo [patch] can point to a location different from
where BitBake actually unpacked the repo.

> chromium.googlesource.com is the canonical source. The
> github.com/google/crosvm mirror is typically more reliable from a
> yocto build infrastructure standpoint (the chromium.org hosts have
> periodically been touchy with cgit-style fetches for me in the past).
> Did you consider the github mirror? If there's a reason chromium is
> preferred (e.g. it gets fixes first, or has signed tags), worth a
> one-line comment in the recipe.

Ack. Will be changed to github url.

> crosvm doesn't tag releases, so "0.1.0" is arbitrary. That's fine —
> we have precedent in the layer (kvmtool's PV is similarly cosmetic).
> Just calling it out so we don't get bug reports asking why we're
> shipping a "0.1.0" of a project that has never had a 1.0.

Ack. Comment will be added.

Thank you,
Keerthivasan Raghavan