[Libguestfs] [PATCH] Rust bindings: Add Rust bindings

[Hiroyuki Katsura]

Dear Pino,

Thank you for your helpful review. I fixed the patch based on your
comments. I’ll send it later.

> The same also for the various .gitkeep files, as they need to be in the
> distribution tarball to ensure the directories exist.

Is ’src/bin/.gitkeep’ required in EXTRA_DIST? I think because src/bin/
bindtests.rs is included in EXTRA_DIST,  ’src/bin/.gitkeep’ is not required
to make sure the directory exists. Is this idea is correct?


> From what I remember about the Rust naming conventions, maybe the name
> should be guestfs-sys?

>From the previous conversation, I thought it was finally concluded that
 ‘xxxx-sys’ should be used when the crate had only externs of linked
libraries. And, this rust bindings has a little higher abstractions.
Therefore, the name of this crate should not be ‘guestfs-sys.’


I also fixed the license of each file. Could you give me some advice if
there are some mistakes?

Regards,
Hiroyuki

2019年7月27日(土) 1:41 Pino Toscano <ptoscano at redhat.com>:

> Hi Hiroyuki,
>
> sorry for the late reply.
>
> Most of the work is definitely nice! There are few notes below,
> although they are not big issues.  I will check this patch once more
> on monday, especially the rust parts.
>
> Otherwise, I'd say that we are close to merging this :)
>
> On Tuesday, 23 July 2019 10:37:17 CEST Hiroyuki Katsura wrote:
> > From: Hiroyuki_Katsura <hiroyuki.katsura.0513 at gmail.com>
> >
> > Rust bindings: Add create / close functions
> >
> > Rust bindings: Add 4 bindings tests
> >
> > Rust bindings: Add generator of structs
> >
> > Rust bindings: Add generator of structs for optional arguments
> >
> > Rust bindings: Add generator of function signatures
> >
> > Rust bindings: Complete actions
> >
> > Rust bindings: Fix memory management
> >
> > Rust bindings: Add bindtests
> >
> > Rust bindings: Add additional 4 bindings tests
> >
> > Rust bindings: Format test files
> >
> > Rust bindings: Incorporate bindings to build system
> > ---
>
> IMHO there should be a commit message saying that this is new binding
> for Rust and its name, and what is included (actions & tests, not
> events, not examples).
>
> Also, as we talked about, make sure to fix the copyright to properly
> credit yourself.
>
> >  rust/src/.gitkeep                   |   0
>
> This .gitkeep file is not needed, as there are other files in src/.
>
> >  rust/tests/.gitkeep                 |   0
>
> Ditto.
>
> > +(* Utilities for Rust *)
> > +(* Are there corresponding functions to them? *)
> > +(* Should they be placed in utils.ml? *)
>
> Usually we add generic functions to common places only when we know in
> advance that there will be multiple users.  Otherwise, like in this
> case, having utilities only where used is perfectly fine.
> In any case, if any of these utilities will be needed in more places in
> the future, it is easy to do a simple no-op commit to just move some
> code.
>
> > +let rec indent n = match n with
> > +  | x when x > 0 -> pr "    "; indent (x - 1)
> > +  | _ -> ()
>
> A small nit here:
>
> let rec indent = function
>   | x when x > 0 -> pr "    "; indent (x - 1)
>   | _ -> ()
>
> > +
> > +(* split_on_char exists since OCaml 4.04 *)
> > +(* but current requirements: >=4.01 *)
> > +let split_on_char c = Str.split (Str.regexp (String.make 1 c))
>
> The generator uses the internal mlstdutils shared code, see
> common/mlstdutils.  One of the things provided are extra functions for
> the String module, and one in particular can help here: String.nsplit.
> Also...
>
> > +let snake2caml name =
> > +  let l = split_on_char '_' name in
> > +  let l = List.map (fun x -> String.capitalize_ascii x) l in
> > +  String.concat "" l
>
> ... this can be simplified a bit using String.nsplit, and currying:
>
> let snake2caml name =
>   let l = String.nsplit "_" name in
>   let l = List.map String.capitalize_ascii l in
>   String.concat "" l
>
> > +(* because there is a function which contains 'unsafe' field *)
> > +let black_list = ["unsafe"]
> > +
> > +let translate_bad_symbols s =
> > +  if List.exists (fun x -> s = x) black_list then
> > +    s ^ "_"
> > +  else
> > +    s
>
> Hm IMHO the condition in the if sounds like List.mem :) What about:
>
>   if List.mem s black_list then
>
> > +      let cname = snake2caml name in
> > +      let rec contains_ptr args = match args with
> > +        | [] -> false
> > +        | OString _ ::_
> > +        | OStringList _::_ -> true
> > +        | _::xs -> contains_ptr xs
>
> As above, you can avoid the explicit match on the last parameter using
> the function syntax.
>
> OTOH, I think you can use List.exists here:
>
> let contains_ptr =
>   List.exists (
>     function
>     | OString _
>     | OStringList _ -> true
>     | _ -> false
>   )
>
> > diff --git a/rust/Cargo.toml.in b/rust/Cargo.toml.in
> > new file mode 100644
> > index 000000000..e25dfe768
> > --- /dev/null
> > +++ b/rust/Cargo.toml.in
> > @@ -0,0 +1,6 @@
> > +[package]
> > +name = "guestfs"
> > +version = "@VERSION@"
> > +edition = "2018"
>
> From what I remember about the Rust naming conventions, maybe the name
> should be guestfs-sys? Martin?
>
> > +EXTRA_DIST = \
> > +     .gitignore \
> > +     $(generator_built) \
> > +     tests/*.rs \
> > +     Cargo.toml \
> > +     Cargo.lock \
> > +     run-bindtests \
> > +     run-tests
>
> Most probably also src/*.rs, as they are not automake sources (which
> are distributed automatically).
>
> The same also for the various .gitkeep files, as they need to be in the
> distribution tarball to ensure the directories exist.
>
> Also, you need to use TESTS_ENVIRONMENT like done elsewhere, so the
> in-built stuff is used when running the tests.
>
> > diff --git a/rust/run-bindtests b/rust/run-bindtests
> > new file mode 100755
> > index 000000000..55484a2c7
> > --- /dev/null
> > +++ b/rust/run-bindtests
> > @@ -0,0 +1,23 @@
> > +#!/bin/sh -
> > +# libguestfs Rust bindings
> > +# Copyright (C) 2013 Red Hat Inc.
> > +#
> > +# This program is free software; you can redistribute it and/or modify
> > +# it under the terms of the GNU General Public License as published by
> > +# the Free Software Foundation; either version 2 of the License, or
> > +# (at your option) any later version.
> > +#
> > +# This program is distributed in the hope that it will be useful,
> > +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > +# GNU General Public License for more details.
> > +#
> > +# You should have received a copy of the GNU General Public License
> > +# along with this program; if not, write to the Free Software
> > +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
> 02110-1301 USA.
> > +
> > +set -e
> > +
> > +$CARGO run --bin bindtests > bindtests.tmp
>
> Hmm I don't think $CARGO is exported.  A simple way is to make it
> available is to export it as test environment, via TESTS_ENVIRONMENT.
>
> > diff --git a/rust/src/base.rs b/rust/src/base.rs
> > new file mode 100644
> > index 000000000..2dfad91a1
> > --- /dev/null
> > +++ b/rust/src/base.rs
> > @@ -0,0 +1,125 @@
> > +/* libguestfs generated file
> > + * WARNING: THIS FILE IS GENERATED
> > + *          from the code in the generator/ subdirectory.
> > + * ANY CHANGES YOU MAKE TO THIS FILE WILL BE LOST.
>
> Definitely not ;-)
>
> --
> Pino Toscano
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20190729/44bf5d3a/attachment.htm>