10:08 p.m.
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(a)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(a)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