diff mbox series

[RFC,v3,13/32] rust: use vendored-sources

Message ID 20210907121943.3498701-14-marcandre.lureau@redhat.com
State New
Headers show
Series Rust binding for QAPI and qemu-ga QMP handler examples | expand

Commit Message

Marc-André Lureau Sept. 7, 2021, 12:19 p.m. UTC
From: Marc-André Lureau <marcandre.lureau@redhat.com>

Most likely, QEMU will want tighter control over the sources, rather
than relying on crates.io downloading, use a git submodule with all the
dependencies. However, cargo --offline was added in 1.36.

"cargo vendor" helps gathering and updating the dependencies.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 configure                 | 8 ++++++++
 meson.build               | 7 ++++++-
 .cargo/config.toml.in     | 5 +++++
 .cargo/meson.build        | 5 +++++
 .gitmodules               | 4 ++++
 rust/vendored             | 1 +
 scripts/archive-source.sh | 2 +-
 scripts/cargo_wrapper.py  | 1 +
 8 files changed, 31 insertions(+), 2 deletions(-)
 create mode 100644 .cargo/config.toml.in
 create mode 100644 .cargo/meson.build
 create mode 160000 rust/vendored

Comments

Ian Jackson Sept. 8, 2021, 3:38 p.m. UTC | #1
marcandre.lureau@redhat.com writes ("[RFC v3 13/32] rust: use vendored-sources"):
> Most likely, QEMU will want tighter control over the sources, rather
> than relying on crates.io downloading, use a git submodule with all the
> dependencies. However, cargo --offline was added in 1.36.

Hi.

pm215 pointed me at this, as I have some background in Rust.
I definitely approve of having Rust in Qemu.  I don't have an opinion
about whether the sources should be vendored this way.

But, I tried to build this, and

    error: failed to select a version for the requirement `cc = "=1.0.70"`
    candidate versions found which didn't match: 1.0.69
    location searched: directory source `/volatile/rustcargo/Rustup/Qemu/qemu.pwt/rust/vendored` (which is replacing registry `crates-io`)
    required by package `nix v0.20.1`
        ... which is depended on by `qga v0.1.0 (/volatile/rustcargo/Rustup/Qemu/qemu.pwt/qga)`
    perhaps a crate was updated and forgotten to be re-vendored?
    As a reminder, you're using offline mode (--offline) which can sometimes cause surprising resolution failures, if this error is too confusing you may wish to retry without the offline flag.

I think the most important part here is to get the general APIs,
presented to general Rust code in Qemu, right.  So I wanted to review
those via the output from rustdoc.

I tried commenting out the `replace-with` in .cargo/config.toml
but evidently the systme isn't intended to be used that way.

Ian.
Marc-André Lureau Sept. 8, 2021, 3:47 p.m. UTC | #2
Hi

On Wed, Sep 8, 2021 at 7:40 PM Ian Jackson <iwj@xenproject.org> wrote:

> marcandre.lureau@redhat.com writes ("[RFC v3 13/32] rust: use
> vendored-sources"):
> > Most likely, QEMU will want tighter control over the sources, rather
> > than relying on crates.io downloading, use a git submodule with all the
> > dependencies. However, cargo --offline was added in 1.36.
>
> Hi.
>
> pm215 pointed me at this, as I have some background in Rust.
> I definitely approve of having Rust in Qemu.  I don't have an opinion
> about whether the sources should be vendored this way.
>
> But, I tried to build this, and
>
>     error: failed to select a version for the requirement `cc = "=1.0.70"`
>     candidate versions found which didn't match: 1.0.69
>     location searched: directory source
> `/volatile/rustcargo/Rustup/Qemu/qemu.pwt/rust/vendored` (which is
> replacing registry `crates-io`)
>     required by package `nix v0.20.1`
>         ... which is depended on by `qga v0.1.0
> (/volatile/rustcargo/Rustup/Qemu/qemu.pwt/qga)`
>     perhaps a crate was updated and forgotten to be re-vendored?
>     As a reminder, you're using offline mode (--offline) which can
> sometimes cause surprising resolution failures, if this error is too
> confusing you may wish to retry without the offline flag.
>
> I think the most important part here is to get the general APIs,
> presented to general Rust code in Qemu, right.  So I wanted to review
> those via the output from rustdoc.
>
> I tried commenting out the `replace-with` in .cargo/config.toml
> but evidently the systme isn't intended to be used that way.
>
> Ian.
>
>
Hmm, I do "cargo vendor --versioned-dirs ../rust/vendored" to vendor crates.

It seems cc was updated, and I didn't update the submodule accordingly. For
reference, this is the dependency tree that WFM:

$ cargo tree -p qga
qga v0.1.0 (/home/elmarco/src/qemu/qga)
├── common v0.1.0 (/home/elmarco/src/qemu/rust/common)
│   ├── libc v0.2.101
│   └── nix v0.20.1
│       ├── bitflags v1.2.1
│       ├── cfg-if v1.0.0
│       ├── libc v0.2.101
│       └── memoffset v0.6.4
│           [build-dependencies]
│           └── autocfg v1.0.1
├── hostname v0.3.1
│   ├── libc v0.2.101
│   └── match_cfg v0.1.0
└── nix v0.20.1 (*)
Ian Jackson Sept. 8, 2021, 3:55 p.m. UTC | #3
Marc-André Lureau writes ("Re: [RFC v3 13/32] rust: use vendored-sources"):
> Hmm, I do "cargo vendor --versioned-dirs ../rust/vendored" to vendor crates.
> 
> It seems cc was updated, and I didn't update the submodule accordingly. For
> reference, this is the dependency tree that WFM:

git submodules are just awful IMO.

> $ cargo tree -p qga                            
> qga v0.1.0 (/home/elmarco/src/qemu/qga)
> ├── common v0.1.0 (/home/elmarco/src/qemu/rust/common)
> │   ├── libc v0.2.101
> │   └── nix v0.20.1
> │       ├── bitflags v1.2.1
> │       ├── cfg-if v1.0.0
> │       ├── libc v0.2.101
> │       └── memoffset v0.6.4
> │           [build-dependencies]
> │           └── autocfg v1.0.1
> ├── hostname v0.3.1
> │   ├── libc v0.2.101
> │   └── match_cfg v0.1.0
> └── nix v0.20.1 (*)

With the .config/cargo.toml "replace-with" commented out, I see this:

rustcargo@zealot:~/Rustup/Qemu/qemu.pwt/build$ cargo tree -p qga
qga v0.1.0 (/volatile/rustcargo/Rustup/Qemu/qemu.pwt/qga)
├── common v0.1.0 (/volatile/rustcargo/Rustup/Qemu/qemu.pwt/rust/common)
│   ├── libc v0.2.101
│   └── nix v0.20.1
│       ├── bitflags v1.2.1
│       ├── cfg-if v1.0.0
│       ├── libc v0.2.101
│       └── memoffset v0.6.4
│           [build-dependencies]
│           └── autocfg v1.0.1
├── hostname v0.3.1
│   ├── libc v0.2.101
│   └── match_cfg v0.1.0
└── nix v0.20.1 (*)
rustcargo@zealot:~/Rustup/Qemu/qemu.pwt/build$ 

Which is the same as yours.  Although "cargo build" doesn't work
build, guessed from the messagese that perhaps this was the automatic
codegen hadn't run.  I'm now trying "make" and and it seems to be
running.

With the "replace-with" uncommented, cargo tree bombs out.  I'm afraid
I haven't used cargo vendor so I'm not sure if I am going in the right
direction with this workaround.  Hopefully it will finish the build.

Would it be possible to have a configure option to use unvendored
upstream Rust libraries from crates.io ?

Ian.
Marc-André Lureau Sept. 8, 2021, 4:15 p.m. UTC | #4
Hi

On Wed, Sep 8, 2021 at 7:55 PM Ian Jackson <iwj@xenproject.org> wrote:

> Marc-André Lureau writes ("Re: [RFC v3 13/32] rust: use vendored-sources"):
> > Hmm, I do "cargo vendor --versioned-dirs ../rust/vendored" to vendor
> crates.
> >
> > It seems cc was updated, and I didn't update the submodule accordingly.
> For
> > reference, this is the dependency tree that WFM:
>
> git submodules are just awful IMO.
>

Yes, but it's often (always?) the user fault. CI should help, when it will
check Rust code.


> > $ cargo tree -p qga
> > qga v0.1.0 (/home/elmarco/src/qemu/qga)
> > ├── common v0.1.0 (/home/elmarco/src/qemu/rust/common)
> > │   ├── libc v0.2.101
> > │   └── nix v0.20.1
> > │       ├── bitflags v1.2.1
> > │       ├── cfg-if v1.0.0
> > │       ├── libc v0.2.101
> > │       └── memoffset v0.6.4
> > │           [build-dependencies]
> > │           └── autocfg v1.0.1
> > ├── hostname v0.3.1
> > │   ├── libc v0.2.101
> > │   └── match_cfg v0.1.0
> > └── nix v0.20.1 (*)
>
> With the .config/cargo.toml "replace-with" commented out, I see this:
>
> rustcargo@zealot:~/Rustup/Qemu/qemu.pwt/build$ cargo tree -p qga
> qga v0.1.0 (/volatile/rustcargo/Rustup/Qemu/qemu.pwt/qga)
> ├── common v0.1.0 (/volatile/rustcargo/Rustup/Qemu/qemu.pwt/rust/common)
> │   ├── libc v0.2.101
> │   └── nix v0.20.1
> │       ├── bitflags v1.2.1
> │       ├── cfg-if v1.0.0
> │       ├── libc v0.2.101
> │       └── memoffset v0.6.4
> │           [build-dependencies]
> │           └── autocfg v1.0.1
> ├── hostname v0.3.1
> │   ├── libc v0.2.101
> │   └── match_cfg v0.1.0
> └── nix v0.20.1 (*)
> rustcargo@zealot:~/Rustup/Qemu/qemu.pwt/build$
>
> Which is the same as yours.  Although "cargo build" doesn't work
> build, guessed from the messagese that perhaps this was the automatic
> codegen hadn't run.  I'm now trying "make" and and it seems to be
> running.
>
> With the "replace-with" uncommented, cargo tree bombs out.  I'm afraid
> I haven't used cargo vendor so I'm not sure if I am going in the right
> direction with this workaround.  Hopefully it will finish the build.
>
> Would it be possible to have a configure option to use unvendored
> upstream Rust libraries from crates.io ?
>

Not easily, but we could have a --disable-rust-offline configure option.
Whether this is desirable, I am not sure.
Marc-André Lureau Sept. 8, 2021, 4:20 p.m. UTC | #5
Hi

On Wed, Sep 8, 2021 at 7:40 PM Ian Jackson <iwj@xenproject.org> wrote:

> marcandre.lureau@redhat.com writes ("[RFC v3 13/32] rust: use
> vendored-sources"):
> > Most likely, QEMU will want tighter control over the sources, rather
> > than relying on crates.io downloading, use a git submodule with all the
> > dependencies. However, cargo --offline was added in 1.36.
>
> Hi.
>
> pm215 pointed me at this, as I have some background in Rust.
> I definitely approve of having Rust in Qemu.  I don't have an opinion
> about whether the sources should be vendored this way.
>
> But, I tried to build this, and
>
>     error: failed to select a version for the requirement `cc = "=1.0.70"`
>     candidate versions found which didn't match: 1.0.69
>     location searched: directory source
> `/volatile/rustcargo/Rustup/Qemu/qemu.pwt/rust/vendored` (which is
> replacing registry `crates-io`)
>     required by package `nix v0.20.1`
>         ... which is depended on by `qga v0.1.0
> (/volatile/rustcargo/Rustup/Qemu/qemu.pwt/qga)`
>     perhaps a crate was updated and forgotten to be re-vendored?
>     As a reminder, you're using offline mode (--offline) which can
> sometimes cause surprising resolution failures, if this error is too
> confusing you may wish to retry without the offline flag.
>
> I think the most important part here is to get the general APIs,
> presented to general Rust code in Qemu, right.  So I wanted to review
> those via the output from rustdoc.
>

You can start by reading `cargo doc -p common --open`. The generated code
needs some environment variables set, so `cargo doc -p qga` will fail
unless you set the environment variable

MESON_BUILD_ROOT=`pwd` cargo doc -p qga --open --document-private-items

works, but the QAPI types aren't documented, so this is a bit useless at
this point. I wonder if I could put the schema doc, hmm...


> I tried commenting out the `replace-with` in .cargo/config.toml
> but evidently the systme isn't intended to be used that way.
>
> Ian.
>
>
Peter Maydell Sept. 8, 2021, 4:22 p.m. UTC | #6
On Wed, 8 Sept 2021 at 17:17, Marc-André Lureau
<marcandre.lureau@gmail.com> wrote:
>
> Hi
>
> On Wed, Sep 8, 2021 at 7:55 PM Ian Jackson <iwj@xenproject.org> wrote:
>>
>> Marc-André Lureau writes ("Re: [RFC v3 13/32] rust: use vendored-sources"):
>> > Hmm, I do "cargo vendor --versioned-dirs ../rust/vendored" to vendor crates.
>> >
>> > It seems cc was updated, and I didn't update the submodule accordingly. For
>> > reference, this is the dependency tree that WFM:
>>
>> git submodules are just awful IMO.
>
>
> Yes, but it's often (always?) the user fault.

I tend to agree with Ian -- submodules are badly designed, and
have lots of sharp edges that it's easy to cut yourself on.
Yes, you can say "well, the user should have held it by the other handle
because that one isn't fitted with the spring-loaded razorblades", but I
would argue that fault is better placed at the door of the designer in
that kind of situation...

-- PMM
Ian Jackson Sept. 8, 2021, 4:22 p.m. UTC | #7
Marc-André Lureau writes ("Re: [RFC v3 13/32] rust: use vendored-sources"):
> On Wed, Sep 8, 2021 at 7:55 PM Ian Jackson <iwj@xenproject.org> wrote:
>  >   git submodules are just awful IMO.
> 
> Yes, but it's often (always?) the user fault.

I must disagree in the strongest possible terms.  I don't think I can
express my feelings on this in a way that would be appropriate in this
context.

Anyway...

My trickery as described above (run configure, edit the "replace-with"
out of .cargo/config.toml, run make) did produce a build.

But to review the internal API I want the rustdoc output.

How can I run rustdoc in a way that will work ?  I tried "cargo doc"
and it complained about a lack of "MESON_BUILD_ROOT".  I guessed and
ran
  MESON_BUILD_ROOT=$PWD cargo doc
which seemed to produce some output and complete but I can't find the
results anywhere.

Can you please give me the set of runes to type view the rustdoc-built
API documentation for the qemu-internal Rust APIs ?

Thanks,
Ian.
Ian Jackson Sept. 8, 2021, 4:29 p.m. UTC | #8
Marc-André Lureau writes ("Re: [RFC v3 13/32] rust: use vendored-sources"):
> You can start by reading `cargo doc -p common --open`. The generated
> code needs some environment variables set, so `cargo doc -p qga`
> will fail unless you set the environment variable
> 
> MESON_BUILD_ROOT=`pwd` cargo doc -p qga --open --document-private-items

Thanks.  I did this (and your rune from bofere) and I have the docs
open.

I wasn't quite sure where to start.  I didn't see where the
entrypoints were.  I did find

 .../target/doc/qga/qmp/fn.qmp_guest_set_vcpus.html

which err, doesn't look like the kind of safe api I was hoping to
find.

Ian.
Marc-André Lureau Sept. 8, 2021, 4:34 p.m. UTC | #9
Hi

On Wed, Sep 8, 2021 at 8:29 PM Ian Jackson <iwj@xenproject.org> wrote:

> Marc-André Lureau writes ("Re: [RFC v3 13/32] rust: use vendored-sources"):
> > You can start by reading `cargo doc -p common --open`. The generated
> > code needs some environment variables set, so `cargo doc -p qga`
> > will fail unless you set the environment variable
> >
> > MESON_BUILD_ROOT=`pwd` cargo doc -p qga --open --document-private-items
>
> Thanks.  I did this (and your rune from bofere) and I have the docs
> open.
>
> I wasn't quite sure where to start.  I didn't see where the
> entrypoints were.  I did find
>
>  .../target/doc/qga/qmp/fn.qmp_guest_set_vcpus.html
>
> which err, doesn't look like the kind of safe api I was hoping to
> find.
>

Yes, this is the shim to provide a C ABI QMP handler from Rust. This is
where all the FFI<->Rust conversion takes place.

The "safe" code is qga/qmp/vcpus.rs. However, there is no documentation
there, since it's not meant to be the public interface. It's documented
with the QAPI schema.
Ian Jackson Sept. 8, 2021, 4:50 p.m. UTC | #10
Marc-André Lureau writes ("Re: [RFC v3 13/32] rust: use vendored-sources"):
> Yes, this is the shim to provide a C ABI QMP handler from Rust. This is where
> all the FFI<->Rust conversion takes place.
> 
> The "safe" code is qga/qmp/vcpus.rs. However, there is no
> documentation there, since it's not meant to be the public
> interface. It's documented with the QAPI schema.

Right, thanks.  That does look like a PoC of a Rust API.  I wanted the
rustdoc output because I find it provides a very uniform and readable
presentation even of an API with no doc comments.

I think maybe a thing I am missing is how you expect this to be used.
Which parts of the system are going to be in Rust.  etc.
And that would help explain what "public" means.

I think the answer is probably in this example:

https://patchew.org/QEMU/20210907121943.3498701-1-marcandre.lureau@redhat.com/20210907121943.3498701-30-marcandre.lureau@redhat.com/

but although my C and Rust are both fine, I don't understand qemu well
enough to make sense of it.

... wait, qga is "qemu guest agent" ?

I think I am sort of seeing this use case now.  But presuambly there
are other use cases for this QMP/QAPI type bridge stuff.

Sorry to be asking such stupid questions.

Thanks,
Ian.
Marc-André Lureau Sept. 8, 2021, 7:33 p.m. UTC | #11
Hi

On Wed, Sep 8, 2021 at 8:51 PM Ian Jackson <iwj@xenproject.org> wrote:

> Marc-André Lureau writes ("Re: [RFC v3 13/32] rust: use vendored-sources"):
> > Yes, this is the shim to provide a C ABI QMP handler from Rust. This is
> where
> > all the FFI<->Rust conversion takes place.
> >
> > The "safe" code is qga/qmp/vcpus.rs. However, there is no
> > documentation there, since it's not meant to be the public
> > interface. It's documented with the QAPI schema.
>
> Right, thanks.  That does look like a PoC of a Rust API.  I wanted the
> rustdoc output because I find it provides a very uniform and readable
> presentation even of an API with no doc comments.
>
> I think maybe a thing I am missing is how you expect this to be used.
> Which parts of the system are going to be in Rust.  etc.
> And that would help explain what "public" means.
>
> I think the answer is probably in this example:
>
>
> https://patchew.org/QEMU/20210907121943.3498701-1-marcandre.lureau@redhat.com/20210907121943.3498701-30-marcandre.lureau@redhat.com/
>
> but although my C and Rust are both fine, I don't understand qemu well
> enough to make sense of it.
>
> ... wait, qga is "qemu guest agent" ?
>
> I think I am sort of seeing this use case now.  But presuambly there
> are other use cases for this QMP/QAPI type bridge stuff.
>
> Sorry to be asking such stupid questions.
>

There is no magic wand to introduce Rust code in an existing C code base.
You need to glue some C ABI to/from Rust. It's a lot of manual work to
properly bind a C API to Rust (it's a project on its own I would say).
Typically, FFI bindings can be automated from headers, and high-level Rust
bindings are done by hand. Then you want high-level bindings to take
advantage of Rust, for idiomatic and safe code. Various internal QEMU API
will have to be bound by hand to start using them from Rust. An isolated
unit (say a parser, a function) could be rewritten in Rust and a C ABI be
provided without much hassle. But in general, code is quickly
interdependent, or the amount of stuff to rewrite in one go is large and
risky to do it that way.

In the glib/gobject world, the ABI are annotated, and you can automate much
of the high-level binding process (which is amazing, given the complexity
of the APIs, with objects, async methods, signals, properties, .. various
concepts that don't match easily in Rust). To help with this process, they
introduced conversion traits (the ToQemu/FromQemu adapted here), common
interfaces, to help automate and compose complex types (and their own
binding generator).

(Unfortunately) QEMU doesn't use gobject, but it relies heavily on two type
systems of its own: QAPI and QOM. QAPI is actually more of an IDL, which
translates C from/to JSON/QMP and has commands and signals (and is the
protocol used to communicate with qemu or qemu-ga etc). A large part of
QEMU are direct users of the QAPI generated types and functions. It is thus
a good target to generate bindings automatically. As demonstrated at the
end, it allows writing QMP handlers in idiomatic Rust. Since
qemu-guest-agent doesn't have a complex internal state (most commands are
really independent, they could be different programs..!), I started
rewriting some handlers there. It feels relatively straightforward to
rewrite in Rust, and we could imagine a complete rewrite of qemu-ga...
However, it is less of a waste to focus on critical parts or newly added
code instead, imho! Furthermore, the Rust doesn't cover all targets C can
currently target, so we must have some care when deciding a language. (you
can imagine I would encourage anyone to do it in Rust!)

QOM is the QEMU object system and is spread throughout the code. If there
is enough interest for this Rust effort, I plan to look at binding it next
(based on my experience working on GObject bindings). I am afraid we are
not going to have as friendly bindings as what the GNOME team achieved, but
since QOM is a bit simpler, we may find acceptable compromises.

With QOM & QAPI, and manual bindings of internal APIs, I think we could
start writing interesting code in Rust, simple devices, external interfaces
etc.


 Hope that helps clarify a bit the current goals
Peter Maydell Sept. 9, 2021, 4:02 p.m. UTC | #12
On Tue, 7 Sept 2021 at 13:32, <marcandre.lureau@redhat.com> wrote:
>
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Most likely, QEMU will want tighter control over the sources, rather
> than relying on crates.io downloading, use a git submodule with all the
> dependencies. However, cargo --offline was added in 1.36.
>
> "cargo vendor" helps gathering and updating the dependencies.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  configure                 | 8 ++++++++
>  meson.build               | 7 ++++++-
>  .cargo/config.toml.in     | 5 +++++
>  .cargo/meson.build        | 5 +++++
>  .gitmodules               | 4 ++++
>  rust/vendored             | 1 +
>  scripts/archive-source.sh | 2 +-
>  scripts/cargo_wrapper.py  | 1 +
>  8 files changed, 31 insertions(+), 2 deletions(-)
>  create mode 100644 .cargo/config.toml.in
>  create mode 100644 .cargo/meson.build
>  create mode 160000 rust/vendored

So, this is a lot of extra code in a submodule. Historically we've
found that submodules are a colossal pain, and so I think we should
think about whether we really want to have all our rust dependencies
in a submodule forever.

I am definitely only at the beginner stage with Rust, but I think
we should have a discussion about what the different alternative
options are here, and what we want to achieve, so that we know
why we're doing this and what we're gaining from the pain...

For instance, could we instead commit Cargo.lock in git and
use that to nail down specific versions of the dependencies ?

FWIW, the "why submodules" for the C dependencies we ship
like that is basically
 * C doesn't have a package manager, so if we need a dependency that
   distros don't ship then we need to wrap it up and provide it ourselves
 * where we ship binary blobs (guest BIOS etc) we want to also ship
   the source code for those blobs
I think for Rust dependencies those don't really apply.

Overall, I think that to the extent that we can look like a "normal"
user of Rust, that's a good plan. Distros may well want to be able
to do "build against our packaged rust stuff rather than downloading
from crates.io" but I imagine they have machinery for that already;
if we act like most other Rust programs we have better chances of
not breaking that machinery.

We do already effectively do "download code when QEMU is built" --
the makefile invokes scripts/git-submodule-update which pulls
down submodule code. (Thanks to Ian for pointing out this framing
of the question.)

(I'm not personally a fan of the "download everything from crates.io"
Rust ecosystem, but it is what it is, and wishing the Rust world
worked more like a trad Linux-distro-provides-all-your-dependencies
isn't, alas, going to make it so :-))

thanks
-- PMM
Marc-André Lureau Sept. 9, 2021, 4:29 p.m. UTC | #13
Hi

On Thu, Sep 9, 2021 at 8:04 PM Peter Maydell <peter.maydell@linaro.org>
wrote:

> On Tue, 7 Sept 2021 at 13:32, <marcandre.lureau@redhat.com> wrote:
> >
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > Most likely, QEMU will want tighter control over the sources, rather
> > than relying on crates.io downloading, use a git submodule with all the
> > dependencies. However, cargo --offline was added in 1.36.
> >
> > "cargo vendor" helps gathering and updating the dependencies.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  configure                 | 8 ++++++++
> >  meson.build               | 7 ++++++-
> >  .cargo/config.toml.in     | 5 +++++
> >  .cargo/meson.build        | 5 +++++
> >  .gitmodules               | 4 ++++
> >  rust/vendored             | 1 +
> >  scripts/archive-source.sh | 2 +-
> >  scripts/cargo_wrapper.py  | 1 +
> >  8 files changed, 31 insertions(+), 2 deletions(-)
> >  create mode 100644 .cargo/config.toml.in
> >  create mode 100644 .cargo/meson.build
> >  create mode 160000 rust/vendored
>
> So, this is a lot of extra code in a submodule. Historically we've
> found that submodules are a colossal pain, and so I think we should
> think about whether we really want to have all our rust dependencies
> in a submodule forever.
>
> I am definitely only at the beginner stage with Rust, but I think
> we should have a discussion about what the different alternative
> options are here, and what we want to achieve, so that we know
> why we're doing this and what we're gaining from the pain...
>
> For instance, could we instead commit Cargo.lock in git and
> use that to nail down specific versions of the dependencies ?
>
>
Yes, that's the main reason this file exists I think.

FWIW, the "why submodules" for the C dependencies we ship
> like that is basically
>  * C doesn't have a package manager, so if we need a dependency that
>    distros don't ship then we need to wrap it up and provide it ourselves
>

Have we considered meson wrap? I never really looked at it in detail, not
sure if that would work for us.
https://mesonbuild.com/Wrap-dependency-system-manual.html

 * where we ship binary blobs (guest BIOS etc) we want to also ship
>    the source code for those blobs
> I think for Rust dependencies those don't really apply.
>

And we mirror all those dependencies at the same location.


> Overall, I think that to the extent that we can look like a "normal"
> user of Rust, that's a good plan. Distros may well want to be able
> to do "build against our packaged rust stuff rather than downloading
> from crates.io" but I imagine they have machinery for that already;
> if we act like most other Rust programs we have better chances of
> not breaking that machinery.
>

True, at least on Fedora, there is machinery to package "regular" Rust
programs/crates in an automated way.  Vendoring dependencies should work
equally, but may not conform with distro policies, so they have extra work
eventually (it seems vendoring is more and more common though, with go
projects for example)


> We do already effectively do "download code when QEMU is built" --
> the makefile invokes scripts/git-submodule-update which pulls
> down submodule code. (Thanks to Ian for pointing out this framing
> of the question.)
>
> (I'm not personally a fan of the "download everything from crates.io"
> Rust ecosystem, but it is what it is, and wishing the Rust world
> worked more like a trad Linux-distro-provides-all-your-dependencies
> isn't, alas, going to make it so :-))
>
>
A nice alternative to vendoring that could work well for QEMU is to mirror
the Rust crate we use, so we have similar control and guarantee over them
as submodules, and use `[patch.crates-io]` to point at qemu-project
locations.
Daniel P. Berrangé Sept. 9, 2021, 4:49 p.m. UTC | #14
On Thu, Sep 09, 2021 at 05:02:01PM +0100, Peter Maydell wrote:
> On Tue, 7 Sept 2021 at 13:32, <marcandre.lureau@redhat.com> wrote:
> >
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > Most likely, QEMU will want tighter control over the sources, rather
> > than relying on crates.io downloading, use a git submodule with all the
> > dependencies. However, cargo --offline was added in 1.36.
> >
> > "cargo vendor" helps gathering and updating the dependencies.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  configure                 | 8 ++++++++
> >  meson.build               | 7 ++++++-
> >  .cargo/config.toml.in     | 5 +++++
> >  .cargo/meson.build        | 5 +++++
> >  .gitmodules               | 4 ++++
> >  rust/vendored             | 1 +
> >  scripts/archive-source.sh | 2 +-
> >  scripts/cargo_wrapper.py  | 1 +
> >  8 files changed, 31 insertions(+), 2 deletions(-)
> >  create mode 100644 .cargo/config.toml.in
> >  create mode 100644 .cargo/meson.build
> >  create mode 160000 rust/vendored
> 
> So, this is a lot of extra code in a submodule. Historically we've
> found that submodules are a colossal pain, and so I think we should
> think about whether we really want to have all our rust dependencies
> in a submodule forever.
> 
> I am definitely only at the beginner stage with Rust, but I think
> we should have a discussion about what the different alternative
> options are here, and what we want to achieve, so that we know
> why we're doing this and what we're gaining from the pain...
> 
> For instance, could we instead commit Cargo.lock in git and
> use that to nail down specific versions of the dependencies ?
> 
> FWIW, the "why submodules" for the C dependencies we ship
> like that is basically
>  * C doesn't have a package manager, so if we need a dependency that
>    distros don't ship then we need to wrap it up and provide it ourselves
>  * where we ship binary blobs (guest BIOS etc) we want to also ship
>    the source code for those blobs
> I think for Rust dependencies those don't really apply.

Even for our existing non-rust usage of submodules, it is very
borderline questionable whether the benefit outweighs the pain
they are causing us. I frequently wish we would just go "cold
turkey" and drop all our submodules and spin off all the bundled
blobs into separate downloads, despite the disruption it would
cause in the short term.

> Overall, I think that to the extent that we can look like a "normal"
> user of Rust, that's a good plan. Distros may well want to be able
> to do "build against our packaged rust stuff rather than downloading
> from crates.io" but I imagine they have machinery for that already;
> if we act like most other Rust programs we have better chances of
> not breaking that machinery.

Yes, distros do have machinery for this, although it is often
hard to fit in with it when you have a mixed language project.
Their machinery typically assumes pure single language project,
so would work nicer if any QEMU rust pieces were separately
released from the rest of QEMU. Obviously this is easier said
than done since QEMU tends towards a monolothic repo approach
historically.

> We do already effectively do "download code when QEMU is built" --
> the makefile invokes scripts/git-submodule-update which pulls
> down submodule code. (Thanks to Ian for pointing out this framing
> of the question.)
> 
> (I'm not personally a fan of the "download everything from crates.io"
> Rust ecosystem, but it is what it is, and wishing the Rust world
> worked more like a trad Linux-distro-provides-all-your-dependencies
> isn't, alas, going to make it so :-))

Yes, I'm inclined to agree here. For better or worse the battle is
over and "download everything from <repo> on the fly" is the accepted
approach for pretty much all modern languages. The language specific
repo essentially is the OS distro from their POV.

Regards,
Daniel
Daniel P. Berrangé Sept. 9, 2021, 4:53 p.m. UTC | #15
On Thu, Sep 09, 2021 at 08:29:58PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Thu, Sep 9, 2021 at 8:04 PM Peter Maydell <peter.maydell@linaro.org>
> wrote:
> 
> > On Tue, 7 Sept 2021 at 13:32, <marcandre.lureau@redhat.com> wrote:
> > >
> > > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > >
> > > Most likely, QEMU will want tighter control over the sources, rather
> > > than relying on crates.io downloading, use a git submodule with all the
> > > dependencies. However, cargo --offline was added in 1.36.
> > >
> > > "cargo vendor" helps gathering and updating the dependencies.
> > >
> > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > ---
> > >  configure                 | 8 ++++++++
> > >  meson.build               | 7 ++++++-
> > >  .cargo/config.toml.in     | 5 +++++
> > >  .cargo/meson.build        | 5 +++++
> > >  .gitmodules               | 4 ++++
> > >  rust/vendored             | 1 +
> > >  scripts/archive-source.sh | 2 +-
> > >  scripts/cargo_wrapper.py  | 1 +
> > >  8 files changed, 31 insertions(+), 2 deletions(-)
> > >  create mode 100644 .cargo/config.toml.in
> > >  create mode 100644 .cargo/meson.build
> > >  create mode 160000 rust/vendored

> > Overall, I think that to the extent that we can look like a "normal"
> > user of Rust, that's a good plan. Distros may well want to be able
> > to do "build against our packaged rust stuff rather than downloading
> > from crates.io" but I imagine they have machinery for that already;
> > if we act like most other Rust programs we have better chances of
> > not breaking that machinery.
> >
> 
> True, at least on Fedora, there is machinery to package "regular" Rust
> programs/crates in an automated way.  Vendoring dependencies should work
> equally, but may not conform with distro policies, so they have extra work
> eventually (it seems vendoring is more and more common though, with go
> projects for example)

I wouldn't assume that we're going to be able to use that RPM support
for rust, if we bundle our rust code inside the QEMU tarball and hidden
behind meson. It generally only works well in single language projects
using the preferred build tool exclusively (Cargo in this case).

> > (I'm not personally a fan of the "download everything from crates.io"
> > Rust ecosystem, but it is what it is, and wishing the Rust world
> > worked more like a trad Linux-distro-provides-all-your-dependencies
> > isn't, alas, going to make it so :-))
> >
> >
> A nice alternative to vendoring that could work well for QEMU is to mirror
> the Rust crate we use, so we have similar control and guarantee over them
> as submodules, and use `[patch.crates-io]` to point at qemu-project
> locations.

The Cargo metadata specifies the versions we'll get so we have full
control over what deps are pulled in. All the mirroring our do is to
cope with occassions where the main crate download website is
inaccessible for some reason. I'm not convinced that's enough to
justify creating extra work for ourselves through mirroring. 

Regards,
Daniel
Ian Jackson Sept. 9, 2021, 5:02 p.m. UTC | #16
Daniel P. Berrangé writes ("Re: [RFC v3 13/32] rust: use vendored-sources"):
> Yes, distros do have machinery for this, although it is often
> hard to fit in with it when you have a mixed language project.
> Their machinery typically assumes pure single language project,
> so would work nicer if any QEMU rust pieces were separately
> released from the rest of QEMU. Obviously this is easier said
> than done since QEMU tends towards a monolothic repo approach
> historically.

Right.

However, for a project that has Rust dependencies, the distros will
(or will soon need) machinery to divert the
langage-specific-package-manager downloads to their own repo.  For
example, the Debian Rust team provide a .cargo/config.toml to replace
crates.io with the local Debian Rust packages, which the Debian
package management system has provided via the (translated)
build-dependencies.

Debian certainly wouldn't want to use any vendored crates bundled with
Qemu.  Indeed Debian people hate vendoring more than they hate
language specific package managers.  At least with the LSPM you can
usually nobble it in one place - ie many of the problems can be solved
automatically.  Vendoring typically involves playing whack-a-mole with
compatibility problems, actually-modified versions, etc. - much human
work (and quite annoying work too!)  (Of course this is less of an
issue if you don't actually modify the vendored code, but anyone who
knows Rust and sees a vendored Rust crate will think it's been
modified.)

> > (I'm not personally a fan of the "download everything from crates.io"
> > Rust ecosystem, but it is what it is, and wishing the Rust world
> > worked more like a trad Linux-distro-provides-all-your-dependencies
> > isn't, alas, going to make it so :-))
> 
> Yes, I'm inclined to agree here. For better or worse the battle is
> over and "download everything from <repo> on the fly" is the accepted
> approach for pretty much all modern languages. The language specific
> repo essentially is the OS distro from their POV.

To be honest, I am of the same mind as you about cargo and crates.io
(and things like it) But I think the ship has sailed.

At least, committing the Cargo.lock file will ensure that the same
versions of the dependencies are used - at least by people who don't
know anything much about Rust.

Marc-André Lureau writes ("Re: [RFC v3 13/32] rust: use vendored-sources"):
> A nice alternative to vendoring that could work well for QEMU is to
> mirror the Rust crate we use, so we have similar control and
> guarantee over them as submodules, and use `[patch.crates-io]` to
> point at qemu-project locations.

This is a very reasonable suggestion.  In my experience crates.io is
very reliable - but (as a test system onwer myself) I know how much
you want to reduce the number of different sites whose upness your CI
depends on, no matter how good their communities think they are :-).

I think there should be a documented config option to disable this.
People who know enough Rust to run `cargo update` etc. will need it,
and having them hand-hack the config files is not really desirable.

Ian.
Ian Jackson Sept. 9, 2021, 5:04 p.m. UTC | #17
Daniel P. Berrangé writes ("Re: [RFC v3 13/32] rust: use vendored-sources"):
> On Thu, Sep 09, 2021 at 08:29:58PM +0400, Marc-André Lureau wrote:
> > True, at least on Fedora, there is machinery to package "regular" Rust
> > programs/crates in an automated way.  Vendoring dependencies should work
> > equally, but may not conform with distro policies, so they have extra work
> > eventually (it seems vendoring is more and more common though, with go
> > projects for example)
> 
> I wouldn't assume that we're going to be able to use that RPM support
> for rust, if we bundle our rust code inside the QEMU tarball and hidden
> behind meson. It generally only works well in single language projects
> using the preferred build tool exclusively (Cargo in this case).

I can't speak to RPM, but Debian's arrangements for avoiding crates.io
downloads at packag-ebuild-time will work just fine even if it
the cargo calls are buried inside meson.  All that would be needed is
a way to disable any source replacement done by qemu (see previous
mail) so that Debian's source replacement takes effect.

Ian.
Paolo Bonzini Sept. 13, 2021, 2:21 p.m. UTC | #18
On 09/09/21 18:29, Marc-André Lureau wrote:
> 
>       * C doesn't have a package manager, so if we need a dependency that
>         distros don't ship then we need to wrap it up and provide it
>     ourselves
> 
> Have we considered meson wrap? I never really looked at it in detail, 
> not sure if that would work for us. 
> https://mesonbuild.com/Wrap-dependency-system-manual.html 
> <https://mesonbuild.com/Wrap-dependency-system-manual.html>

Sure, it would be possible to use wrap with meson 0.56.x or newer (due 
to https://github.com/mesonbuild/meson/pull/7740).  It would all be 
hidden behind configure/Makefile, which would invoke "meson subprojects 
download" in addition to "git submodule update".

Paolo
diff mbox series

Patch

diff --git a/configure b/configure
index 470b90543f..82a94ab93b 100755
--- a/configure
+++ b/configure
@@ -2068,6 +2068,14 @@  if test -z "$ninja"; then
     fi
 fi
 
+case "$with_rust" in
+  auto|enabled)
+    if test -e "${source_path}/.git"; then
+      git_submodules="${git_submodules} rust/vendored"
+    fi
+    ;;
+esac
+
 # Check that the C compiler works. Doing this here before testing
 # the host CPU ensures that we had a valid CC to autodetect the
 # $cpu var (and we should bail right here if that's not the case).
diff --git a/meson.build b/meson.build
index a21c70d77f..29d218d35a 100644
--- a/meson.build
+++ b/meson.build
@@ -108,7 +108,11 @@  endif
 
 bzip2 = find_program('bzip2', required: install_edk2_blobs)
 
-cargo = find_program('cargo', required: get_option('with_rust'))
+cargo = find_program('cargo',
+                     required: get_option('with_rust'),
+                     # require --offline support (1.36), but fixed (1.42)
+                     version: '>= 1.42')
+
 with_rust = cargo.found()
 cargo_wrapper = [
   find_program('scripts/cargo_wrapper.py'),
@@ -126,6 +130,7 @@  if with_rust
       error('cross-compiling, but no Rust target-triple defined.')
     endif
   endif
+  subdir('.cargo')
 endif
 config_host_data.set('CONFIG_WITH_RUST', with_rust)
 
diff --git a/.cargo/config.toml.in b/.cargo/config.toml.in
new file mode 100644
index 0000000000..d1531aa52a
--- /dev/null
+++ b/.cargo/config.toml.in
@@ -0,0 +1,5 @@ 
+[source.crates-io]
+replace-with = "vendored-sources"
+
+[source.vendored-sources]
+directory = "@vendored_directory@"
diff --git a/.cargo/meson.build b/.cargo/meson.build
new file mode 100644
index 0000000000..4e7c296ab0
--- /dev/null
+++ b/.cargo/meson.build
@@ -0,0 +1,5 @@ 
+configure_file(output: 'config.toml',
+               input: 'config.toml.in',
+               configuration: {
+                   'vendored_directory': meson.source_root() / 'rust/vendored'
+               })
diff --git a/.gitmodules b/.gitmodules
index 08b1b48a09..f767a4f386 100644
--- a/.gitmodules
+++ b/.gitmodules
@@ -64,3 +64,7 @@ 
 [submodule "roms/vbootrom"]
 	path = roms/vbootrom
 	url = https://gitlab.com/qemu-project/vbootrom.git
+[submodule "rust/vendored"]
+	path = rust/vendored
+	#url = https://gitlab.com/qemu-project/qemu-rust-vendored.git
+	url = https://github.com/elmarco/qemu-rust-vendored.git
diff --git a/rust/vendored b/rust/vendored
new file mode 160000
index 0000000000..7077bbbd11
--- /dev/null
+++ b/rust/vendored
@@ -0,0 +1 @@ 
+Subproject commit 7077bbbd11a67d60062a9483f996113a349a4ca1
diff --git a/scripts/archive-source.sh b/scripts/archive-source.sh
index c6169db69f..03afcee8b9 100755
--- a/scripts/archive-source.sh
+++ b/scripts/archive-source.sh
@@ -26,7 +26,7 @@  sub_file="${sub_tdir}/submodule.tar"
 # independent of what the developer currently has initialized
 # in their checkout, because the build environment is completely
 # different to the host OS.
-submodules="dtc slirp meson ui/keycodemapdb"
+submodules="dtc slirp meson ui/keycodemapdb rust/vendored"
 submodules="$submodules tests/fp/berkeley-softfloat-3 tests/fp/berkeley-testfloat-3"
 sub_deinit=""
 
diff --git a/scripts/cargo_wrapper.py b/scripts/cargo_wrapper.py
index 75518e8c02..c07f51494b 100644
--- a/scripts/cargo_wrapper.py
+++ b/scripts/cargo_wrapper.py
@@ -78,6 +78,7 @@  def get_cargo_rustc(
         target_dir,
         "--manifest-path",
         manifest_path,
+        "--offline",
     ]
     cargo_cmd += cargo_rustc_args
     if args.target_triple: