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