Libguestfs - Apr 2020 - Re: [supermin PATCH 3/4] Extend modes with list of outputs

This is an attempt to make supermin check for the existing results of an
output when checking whether the appliance must be rebuilt using
--if-newer.

At the moment it is implemented only for the build mode, and for its
ext2 output format.

Pino Toscano (4):
  build: factor ext2 filenames
  Tighten Unix_error check for missing outputdir
  Extend modes with list of outputs
  build: set kernel/initrd/root as outputs (RHBZ#1813809)

 src/mode_build.ml           | 22 ++++++++++++--
 src/mode_build.mli          |  4 +++
 src/mode_prepare.ml         |  9 +++++-
 src/mode_prepare.mli        |  4 +++
 src/supermin.ml             | 11 +++++--
 tests/Makefile.am           |  3 +-
 tests/test-if-newer-ext2.sh | 57 +++++++++++++++++++++++++++++++++++++
 7 files changed, 102 insertions(+), 8 deletions(-)
 create mode 100755 tests/test-if-newer-ext2.sh

-- 
2.25.1

Factor them in own variables to avoid repeating them in following
commits.

This is just refactoring, no behaviour changes.
---
 src/mode_build.ml | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/src/mode_build.ml b/src/mode_build.ml
index 8a06012..d54a3cd 100644
--- a/src/mode_build.ml
+++ b/src/mode_build.ml
@@ -59,6 +59,10 @@ and string_of_file_content = function
   | Excludefiles -> "excludefiles"
   | Empty -> "empty"
 
+let kernel_filename = "kernel"
+and appliance_filename = "root"
+and initrd_filename = "initrd"
+
 let rec build debug
     (copy_kernel, format, host_cpu,
      packager_config, tmpdir, use_installed, size,
@@ -223,9 +227,9 @@ let rec build debug
     Format_chroot.build_chroot debug files outputdir packagelist_file
 
   | Ext2 ->
-    let kernel = outputdir // "kernel"
-    and appliance = outputdir // "root"
-    and initrd = outputdir // "initrd" in
+    let kernel = outputdir // kernel_filename
+    and appliance = outputdir // appliance_filename
+    and initrd = outputdir // initrd_filename in
     let kernel_version, modpath        Format_ext2_kernel.build_kernel debug
host_cpu copy_kernel kernel in
     Format_ext2.build_ext2 debug basedir files modpath kernel_version
-- 
2.25.1

When getting the timestamp of the output directory for the --if-newer
checks, ignore ENOENT instead of any Unix_error: the lack of it is OK,
while any other stat error is most likekly an actual problem.
---
 src/supermin.ml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/supermin.ml b/src/supermin.ml
index 7c7135b3..80c48e6 100644
--- a/src/supermin.ml
+++ b/src/supermin.ml
@@ -245,7 +245,7 @@ appliance automatically.
         exit 0
       )
     with
-      Unix_error _ -> () (* just continue *)
+      Unix_error (ENOENT, _, _) -> () (* just continue *)
   );
 
   (* Create the output directory nearly atomically. *)
-- 
2.25.1

Add a function for each mode to return the list of potential outputs, so
that the existance/timestamp checks done for --if-newer can take those
into accounts.

At the moment both modes return no outputs, so there is no behaviour
change.
---
 src/mode_build.ml    | 7 +++++++
 src/mode_build.mli   | 4 ++++
 src/mode_prepare.ml  | 9 ++++++++-
 src/mode_prepare.mli | 4 ++++
 src/supermin.ml      | 9 +++++++--
 5 files changed, 30 insertions(+), 3 deletions(-)

diff --git a/src/mode_build.ml b/src/mode_build.ml
index d54a3cd..4a58460 100644
--- a/src/mode_build.ml
+++ b/src/mode_build.ml
@@ -462,3 +462,10 @@ and munge files    let files = loop files in
 
   files
+
+and get_outputs
+    (copy_kernel, format, host_cpu,
+     packager_config, tmpdir, use_installed, size,
+     include_packagelist)
+    inputs +  []
diff --git a/src/mode_build.mli b/src/mode_build.mli
index 0f8b956..4fba2ab 100644
--- a/src/mode_build.mli
+++ b/src/mode_build.mli
@@ -21,3 +21,7 @@
 val build : int -> (bool * Types.format * string * string option * string *
bool * int64 option * bool) -> string list -> string -> unit
 (** [build debug (args...) inputs outputdir] performs the
     [supermin --build] subcommand. *)
