Re: [RFC PATCH v1 0/6] Implement ARM PL011 in Rust

[Manos Pitsidianakis <manos.pitsidianakis@linaro.org>]

On Tue, 11 Jun 2024 14:09, "Daniel P. Berrangé" <berrange@redhat.com> wrote:
>On Tue, Jun 11, 2024 at 01:58:10PM +0300, Manos Pitsidianakis wrote:
>> On Tue, 11 Jun 2024 13:57, "Daniel P. Berrangé" <berrange@redhat.com> wrote:
>> > On Mon, Jun 10, 2024 at 11:29:36PM +0300, Manos Pitsidianakis wrote:
>> > > On Mon, 10 Jun 2024 22:37, Pierrick Bouvier <pierrick.bouvier@linaro.org> wrote:
>> > > > Hello Manos,
>> > > > > On 6/10/24 11:22, Manos Pitsidianakis wrote:
>> > > > > Hello everyone,
>> > > > > > > This is an early draft of my work on implementing a very
>> > > simple device,
>> > > > > in this case the ARM PL011 (which in C code resides in hw/char/pl011.c
>> > > > > and is used in hw/arm/virt.c).
>> > > > > > > The device is functional, with copied logic from the C code
>> > > but with
>> > > > > effort not to make a direct C to Rust translation. In other words, do
>> > > > > not write Rust as a C developer would.
>> > > > > > > That goal is not complete but a best-effort case. To give a
>> > > specific
>> > > > > example, register values are typed but interrupt bit flags are not (but
>> > > > > could be). I will leave such minutiae for later iterations.
>> > 
>> > snip
>> > 
>> > > > Maybe it could be better if build.rs file was *not* needed for new
>> > > > devices/folders, and could be abstracted as a detail of the python
>> > > > wrapper script instead of something that should be committed.
>> > > 
>> > > 
>> > > That'd mean you cannot work on the rust files with a LanguageServer, you
>> > > cannot run cargo build or cargo check or cargo clippy, etc. That's why I
>> > > left the alternative choice of including a manually generated bindings file
>> > > (generated.rs.inc)
>> > 
>> > I would not expect QEMU developers to be running 'cargo <anything>'
>> > directly at all.
>> > 
>> > QEMU's build system is 'meson' + 'ninja' with a 'configure' + 'make'
>> > convenience facade.
>> > 
>> > Any use of 'cargo' would be an internal impl detail of meson rules
>> > for building rust code, and developers should still exclusively work
>> > with 'make' or 'ninja' to run builds & tests.
>> 
>> No, that's not true. If I wrote the pl011 device with this workflow I'd just
>> waste time using meson. Part of the development is making sure the library
>> type checks, compiles, using cargo to run style formatting, to check for
>> lints, perhaps run tests. Doing this only through meson is an unnecessary
>> complication.
>
>I don't see why it should waste time, when we ultimately end up calling
>the same underlying tools. We need to have a consistent experiance for
>developers working on QEMU, not have to use different tools for different
>parts of QEMU depending on whether a piece of code happens to be rust
>or C.

This is a technicality but for the spirit of conversation and curiosity 
I will reply with my view: we end up calling meson and ninja, yes, but 
they wrap cargo under the hood. Cargo has its own tooling and workflows. 
At the end of compilation it spits out a dynamic or static library. That 
happens independently of QEMU.

If you start writing a new project, you don't go linking it to QEMU 
right away (you can, but there's no need to yet). You make an idiomatic 
Rust crate with rust tooling. FFI calls can exist and you are still able 
to compile/check your crate. This is the typical rust workflow, and it's 
ubiquitous, so I think we should not deprive anyone of it.

The only thing we cannot check with cargo alone is if the crate uses the 
QEMU apis correctly (logic errors, not compilation errors)

Until/if we can build with just meson and rustc (a hypothetical scenario 
at the moment) cargo becomes part of our development and build tools.

I understand your argument and it's not irrational at all. I offer this 
perspective as a Rust developer and not a QEMU developer. As a QEMU 
developer I shouldn't need to care about cargo unless I'm touching part 
of that code; the build system details (the cargo wrapper) can be 
abstracted away for everyone else.