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

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

[Houder]

[-- Attachment #1: Type: text/plain, Size: 893 bytes --]

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.

Regards,
henri

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Do-not-clear-the-prev-curr-and-exp-fields-of-package.patch --]
[-- Type: text/x-diff; name=0001-Do-not-clear-the-prev-curr-and-exp-fields-of-package.patch, Size: 4194 bytes --]

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

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

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 5850 bytes --]

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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Jon Turney]

On 04/05/2016 13:38, Houder wrote:
> Please, submit this patch.

Thanks for the 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().

Can you clarify a bit about the problem that this patch solves?  And how 
you tested that it fixes it?

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

[Houder]

On 2016-05-23 15:44, Jon Turney wrote:
>> 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().
> 
> Can you clarify a bit about the problem that this patch solves?  And
> how you tested that it fixes it?

Hi Jon,

I reported the "problem" in March, here:

     https://cygwin.com/ml/cygwin/2016-03/msg00425.html
      - Package choosing algorithm ...

However, in a nutshell the story is as follows:

I install from "Local Directory" (i.e. not directly from a mirror; 
neither do
I operate a local mirror).

At that point in time I had installed a TEST version of the cygwin pkg. 
Then
Corinna launched another TEST version of that package. After downloading 
that
new version, I also removed the CURRENT version of that pkg (yes, nobody 
does
that, except me).

After that, I started setup.exe again in order to install the test 
version of
the cygwin pkg, which I had downloaded.

To my surprise, the new test version was NOT offered for install ... As 
I did
not understand why, I started the thread above.

Especially when Corinna told me, that it should work, I really started 
to get
curious, and decided to study the source code of setup ...

After some effort, I found the cause of the problem.

Yes, the patch has been tested, but in order to describe what I did, I 
really
need some more time (as it has been a while), and because I still have a 
hard
time putting it down in English (not being my native tongue).

Thank you for your interest in my patch (although it may not solve a 
problem,
that is really important, it does improve the 'clarity' of the source 
code, I
believe).

Regards,
Henri

Re: [PATCH] Do not clear the prev, curr and ... Minor clarification.

[Houder]

On 2016-05-23 17:26, Houder wrote:
> On 2016-05-23 15:44, Jon Turney wrote:

>> Can you clarify a bit about the problem that this patch solves?  And
>> how you tested that it fixes it?
> 
> Hi Jon,
> 
> I reported the "problem" in March, here:
> 
>     https://cygwin.com/ml/cygwin/2016-03/msg00425.html
>      - Package choosing algorithm ...
> 
> However, in a nutshell the story is as follows:
> 
> I install from "Local Directory" (i.e. not directly from a mirror; 
> neither do
> I operate a local mirror).
> 
> At that point in time I had installed a TEST version of the cygwin pkg. 
> Then
> Corinna launched another TEST version of that package. After 
> downloading that
> new version, I also removed the CURRENT version of that pkg (yes, 
> nobody does
> that, except me).

... in order not to create any confusion, I might clarify:

... I removed the current version of the pkg in my repository (disk) -- 
removed
the tarball, not the pkg from the system (Cygwin).

Regards,
Henri