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

Hi Bruce,

Thank you for the comments.  Here follows responses:

> The big one is do_filter_minijail_cargo_config. The problem is real but
> the workaround feels heavier than it needs to be — and I think we can
> delete the task entirely with a one-character change to SRC_URI.
>
> What's happening today: cargo_common_do_patch_paths() walks SRC_URI for
> git entries with name= set, and for each one appends a [patch.crates-io]
> entry to ${CARGO_HOME}/config.toml keyed on the name= value. So name=
> minijail produces
>
>     [patch.crates-io]
>     minijail = { path = "${UNPACKDIR}/git/third_party/minijail" }
>
> But crosvm's own upstream Cargo.toml already has
>
>     [patch.crates-io]
>     minijail = { path = "./third_party/minijail" }
>
> Two entries, same key. Cargo doesn't allow duplicate [patch.crates-io]
> keys regardless of whether the paths resolve to the same directory —
> so the build breaks and the sed task exists to delete the auto-injected
> line.
>
> If we rename the SRC_URI name= to anything that isn't "minijail", the
> collision disappears:
>
>     SRC_URI = " \
>         git://github.com/google/crosvm.git;branch=main;protocol=https;name=crosvm \
>         git://github.com/google/minijail.git;branch=main;protocol=https;name=minijail_src;destsuffix=${BB_GIT_DEFAULT_DESTSUFFIX}/third_party/minijail \
>     "
>
>     SRCREV_crosvm       = "..."
>     SRCREV_minijail_src = "..."
>     SRCREV_FORMAT       = "crosvm_minijail_src"
>
> The unpacked source still lands at the same place on disk (destsuffix
> is what controls that), so crosvm's own Cargo.toml relative-path
> patch still finds it and the build consumes the source as before.
> The only change is that cargo_common now writes
>
>     minijail_src = { path = "..." }
>
> into config.toml — an entry keyed on a crate name nothing in the
> dep graph references. Cargo will emit a non-fatal warning along the
> lines of
>
>     warning: Patch `minijail_src v...` was not used in the crate graph
>
> That warning is expected and fine — please don't try to suppress it.
> It's the price of having two patch tables (upstream's and OE-core's)
> both pointing at the same on-disk source from different angles, and
> making them silently co-exist is better than fighting either one.
>
> With that, the entire do_filter_minijail_cargo_config task and its
> addtask line can go away. A one-line comment above the SRC_URI noting
> "name= is deliberately not 'minijail' so cargo_common's auto-injected
> [patch.crates-io] entry doesn't collide with crosvm's own Cargo.toml,
> which already patches minijail at ./third_party/minijail" would help
> the next person who looks at this.

How did you get it to compile ? Is some information/steps/code missing ?
I tried the change and it errored out: The lack of config.toml causes cargo to error out.
The destination has been specified so that the relative path entry for the patch
mentioned inside crosvm cargo.toml can take effect. The one inside is as follows:

>> [patch.crates-io]
>> audio_streams = { path = "common/audio_streams" }
>> libcras = { path = "libcras_stub" } # ignored by ebuild
>> minijail = { path = "third_party/minijail/rust/minijail" } # ignored by ebuild

Reference: https://github.com/google/crosvm/blob/b04c13a65b93034010b20612e3566d4f9d83c4f0/Cargo.toml#L647-L650

However the entry produced by my build is:

[patch."https://github.com/google/minijail.git"]
minijail_src = { path = "/local/mnt/workspace/kraghava/upstream-crossvm/build-qemuarm64/tmp/work/cortexa57-oe-linux/crosvm/0.1.0+git/sources/crosvm-0.1.0+git/third_party/minijail" }

The src path terminates at "${UNPACKDIR}/git/third_party/minijail" without nesting into
"rust/minijail" which contains the Cargo.toml for the minijail. This cases cargo error.

Did you use a combination of "destsuffix" (and),(or) "subdir" ?
"subdir" causes bitbake to put the repo in a different dir that does not satisy the
relative need of the crosvm Cargo.toml.

>  - LIC_FILES_CHKSUM points at ${COMMON_LICENSE_DIR}/BSD-3-Clause-Clear
>     rather than the LICENSE file in the cloned source. The Yocto-shipped
>     template is a generic copy; an edit to upstream's LICENSE (year
>     bump, contributor line) won't trip do_populate_lic the way it
>     should. Better as file://LICENSE;md5=<sum-from-actual-source>.

Ack. It will be addresed.

>  * BBCLASSEXTEND = "native" — I'd prefer we not have it in this
>       first version. Do a runqemu alternative as a separate series
>       and do it then.
>

Ack. It will be addresed.

> * DEPENDS includes wayland/wayland-native/wayland-protocols
>       unconditionally. Is crosvm always built with the GPU/display
>       feature on here? If those feature-bits can be disabled, please
>       consider a PACKAGECONFIG so wayland is only pulled in when the
>       display path is actually wanted. Acceptable to defer for now,
>       but please call it out as a follow-up.

It will be done as a follow up. I will add an explicit TODO
comment to map cargo feature flags to pkgconfig flags.
It is defered for now in this patch series.

>  * 966 pinned crates and a single BSD-3-Clause-Clear LICENSE
>       declaration — there is almost certainly licence variation in
>       that set. This isn't unique to crosvm (other recipes in the
>       layer are guilty too and I'm working through them), so it
>       isn't blocking, but please file/track it as a follow-up.

I will add an explicit TODO comment for this.

Thank you,
Keerthivasan Raghavan