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