[Libguestfs] [PATCH v11 6/6] New tool: virt-builder-repository

[Richard W.M. Jones]

On Thu, Oct 05, 2017 at 04:58:30PM +0200, Cédric Bosdonnat wrote:
> virt-builder-repository allows users to easily create or update
> a virt-builder source repository out of disk images. The tool can
> be run in either interactive or automated mode.
> ---
>  .gitignore                          |   3 +
>  builder/Makefile.am                 |  86 +++++-
>  builder/repository_main.ml          | 597 ++++++++++++++++++++++++++++++++++++
>  builder/test-docs.sh                |   2 +
>  builder/virt-builder-repository.pod | 213 +++++++++++++

You will also need to add references to virt-builder-repository
(man page, tools) in at least:

* docs/guestfs-building.pod in the F<builder> section

* builder/virt-builder.pod man page, in the SEE ALSO section and
  anywhere else in that page that may be appropriate

* lib/guestfs.pod & fish/guestfish.pod

* installcheck.sh.in

Does run.in need to be modified to set the new environment
variables?

Also this new tool lacks any kind of test.

[...]
> +REPOSITORY_SOURCES_C = \
> +	index-scan.c \
> +	index-struct.c \
> +	index-parse.c \
> +	index-parser-c.c \
> +	yajl-c.c
> +
> +

^ Two blank lines here.

>  TESTS_ENVIRONMENT = $(top_builddir)/run --test
> @@ -286,7 +368,7 @@ yajl_tests_LINK = \
>  
>  index_parser_tests_DEPENDENCIES = \
>  	$(index_parser_tests_THEOBJECTS) \
> -	../mllib/mllib.$(MLARCHIVE) \
> +	../common/mltools/mltools.$(MLARCHIVE) \

This looks like a general fix?  Should this go into a separate patch?

> +open StringSet

I'm 99% certain that you shouldn't be opening StringSet.  What
did you need that for?

> +  let gpgkey = match !gpgkey with "" -> None | s -> Some s in

Using my proposed, not upstream, Option module, you will be able to
write this as:

  let gpgkey = Option.default "" !gpgkey in

> +  let no_compression = !no_compression in

Double negatives!  Can this be called ‘compression’ instead?

> +  (* Check options *)
> +  let repo =
> +    match args with
> +    | [repo] -> repo
> +    | [] ->
> +      error (f_"virt-builder-repository /path/to/repo\nUse ‘/path/to/repo’ to point to the repository folder.")

OCaml lets you use multiline strings directly:

  | [] ->
     error (f_"Usage: virt-builder-repository /path/to/repo

Use ‘/path/to/repo’ to point to the repository folder.")

> +let increment_revision = function
> +  | Utils.Rev_int n -> Utils.Rev_int (n + 1)
> +  | Utils.Rev_string s -> Utils.Rev_int ((int_of_string s) + 1)

You could probably put this little utility function into builder/utils.ml.

> +let checksums_get_sha512 = function
> +  | None -> Checksums.SHA512 ""
> +  | Some csums ->
> +      try
> +        List.find (
> +          function
> +          | Checksums.SHA512 _ -> true
> +          | _ -> false
> +        ) csums
> +      with Not_found -> Checksums.SHA512 ""

This looks like you're subverting type safety.  There shouldn't be any
case where SHA512 is an empty string.  If you meant "I want to return
nothing from this function", then make the function return an option
type.

> +let osinfo_ids = ref None
> +
> +let osinfo_get_short_ids () =
> +  match !osinfo_ids with
> +  | Some ids -> ids
> +  | None -> (
> +    let set = ref StringSet.empty in
> +    Osinfo.iterate_db (
> +      fun filepath ->
> +        let doc = Xml.parse_file filepath in
> +        let xpathctx = Xml.xpath_new_context doc in
> +        let nodes = xpath_get_nodes xpathctx "/libosinfo/os/short-id" in
> +        List.iter (
> +          fun node ->
> +            let id = Xml.node_as_string node in
> +            set := StringSet.add id !set
> +        ) nodes
> +    );
> +    osinfo_ids := Some (!set);
> +    !set
> +  )

If the Osinfo module implemented a ‘fold’ function (instead of iter)
then you could write this much more conveniently.  It would be
something like:

  let set = Osinfo.fold (fun set filepath -> ...) StringSet.empty in

where the ‘...’ bit non-imperatively updates ‘set’.

> +let compress_to file outdir =

I reviewed up to this function.  Will come back to this later.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org