+
+val get_outputs : (bool * Types.format * string * string option * string * bool
* int64 option * bool) -> string list -> string list
+(** [get_outputs (args...) inputs] gets the potential outputs for the
+    appliance. *)
diff --git a/src/mode_prepare.ml b/src/mode_prepare.ml
index 70f9dd4..e0ad1a8 100644
--- a/src/mode_prepare.ml
+++ b/src/mode_prepare.ml
@@ -21,7 +21,7 @@ open Printf
 open Package_handler
 open Utils
 
-let prepare debug (copy_kernel, format, host_cpu,
+let rec prepare debug (copy_kernel, format, host_cpu,
              packager_config, tmpdir, use_installed, size,
              include_packagelist)
     inputs outputdir @@ -175,3 +175,10 @@ let prepare debug (copy_kernel,
format, host_cpu,
     (* No config files to copy, so do not create base.tar.gz. *)
     if debug >= 1 then printf "supermin: not creating
base.tar.gz\n%!";
   )
+
+and get_outputs
+    (copy_kernel, format, host_cpu,
+     packager_config, tmpdir, use_installed, size,
+     include_packagelist)
+    inputs +  []
diff --git a/src/mode_prepare.mli b/src/mode_prepare.mli
index e2d677a..be45730 100644
--- a/src/mode_prepare.mli
+++ b/src/mode_prepare.mli
@@ -21,3 +21,7 @@
 val prepare : int -> (bool * Types.format * string * string option * string
* bool * int64 option * bool) -> string list -> string -> unit
 (** [prepare debug (args...) inputs outputdir] performs the
     [supermin --prepare] subcommand. *)
+
+val get_outputs : (bool * Types.format * string * string option * string * bool
* int64 option * bool) -> string list -> string list
+(** [get_outputs (args...) inputs] gets the potential outputs for the
+    appliance. *)
diff --git a/src/supermin.ml b/src/supermin.ml
index 80c48e6..4091f1d 100644
--- a/src/supermin.ml
+++ b/src/supermin.ml
@@ -236,10 +236,15 @@ appliance automatically.
    *)
   if if_newer then (
     try
-      let odate = (lstat outputdir).st_mtime in
+      let mode_outputs +        match mode with
+        | Prepare -> Mode_prepare.get_outputs args inputs
+        | Build -> Mode_build.get_outputs args inputs in
+      let mode_outputs = List.map ((//) outputdir) mode_outputs in
+      let odates = List.map (fun d -> (lstat d).st_mtime) (outputdir ::
mode_outputs) in
       let idates = List.map (fun d -> (lstat d).st_mtime) inputs in
       let pdate = (get_package_handler ()).ph_get_package_database_mtime () in
-      if List.for_all (fun idate -> idate < odate) (pdate :: idates) then
(
+      if List.for_all (fun idate -> List.for_all (fun odate -> idate <
odate) odates) (pdate :: idates) then (
         if debug >= 1 then
           printf "supermin: if-newer: output does not need
rebuilding\n%!";
         exit 0
-- 
2.25.1

Return the kernel, initrd, and root files as outputs for this mode when
the output format is ext2, so --if-newer will check whether they exist
or they are old.

Add a simple test to verify this behaviour.
---
 src/mode_build.ml           |  7 ++++-
 tests/Makefile.am           |  3 +-
 tests/test-if-newer-ext2.sh | 57 +++++++++++++++++++++++++++++++++++++
 3 files changed, 65 insertions(+), 2 deletions(-)
 create mode 100755 tests/test-if-newer-ext2.sh

diff --git a/src/mode_build.ml b/src/mode_build.ml
index 4a58460..ed47366 100644
--- a/src/mode_build.ml
+++ b/src/mode_build.ml
@@ -468,4 +468,9 @@ and get_outputs
      packager_config, tmpdir, use_installed, size,
      include_packagelist)
     inputs -  []
+  match format with
+  | Chroot ->
+    (* The content for chroot depends on the packages. *)
+    []
+  | Ext2 ->
+    [kernel_filename; appliance_filename; initrd_filename]
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 42d1b82..070f6d9 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -28,7 +28,8 @@ TESTS = \
 	test-execstack.sh \
 	test-build-bash.sh \
 	test-binaries-exist.sh \
-	test-harder.sh
+	test-harder.sh \
+	test-if-newer-ext2.sh
 
 if NETWORK_TESTS
 TESTS += \
diff --git a/tests/test-if-newer-ext2.sh b/tests/test-if-newer-ext2.sh
new file mode 100755
index 0000000..4f49bda
--- /dev/null
+++ b/tests/test-if-newer-ext2.sh
@@ -0,0 +1,57 @@
+#!/bin/bash -
+# supermin
+# (C) Copyright 2009-2020 Red Hat Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program 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 General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+
+set -e
+
+# XXX Hack for Arch.
+if [ -f /etc/arch-release ]; then
+    export SUPERMIN_KERNEL=/boot/vmlinuz-linux
+fi
+
+tmpdir=`mktemp -d`
+
+d1=$tmpdir/d1
+d2=$tmpdir/d2
+
+# We assume 'bash' is a package everywhere.
+../src/supermin -v --prepare --use-installed bash -o $d1
+
+run_supermin ()
+{
+  ../src/supermin -v --build -f ext2 --if-newer $d1 -o $d2
+}
+
+# Build the appliance the first time, which will work.
+run_supermin
+
+# No changes, hence nothing to do.
+run_supermin | grep 'if-newer: output does not need rebuilding'
+
+# Try removing any of the files, and check that supermin will detect that.
+ext2_files="kernel initrd root"
+for ext2_file in $ext2_files
+do
+  rm $d2/$ext2_file
+  run_supermin
+  for ext2_file in $ext2_files
+  do
+    test -e $d2/$ext2_file
+  done
+done
+
+rm -rf $tmpdir ||:
-- 
2.25.1

On Fri, Apr 03, 2020 at 12:20:57PM +0200, Pino Toscano
wrote:
> Add a function for each mode to return the list of potential outputs, so
> that the existance/timestamp checks done for --if-newer can take those
> into accounts.
> 
> At the moment both modes return no outputs, so there is no behaviour
> change.
Patches 1, 2 and 4 are fine.

I have a minor problem with this patch:
> diff --git a/src/mode_prepare.ml b/src/mode_prepare.ml
> index 70f9dd4..e0ad1a8 100644
> --- a/src/mode_prepare.ml
> +++ b/src/mode_prepare.ml
> @@ -21,7 +21,7 @@ open Printf
>  open Package_handler
>  open Utils
>  
> -let prepare debug (copy_kernel, format, host_cpu,
> +let rec prepare debug (copy_kernel, format, host_cpu,
>               packager_config, tmpdir, use_installed, size,
>               include_packagelist)
>      inputs outputdir > @@ -175,3 +175,10 @@ let prepare debug
(copy_kernel, format, host_cpu,
>      (* No config files to copy, so do not create base.tar.gz. *)
>      if debug >= 1 then printf "supermin: not creating
base.tar.gz\n%!";
>    )
> +
> +and get_outputs
> +    (copy_kernel, format, host_cpu,
> +     packager_config, tmpdir, use_installed, size,
> +     include_packagelist)
> +    inputs > +  []
> diff --git a/src/mode_prepare.mli b/src/mode_prepare.mli
> index e2d677a..be45730 100644
> --- a/src/mode_prepare.mli
> +++ b/src/mode_prepare.mli
> @@ -21,3 +21,7 @@
>  val prepare : int -> (bool * Types.format * string * string option *
string * bool * int64 option * bool) -> string list -> string -> unit
>  (** [prepare debug (args...) inputs outputdir] performs the
>      [supermin --prepare] subcommand. *)
> +
> +val get_outputs : (bool * Types.format * string * string option * string *
bool * int64 option * bool) -> string list -> string list
> +(** [get_outputs (args...) inputs] gets the potential outputs for the
> +    appliance. *)
> diff --git a/src/supermin.ml b/src/supermin.ml
> index 80c48e6..4091f1d 100644
> --- a/src/supermin.ml
> +++ b/src/supermin.ml
> @@ -236,10 +236,15 @@ appliance automatically.
>     *)
>    if if_newer then (
>      try
> -      let odate = (lstat outputdir).st_mtime in
> +      let mode_outputs > +        match mode with
> +        | Prepare -> Mode_prepare.get_outputs args inputs
> +        | Build -> Mode_build.get_outputs args inputs in
We actually document that --if-newer can only be used in build mode.
It sort of makes no sense in prepare mode.  So shouldn't this just
give an error (either here, or earlier) if we're in prepare mode?

Rich.
> +      let mode_outputs = List.map ((//) outputdir) mode_outputs in
> +      let odates = List.map (fun d -> (lstat d).st_mtime) (outputdir ::
mode_outputs) in
>        let idates = List.map (fun d -> (lstat d).st_mtime) inputs in
>        let pdate = (get_package_handler ()).ph_get_package_database_mtime
() in
> -      if List.for_all (fun idate -> idate < odate) (pdate :: idates)
then (
> +      if List.for_all (fun idate -> List.for_all (fun odate -> idate
< odate) odates) (pdate :: idates) then (
>          if debug >= 1 then
>            printf "supermin: if-newer: output does not need
rebuilding\n%!";
>          exit 0
> -- 
> 2.25.1
> 
> _______________________________________________
> Libguestfs mailing list
> Libguestfs@redhat.com
> https://www.redhat.com/mailman/listinfo/libguestfs
-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html