On Mon, Jun 26, 2023 at 08:02:08AM +0000, Tage Johansson wrote:
> diff --git a/generator/generator.ml b/generator/generator.ml
> index c73824e..18856f5 100644
> --- a/generator/generator.ml
> +++ b/generator/generator.ml
> @@ -61,3 +61,5 @@ let () =
>    output_to "golang/closures.go" GoLang.generate_golang_closures_go;
>    output_to "golang/wrappers.go" GoLang.generate_golang_wrappers_go;
>    output_to "golang/wrappers.h" GoLang.generate_golang_wrappers_h;
> +
> +  output_to ~rustfmt:true "rust/src/bindings.rs" Rust.generate_rust_bindings;
> diff --git a/generator/utils.ml b/generator/utils.ml
> index 3a96929..3a1cc9f 100644
> --- a/generator/utils.ml
> +++ b/generator/utils.ml
> @@ -413,7 +413,7 @@ let files_equal n1 n2 =
>    | 1 -> false
>    | i -> failwithf "%s: failed with error code %d" cmd i
>  
> -let output_to filename k =
> +let output_to ?(rustfmt = false) filename k =

I feel the addition of ~rustfmt might even be a separate patch, and as
discussed previously it could be a generic "~format" parameter taking
various values like RustFmt | GoFmt | Indent.  It would allow this to
go upstream sooner.

>    lineno := 1; col := 0;
>    let filename_new = filename ^ ".new" in
>    let c = open_out filename_new in
> @@ -422,6 +422,13 @@ let output_to filename k =
>    close_out c;
>    chan := NoOutput;
>  
> +  if rustfmt then
> +    (match system (sprintf "rustfmt %s" filename_new) with
> +      | WEXITED 0 -> ()
> +      | WEXITED i -> failwith (sprintf "Rustfmt failed with exit code %d" i)
> +      | _ -> failwith "Rustfmt was killed or stopped by a signal.");
> +
> +

Try to avoid adding extra whitespace lines such as here.

>    (* Is the new file different from the current file? *)
>    if Sys.file_exists filename && files_equal filename filename_new then
>      unlink filename_new                 (* same, so skip it *)
> diff --git a/generator/utils.mli b/generator/utils.mli
> index b4a2525..7489fe0 100644
> --- a/generator/utils.mli
> +++ b/generator/utils.mli
> @@ -50,7 +50,8 @@ val files_equal : string -> string -> bool
>  
>  val generate_header : ?extra_sources:string list -> comment_style -> unit
>  
> -val output_to : string -> (unit -> 'a) -> unit
> +(** Redirect stdout to a file. If `rustfmt` is true, will format the text 
> with rustfmt. *)

And this line is too long.

> +EXTRA_DIST = \
> +     $(generator_built) \
> +     .gitignore \
> +     Cargo.toml \
> +     src/lib.rs \
> +     src/error.rs \
> +     src/handle.rs \
> +     src/types.rs \
> +     src/utils.rs \
> +     libnbd-sys/Cargo.toml \
> +     libnbd-sys/build.rs \
> +     libnbd-sys/wrapper.h \
> +     libnbd-sys/src/lib.rs \
> +     $(NULL)

I think I asked last time why there are two copies of some files, in
this case src/lib.rs.  Are they the same file?

As this is just a file in the tarball, it's not too concerning.  Maybe
Rust has a good reason for it, although I've not seen it need this
before.

If the file was duplicated in git (which I don't think it is) then
it'd be more serious and we'd reject the change.

> +if HAVE_RUST
> +
> +all-local: $(source_files)
> +     rm -f libnbd-sys/libnbd_version.t
> +     $(abs_top_builddir)/run echo $(VERSION) > libnbd-sys/libnbd_version.t
> +     mv libnbd-sys/libnbd_version.t libnbd-sys/libnbd_version

Yes, this is a safer way to generate files atomically from 'make'.

> diff --git a/rust/cargo_test/src/lib.rs b/rust/cargo_test/src/lib.rs
> new file mode 100644
> index 0000000..a5cbb84
> --- /dev/null
> +++ b/rust/cargo_test/src/lib.rs
> @@ -0,0 +1,31 @@
> +// nbd client library in userspace
> +// Copyright Tage Johansson
> +//
> +// This library is free software; you can redistribute it and/or
> +// modify it under the terms of the GNU Lesser General Public
> +// License as published by the Free Software Foundation; either
> +// version 2 of the License, or (at your option) any later version.
> +//
> +// This library 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
> +// Lesser General Public License for more details.
> +//
> +// You should have received a copy of the GNU Lesser General Public
> +// License along with this library; if not, write to the Free Software
> +// Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 
> USA
> +
> +/// A dummy test function which adds one to an 32-bit integer.
> +pub fn add_one(i: i32) -> i32 {
> +    i + 1
> +}
> +
> +#[cfg(test)]
> +mod tests {
> +    use super::*;
> +
> +    #[test]
> +    fn test_add_one() {
> +        assert_eq!(add_one(42), 43);
> +    }
> +}

So I see it's a different file.  I'm not sure why it's needed though.

[I only reviewed down to this point]

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
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Reply via email to