Libguestfs - Aug 2023 - [libnbd PATCH v7 0/9] Rust Bindings for Libnbd

On Thu, Aug 17, 2023 at 04:13:36AM +0000, Tage Johansson
wrote:
> 
> On 8/16/2023 9:11 PM, Eric Blake wrote:
> > On Thu, Aug 10, 2023 at 11:24:27AM +0000, Tage Johansson wrote:
> > > This is the 7th version of the Rust bindings for Libnbd. It is
more or
> > > less identical to the 6th version without the already merged
patches.
> > > 
> > > Best regards,
> > > Tage
> > > 
> > I still hope to do more review of both merged patches and this one (in
> > part, to learn more about Rust myself).  But my first observation is
> > that you currently have several build failures on different platforms.
> > Here's some CI links summarizing the types of failures I'm
seeing,
> > when I try to turn on --enable-rust as part of our CI coverage:
> 
> 
> All of these errors are due to the Rustc version is too old. At least rustc
> 1.70.0 is required. Is it possible to update rust in the CI machines,
> perhaps with rustup?
There are several options.  A good thing to remember is that both qemu
and libvirt have a written policy on supported development platforms.
For example:
https://www.qemu.org/docs/master/about/build-platforms.html

Since libnbd aims to be interoperable with qemu, it is generally
advisable that we should strive to compile as much as possible on the
same set of platforms as qemu has chosen to be viable for development.
The libvirt-ci project provides 'lcitool' which makes it easy to
create CI runners for all of these supported systems:
https://gitlab.com/libvirt/libvirt-ci

which is how I found the problems in our CI - I'm trying to push my
patch to bump our CI engine to use the latest definitions, including
adding cargo as a build dependency.  I'm fine with saying 'to build
this aspect, you need to install extra software from your distro', but
I'm reluctant to say 'to build this aspect, you need to pull in extra
software from a third-party source not already shipped by your
distro'.

So, as I see it, that leaves us with these options (or some hybrid
combination of them):

1. Declare that we really do require 1.70.0 as our minimum version.
CI should be tweaked to not even attempt Rust bindings on platforms
that don't supply that.  This has some suboptions:

1a. tweak ci/manifest.yml to not pull in cargo on affected systems
(nbdkit already has examples of avoiding Rust bindings on
known-problematic platforms; it would be easy enough to copy that
logic over to libnbd)

1b. tweak configure.ac to probe that rustc is new enough to provide
all features we depend on.  If the Rust environment is too old, refuse
to build Rust bindings on that platform, even if cargo is available as
a program to call.

Note that 1b is better than 1a: both can get the CI green, but only
the latter plays nicely to all other developers and not just the CI
system.

2. Decide that it is worth trying harder to support Rust bindings back
to an older release, based on whatever the CI systems already have.  I
don't know if Debian 12's 0.66.0 is reasonable, but CentOS 8's
1.69.0
seems fairly close to 1.70.0.  But I don't know how easy or hard it
would be to get the same functionality using only the older features.

Note that the CI system also tries on Debian 11, with an even older
cargo 0.47.0, but it passed because configure noted:
checking for cargo... cargo
checking for rustfmt... no
checking if cargo is usable... configure: WARNING: Rust (cargo) is installed but
not usable
no

That's more of a 1b approach, but obviously not quite strong enough to
filter out the other old versions on Debian 12.
> 
> It is possible to add a minimum version requirement in Cargo.toml, I guess
I
> should do that to make the errors a bit more clear.
It's okay if older platforms can't build Rust bindings, but anything
we can do to make it so that configure detects that gracefully up
front (and the rest of the build still succeeds), rather than falling
apart later during make when the missing feature turns into a compiler
error, is a good thing.  And whatever we do, it doesn't hurt if we
amend README to call out the minimum Rust version we are willing to
support.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

On Thu, Aug 17, 2023 at 09:29:47AM -0500, Eric Blake
wrote:
> On Thu, Aug 17, 2023 at 04:13:36AM +0000, Tage Johansson wrote:
> > It is possible to add a minimum version requirement in Cargo.toml, I
guess I
> > should do that to make the errors a bit more clear.
>
> It's okay if older platforms can't build Rust bindings, but
anything
> we can do to make it so that configure detects that gracefully up
> front (and the rest of the build still succeeds), rather than
> falling apart later during make when the missing feature turns into
> a compiler error, is a good thing.  And whatever we do, it doesn't
> hurt if we amend README to call out the minimum Rust version we are
> willing to support.
^ This is the important bit.  ./configure should disable the Rust
bindings if they can't be built (for any reason, but including Rust
being too old).  This avoids people hitting problems later on.  We
don't control the environment where people try to compile libnbd.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW