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

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

On Friday, 3 April 2020 13:50:09 CEST Richard W.M. Jones
wrote:
> 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?
Should we error out, or simply ignore it like done for other options
specific to a certain mode (eg --host-cpu)?

Your note also made me discover a bug related to that option:

  (on Fedora)
  $ touch filesystem
  $ supermin --prepare -o foo --if-newer -v filesystem
  supermin: version: 5.2.0
  supermin: rpm: detected RPM version 4.15
  supermin: rpm: detected RPM architecture x86_64
  supermin: package handler: fedora/rpm
  supermin: prepare: filesystem
  [etc..]
  $ supermin --prepare -o foo --if-newer -v filesystem
  supermin: version: 5.2.0
  supermin: rpm: detected RPM version 4.15
  supermin: rpm: detected RPM architecture x86_64
  supermin: package handler: fedora/rpm
  supermin: if-newer: output does not need rebuilding

-- 
Pino Toscano

On Fri, Apr 03, 2020 at 04:55:42PM +0200, Pino Toscano
wrote:
> On Friday, 3 April 2020 13:50:09 CEST Richard W.M. Jones wrote:
> > 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?
> 
> Should we error out, or simply ignore it like done for other options
> specific to a certain mode (eg --host-cpu)?
Whichever you think is better.  I suspect if we error out this one,
we'll end up receiving bugs saying that we do the same for all other
options that aren't relevant in the current mode :-/
> Your note also made me discover a bug related to that option:
> 
>   (on Fedora)
>   $ touch filesystem
>   $ supermin --prepare -o foo --if-newer -v filesystem
>   supermin: version: 5.2.0
>   supermin: rpm: detected RPM version 4.15
>   supermin: rpm: detected RPM architecture x86_64
>   supermin: package handler: fedora/rpm
>   supermin: prepare: filesystem
>   [etc..]
>   $ supermin --prepare -o foo --if-newer -v filesystem
>   supermin: version: 5.2.0
>   supermin: rpm: detected RPM version 4.15
>   supermin: rpm: detected RPM architecture x86_64
>   supermin: package handler: fedora/rpm
>   supermin: if-newer: output does not need rebuilding
Yup, that's all wrong (and weird because it's looking at ‘filesystem’
as a file).

Rich.

-- 
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