Re: [PATCH] Do not clear the prev, curr and exp fields of packagemeta. Never.

[Corinna Vinschen <corinna-cygwin at cygwin dot com>]

On May  4 14:38, Houder wrote:
> Hi,
> 
> I have prepared a patch to setup.exe, but I do not know how to submit it.
> For
> this reason. I have attached the result of 'git format-patch' to this
> message.
> 
> Please, submit this patch.
> 
> This patch rectifies
> 
>  - packagemeta::ScanDownloadedFiles (package_meta.cc), and
>  - packagemeta::trustp (package_meta.h)
> 
> Clarification:
> Class packagemeta has "fields" prev, curr, exp and installed, which in fact
> represent the info from setup.ini/installed.db, and should never be cleared
> therefore. _Currently_ these fields are cleared in ScanDownloadedFiles() in
> order to notify that the tarball for the associated field is not available.
> This is a mistake, as it destroys relevant info (e.g. the version number of
> a version).
> To ascertain whether or not the associated tarball is available, one should
> invoke <field>.accesible().
> 
> Note: yes, "field" is type packageversion.

The patch seems to make sense, but I'm currently so busy with
non-Cygwin stuff that I'm not sure I'm looking cross-eyed.
Can somebody hacking on setup have a look, too, please?


Thanks,
Corinna


> 
> Regards,
> henri

> From 3ddf47d9c75a6a547b9121dccf9a6ea3c0e4cd33 Mon Sep 17 00:00:00 2001
> From: Henri <houder@xs4all.nl>
> Date: Wed, 4 May 2016 13:06:34 +0200
> Subject: [PATCH] Do not clear the prev, curr and exp fields of packagemeta.
>  Never.
> 
>  * package_meta.cc (packagemeta::ScanDownloadedFiles): Do _not_ clear the prev,
>    curr or exp "fields" of a package in order to notify that the tarball is not
>    available (accessible). These fields contain relevant info from setup.ini.
>    The same applies for the installed field (from installed.db) of a package.
>    Rather remove these statements from ScanDownloadedFiles(), because they are
>    problably a left over from earlier days.
>    To ascertain whether the tarball, associated with a version, is available or
>    not, the method <version>.accessible() should be invoked.
> 
>  * package_meta.h (packagemeta::trustp): use the accessible() method in order
>    to verify whether or not the associated tarball is available for a version.
>    Note: in order to be able to compare version numbers of different versions,
>    the fields prev, curr, exp and installed should have been initialised from
>    setup.ini and installed.db, and should never have been cleared.
> ---
>  package_meta.cc | 16 ++++++++++++++--
>  package_meta.h  | 21 +++++++++++++++++++--
>  2 files changed, 33 insertions(+), 4 deletions(-)
> 
> diff --git a/package_meta.cc b/package_meta.cc
> index 34ff78c..cc5dc0a 100644
> --- a/package_meta.cc
> +++ b/package_meta.cc
> @@ -683,15 +683,27 @@ packagemeta::ScanDownloadedFiles (bool mirror_mode)
>  	  /* For local installs, if there is no src and no bin, the version
>  	   * is unavailable
>  	   */
> -	  if (!i->accessible () && !pkgsrcver.accessible ()
> -	      && *i != pkg.installed)
> +	  if (*i != pkg.installed
> +	      && !i->accessible () && !pkgsrcver.accessible () )
>  	    {
> +              /*
> +               - remove a version from versions (a set) in case the associated
> +                 tarball is not available (accessible)
> +               - however, do not remove the info that has been collected from
> +                 setup.ini and installed.db, because you might need it to e.g.
> +                 compare the version numbers of different versions
> +               - package_meta::trustp(): use <version>.accesible() in order to
> +                 verify whether or not the associated tarball is available (for
> +                 version = prev, curr or exp)
> +               */
> +	      #if 0
>  	      if (pkg.prev == *i)
>  		pkg.prev = packageversion ();
>  	      if (pkg.curr == *i)
>  		pkg.curr = packageversion ();
>  	      if (pkg.exp == *i)
>  		pkg.exp = packageversion ();
> +	      #endif
>  	      pkg.versions.erase (i++);
>  	      /* For now, leave the source version alone */
>  	    }
> diff --git a/package_meta.h b/package_meta.h
> index b24d4fc..b2f35e7 100644
> --- a/package_meta.h
> +++ b/package_meta.h
> @@ -105,12 +105,29 @@ public:
>      if (_default && curr && installed
>  	&& packageversion::compareVersions (curr, installed) < 0)
>        {
> -	if (exp && packageversion::compareVersions (installed, exp) < 0)
> +        /*
> +         - the expression above (i.e. curr && installed) reflect that setup.ini
> +           has a current entry for the package, and that the package is also in
> +           installed.db
> +         - this expression is a precondition that must be satisfied, or else it
> +           would not be possible to invoke the comparision function
> +         - below it is sufficient to use exp.accessible() as a precondition, as
> +           it implies that exp is true
> +           (if setup.ini does not have an experimental entry, exp.accessible()
> +            will yield false)
> +         - at the same time the expression verifies that the associated tarball
> +           is available
> +         */
> +	if (exp.accessible() && packageversion::compareVersions (installed, exp) < 0)
>  	  return exp;
>  	return installed;
>        }
>      /* Otherwise, if a "curr" version exists, return "curr". */
> -    if (curr)
> +    /*
> +     - again, use accessible() to verify whether or not the associated tarball
> +       is available
> +     */
> +    if (curr.accessible() )
>        return curr;
>      /* Otherwise return the installed version. */
>      return installed;
> -- 
> 2.8.0
> 


-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

Attachment: signature.asc
Description: PGP signature