[Libguestfs] [PATCH v8 7/7] Add a virt-builder-repository tool

[Pino Toscano]

On Tuesday, 12 September 2017 09:03:14 CEST 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.
> ---

Since this patch was done, Rich introduced Unicode quotes, so
- '%s'/'...' -> ‘%s’/‘...’
- don't/etc'etera -> don’t/etc’etera
and so on -- mostly in error/etc messages, and documentation

This looks almost like the last version I reviewed; will give a test
run on Monday.

A minor note on the commit message: I'd use
  New tool: virt-builder-repository
which is what was used in the past for other tools (so it makes it
sligtly easier to search for that).

> @@ -101,8 +137,7 @@ virt_builder_CPPFLAGS = \
>  	-I$(shell $(OCAMLC) -where) \
>  	-I$(top_srcdir)/gnulib/lib \
>  	-I$(top_srcdir)/common/utils \
> -	-I$(top_srcdir)/lib \
> -	-I$(top_srcdir)/fish
> +	-I$(top_srcdir)/lib

Extra change, can go in directly if split as own commit.

> +virt_builder_repository_SOURCES = $(REPOSITORY_SOURCES_C)
> +virt_builder_repository_CPPFLAGS = \
> +	-I. \
> +	-I$(top_builddir) \
> +	-I$(top_srcdir)/gnulib/lib -I$(top_builddir)/gnulib/lib \
> +	-I$(shell $(OCAMLC) -where) \
> +	-I$(top_srcdir)/gnulib/lib \
> +	-I$(top_srcdir)/lib \
> +	-I$(top_srcdir)/fish

guestfish is needed here?

> +module StringSet = Set.Make(String)

This is now provided by mlstdutils.

> +let increment_revision = function
> +  | Utils.Rev_int n -> Utils.Rev_int (n + 1)
> +  | Utils.Rev_string s ->
> +    if Str.string_match (Str.regexp "^\\(.*[-._]\\)\\([0-9]+\\)$") s 0 then

There's a PCRE OCaml module now, so maybe it could be a better choice
here (and make the regexp less ugly).

> +let get_mime_type filepath =
> +  let file_cmd = "file --mime-type " ^ (quote filepath) in
> +  let output = List.hd (external_command file_cmd) in
> +  let _, mime = String.split ":" output in
> +  String.trim mime

- passing --brief removes the filename from the output, and thus the
  need to strip it (which will not work in case the file has a colon in
  the name)
- List.hd will raise an exception if the list is empty (so the command
  produced no output for some reason) -- you might need to check it,
  for example like I did in dib/utils.ml:var_from_lines

> +    printf "%s%s%s" message default_str list_str;

I think it might be a good idea to flush stdout before reading the
answer from the user -- just append "%!" at the end of the format
string.

> +      if x == "" then

You just need '=', as '==' is stronger than that (compares also object equality).

> +    (id, { Index.printable_name = printable_name;
> +           osinfo = osinfo;
> +           file_uri = Filename.basename out_path;
> +           arch = arch;
> +           signature_uri = None;
> +           checksums = Some [checksum];
> +           revision = revision;
> +           format = Some format;
> +           size = size;
> +           compressed_size = Some compressed_size;
> +           expand = expand;
> +           lvexpand = lvexpand;
> +           notes = notes;
> +           hidden = hidden;
> +           aliases = aliases;
> +           sigchecker = sigchecker;
> +           proxy = Curl.UnsetProxy })

Most probably it won't change (although I remember a chat on IRC about
different proxy values leading to failures in the unit test added in
patch #4), but better use one (e.g. Curl.SystemProxy) thoroughly.

> +  (* If debugging, echo the command line arguments. *)
> +  debug "command line: %s\n" (String.concat " " (Array.to_list Sys.argv));

debug already prints the newline at the end of the message. (Also in
other occurrences.)

> +  (* Remove the processed image files *)
> +  if not cmdline.no_compression then
> +      List.iter (
> +        fun filename -> Sys.remove (cmdline.repo // filename)
> +      ) images

Indented twice?

> +The file value needs to match the image name extended with the ".xz"

C<.xz>.

> +suffix if the C<--no-compression> parameter is not provided or the

Options with I<>.

> +To remove an image from the repository, just remove the corresponding
> +compressed image file before running virt-builder-repository.

I'd remove "compressed" here, since both options (compressed, and not)
are supported.

Thanks,
-- 
Pino Toscano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part.
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20170915/c897f24f/attachment.sig>