setup: problems with local install

setup: problems with local install

[Ken Brown]

This is a followup to the thread started here:

   https://cygwin.com/ml/cygwin/2018-03/msg00027.html

There are two problems with installing from a local directory.

1. In the Category view, "No packages found" is displayed where it 
should say "All" in the first line of the list.  This is probably just a 
cosmetic issue, and I haven't tried to track it down yet.

2. In several of the views, all packages from setup.ini are listed, even 
if there is no corresponding archive in the local directory.  What 
happens is that packagemeta::scan() calls pkg.source()->sites.clear() 
for such packages, but this information is never used to prevent the 
package from appearing in the list.

It used to be that such packages would be declared inaccessible, but 
SolvableVersion::accessible() no longer does this.

Jon, you wrote the following comment in the definition of 
SolvableVersion::accessible():

"The 'accessible' check used to test if an archive was available locally 
or from a mirror.  This seems utterly pointless as binary packages which 
aren't 'accessible' never get to appear in the package list."

Do we need to reinstate the old function of the accessibility check?

Ken

Re: setup: problems with local install

[Ken Brown]

On 3/5/2018 1:34 PM, Ken Brown wrote:
> This is a followup to the thread started here:
> 
>    https://cygwin.com/ml/cygwin/2018-03/msg00027.html
> 
> There are two problems with installing from a local directory.
> 
> 1. In the Category view, "No packages found" is displayed where it 
> should say "All" in the first line of the list.  This is probably just a 
> cosmetic issue, and I haven't tried to track it down yet.

This actually happens in all setup runs, not just local installs.  It 
looks like it was caused by the rearranging that was done in the 
following commit:

commit 0c539f7f7d86fb100f260f21367682fa2c0bb529
Author: Jon Turney <jon.turney@dronecode.org.uk>
Date:   Sat Nov 4 18:01:49 2017 +0000

     Correctly order preparing packagedb for chooser

I think the problem might be that createListview() is now called too 
early in ChooserPage::OnInit().  But it's not immediately obvious to me 
how to fix this without breaking something else.

Ken

Re: setup: problems with local install

[Jon Turney]

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

On 05/03/2018 18:34, Ken Brown wrote:
> This is a followup to the thread started here:
> 
>    https://cygwin.com/ml/cygwin/2018-03/msg00027.html
> 
> There are two problems with installing from a local directory.

Thanks very much for looking into these.

> 2. In several of the views, all packages from setup.ini are listed, even 
> if there is no corresponding archive in the local directory.  What 
> happens is that packagemeta::scan() calls pkg.source()->sites.clear() 
> for such packages, but this information is never used to prevent the 
> package from appearing in the list.
> 
> It used to be that such packages would be declared inaccessible, but 
> SolvableVersion::accessible() no longer does this.
> 
> Jon, you wrote the following comment in the definition of 
> SolvableVersion::accessible():
> 
> "The 'accessible' check used to test if an archive was available locally 
> or from a mirror.  This seems utterly pointless as binary packages which 
> aren't 'accessible' never get to appear in the package list."
> 
> Do we need to reinstate the old function of the accessibility check?

I guess I looked at packagemeta::ScanDownloadedFiles() and saw that it 
was removing versions, and thought everything was good.

I didn't notice accessible() was indirectly how the result of scan() was 
returned :S

So yeah, I guess putting some complexity back in accessible() would 
work, or perhaps the attached?  (This doesn't do the right thing for a 
few packages, for reasons I'm still looking into...)

(I also note we have also have another 'erase an element from a vector 
while we are iterating over it' here, so that needs fixing, as well)

I'm kind of uncertain what the side-effects of this code are when source 
!= IDC_SOURCE_LOCALDIR, or if they are desired?  Perhaps it's removing 
packages which have corrupt local copies or something?  It would be 
clearer to omit the whole thing in that case.


[-- Attachment #2: 0001-Fix-packagemeta-ScanDownloadedFiles.patch --]
[-- Type: text/plain, Size: 2759 bytes --]

From d8aac08cf3d1ddf98ab9a6a8706b2c7b8bdfd7ad Mon Sep 17 00:00:00 2001
From: Jon Turney <jon.turney@dronecode.org.uk>
Date: Tue, 6 Mar 2018 14:56:40 +0000
Subject: [PATCH setup] Fix packagemeta::ScanDownloadedFiles

packagemeta::scan clears the site list if the package was not found, and
packagemeta::ScanDownloadedFiles uses packageversion::accessible() to check
that.

Instead communicate via a return value
---
 package_meta.cc | 27 ++++++++++++---------------
 package_meta.h  |  2 +-
 2 files changed, 13 insertions(+), 16 deletions(-)

diff --git a/package_meta.cc b/package_meta.cc
index c488e35..f0a0c20 100644
--- a/package_meta.cc
+++ b/package_meta.cc
@@ -664,33 +664,30 @@ packagemeta::logSelectionStatus() const
 }
 
 /* scan for local copies of package */
-void
+bool
 packagemeta::scan (const packageversion &pkg, bool mirror_mode)
 {
-  /* Already have something */
+  /* empty version */
   if (!pkg)
-    return;
+    return true;
 
-  /* Remove mirror sites.
-   * FIXME: This is a bit of a hack.
-   */
   try
     {
       if (!check_for_cached (*(pkg.source ()), NULL, mirror_mode, false)
-	  && ::source == IDC_SOURCE_LOCALDIR)
-	pkg.source ()->sites.clear ();
+          && ::source == IDC_SOURCE_LOCALDIR)
+        return false;
     }
   catch (Exception * e)
     {
       // We can ignore these, since we're clearing the source list anyway
       if (e->errNo () == APPERR_CORRUPT_PACKAGE)
-	{
-	  pkg.source ()->sites.clear ();
-	  return;
-	}
+        return false;
+
       // Unexpected exception.
       throw e;
     }
+
+  return true;
 }
 
 void
@@ -712,15 +709,15 @@ packagemeta::ScanDownloadedFiles (bool mirror_mode)
 			   && (*i != pkg.installed
 			       || pkg.installed == pkg.curr
 			       || pkg.installed == pkg.exp);
-	  scan (*i, lazy_scan);
+	  bool accessible = scan (*i, lazy_scan);
 	  packageversion foo = *i;
 	  packageversion pkgsrcver = foo.sourcePackage ();
-	  scan (pkgsrcver, lazy_scan);
+	  bool src_accessible = scan (pkgsrcver, lazy_scan);
 
 	  /* For local installs, if there is no src and no bin, the version
 	   * is unavailable
 	   */
-	  if (!i->accessible () && !pkgsrcver.accessible ()
+	  if (!accessible && !src_accessible
 	      && *i != pkg.installed)
 	    {
 	      if (pkg.curr == *i)
diff --git a/package_meta.h b/package_meta.h
index 32372e2..600a163 100644
--- a/package_meta.h
+++ b/package_meta.h
@@ -170,7 +170,7 @@ protected:
 private:
   std::string trustLabel(packageversion const &) const;
   std::vector <Script> scripts_;
-  static void scan (const packageversion &pkg, bool mirror_mode);
+  static bool scan (const packageversion &pkg, bool mirror_mode);
 
   bool _picked; /* true if desired version is to be (re)installed */
   bool _srcpicked;
-- 
2.16.2

Re: setup: problems with local install

[Jon Turney]

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

On 06/03/2018 02:18, Ken Brown wrote:
> On 3/5/2018 1:34 PM, Ken Brown wrote:
>> This is a followup to the thread started here:
>>
>>    https://cygwin.com/ml/cygwin/2018-03/msg00027.html
>>
>> There are two problems with installing from a local directory.
>>
>> 1. In the Category view, "No packages found" is displayed where it 
>> should say "All" in the first line of the list.  This is probably just 
>> a cosmetic issue, and I haven't tried to track it down yet.
> 
> This actually happens in all setup runs, not just local installs.  It 
> looks like it was caused by the rearranging that was done in the 
> following commit:
> 
> commit 0c539f7f7d86fb100f260f21367682fa2c0bb529
> Author: Jon Turney <jon.turney-GrJqePx9RPPAJUdA+FbntA@public.gmane.org>
> Date:   Sat Nov 4 18:01:49 2017 +0000
> 
>      Correctly order preparing packagedb for chooser
> 
> I think the problem might be that createListview() is now called too 
> early in ChooserPage::OnInit().  But it's not immediately obvious to me 
> how to fix this without breaking something else.
I'm kind of inclined to fix this with the attached: I think we only ever 
got "No packages found" before if there were 0 packages, which 
presumably doesn't happen very often :)

We don't update this header if a search reduces the number of packages 
shown to 0, which might make some sort of sense.

(Also, weirdly, we don't remove empty categories from view, except if we 
are doing a search, but this seems to be deliberate? (PickView.cc:311))

[-- Attachment #2: 0001-Always-give-the-fake-root-category-the-name-All.patch --]
[-- Type: text/plain, Size: 1430 bytes --]

From 1a0cf0a86088b4aae7d5a18953f7c214b07d8a03 Mon Sep 17 00:00:00 2001
From: Jon Turney <jon.turney@dronecode.org.uk>
Date: Tue, 6 Mar 2018 16:17:30 +0000
Subject: [PATCH setup] Always give the fake root category the name 'All'

After we rearranged things in 0c539f7f, it's now too early to tell if we
have any packages or not.

The only thing about this category that is ever used is it's name, so we
don't acually need to use the real 'All' category here.

Saying 'No packages found' was never particularly helpful here, so just use
a fake category with the fixed name 'All'.
---
 choose.cc | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/choose.cc b/choose.cc
index 9cf3a50..5a4d3ad 100644
--- a/choose.cc
+++ b/choose.cc
@@ -137,11 +137,8 @@ ChooserPage::createListview ()
 {
   SetBusy ();
   static std::vector<packagemeta *> empty_cat;
-  static Category dummy_cat (std::string ("No packages found."), empty_cat);
-  packagedb db;
-  packagedb::categoriesType::iterator it = db.categories.find("All");
-  Category &cat = (it == db.categories.end ()) ? dummy_cat : *it;
-  chooser = new PickView (cat);
+  static Category dummy_cat (std::string ("All"), empty_cat);
+  chooser = new PickView (dummy_cat);
   RECT r = getDefaultListViewSize();
   if (!chooser->Create(this, WS_CHILD | WS_HSCROLL | WS_VSCROLL | WS_VISIBLE,&r))
     throw new Exception (TOSTRING(__LINE__) " " __FILE__,
-- 
2.16.2

Re: setup: problems with local install

[Jon Turney]

On 06/03/2018 15:18, Jon Turney wrote:
> So yeah, I guess putting some complexity back in accessible() would 
> work, or perhaps the attached?  (This doesn't do the right thing for a 
> few packages, for reasons I'm still looking into...)

To be specific it was doing the wrong thing for those few packages with 
no source

>   /* scan for local copies of package */
> -void
> +bool
>   packagemeta::scan (const packageversion &pkg, bool mirror_mode)
>   {
> -  /* Already have something */
> +  /* empty version */
>     if (!pkg)
> -    return;
> +    return true;

So, this needs to be 'return false', as the empty version is always 
inaccessible, to get the same behaviour as before.

Re: setup: problems with local install

[Ken Brown]

On 3/6/2018 10:18 AM, Jon Turney wrote:
> (I also note we have also have another 'erase an element from a vector 
> while we are iterating over it' here, so that needs fixing, as well)

I think this one might be OK.  If I'm not mistaken, 
pkg.versions.erase(i++) passes a copy of i to erase, and then increments 
i before erase() has done its work.  But I'm no expert on this.

Ken

Re: setup: problems with local install

[Jon Turney]

On 06/03/2018 19:31, Ken Brown wrote:
> On 3/6/2018 10:18 AM, Jon Turney wrote:
>> (I also note we have also have another 'erase an element from a vector 
>> while we are iterating over it' here, so that needs fixing, as well)
> 
> I think this one might be OK.  If I'm not mistaken, 
> pkg.versions.erase(i++) passes a copy of i to erase, and then increments 
> i before erase() has done its work.  But I'm no expert on this.

I think that's still not ok for a vector, but this is actually a set, so 
there's actually no problem.

See e.g. [1], or refer to your copy of the C++ standard :)

[1] https://stackoverflow.com/questions/6438086/iterator-invalidation-rules

Re: setup: problems with local install

[Achim Gratz]

Ken Brown writes:
> I think this one might be OK.  If I'm not mistaken,
> pkg.versions.erase(i++) passes a copy of i to erase, and then
> increments i before erase() has done its work.  But I'm no expert on
> this.

I can't cite chapter and verse right now, but AFAIK iterators over sets
are explicitly OK'ed for deleting the current element of the set
iterated over (that may be restricted to not changing direction of
iteration depending on which version of the standard the implementation
conforms to).


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

SD adaptation for Waldorf microQ V2.22R2:
http://Synth.Stromeko.net/Downloads.html#WaldorfSDada

Re: setup: problems with local install

[Ken Brown]

On 3/6/2018 1:47 PM, Jon Turney wrote:
> On 06/03/2018 15:18, Jon Turney wrote:
>> So yeah, I guess putting some complexity back in accessible() would 
>> work, or perhaps the attached?  (This doesn't do the right thing for a 
>> few packages, for reasons I'm still looking into...)
> 
> To be specific it was doing the wrong thing for those few packages with 
> no source
> 
>>   /* scan for local copies of package */
>> -void
>> +bool
>>   packagemeta::scan (const packageversion &pkg, bool mirror_mode)
>>   {
>> -  /* Already have something */
>> +  /* empty version */
>>     if (!pkg)
>> -    return;
>> +    return true;
> 
> So, this needs to be 'return false', as the empty version is always 
> inaccessible, to get the same behaviour as before.

I've found another problem with local installs: If a package needs 
upgrading, then the chooser will offer the upgraded version for install, 
even if there's no archive available.  As a result, the current version 
will get uninstalled, and then setup will discover that it doesn't have 
the archive to install the new version.

The problem occurs because packagedb::defaultTrust() is called after 
ScanDownloadedFiles() has already done its work.  solution.update() and 
solution.trans2db() are called, and pkg->desired is set equal to an 
inaccessible version pv (which has been previously removed from 
pkg->versions).

I guess trans2db() should check that pv is in pkg->versions before 
acting on an install transaction for pv.  And then we also have to make 
sure to ignore the erase transaction for the current version of pkg.

Alternatively, can we just remove an inaccessible packageversion from 
the libsolv pool, or at at least just tell libsolv that we don't want to 
install it?

Ken

Re: setup: problems with local install

[Ken Brown]

On 3/7/2018 4:52 PM, Ken Brown wrote:
> On 3/6/2018 1:47 PM, Jon Turney wrote:
>> On 06/03/2018 15:18, Jon Turney wrote:
>>> So yeah, I guess putting some complexity back in accessible() would 
>>> work, or perhaps the attached?  (This doesn't do the right thing for 
>>> a few packages, for reasons I'm still looking into...)
>>
>> To be specific it was doing the wrong thing for those few packages 
>> with no source
>>
>>>   /* scan for local copies of package */
>>> -void
>>> +bool
>>>   packagemeta::scan (const packageversion &pkg, bool mirror_mode)
>>>   {
>>> -  /* Already have something */
>>> +  /* empty version */
>>>     if (!pkg)
>>> -    return;
>>> +    return true;
>>
>> So, this needs to be 'return false', as the empty version is always 
>> inaccessible, to get the same behaviour as before.
> 
> I've found another problem with local installs: If a package needs 
> upgrading, then the chooser will offer the upgraded version for install, 
> even if there's no archive available.  As a result, the current version 
> will get uninstalled, and then setup will discover that it doesn't have 
> the archive to install the new version.
> 
> The problem occurs because packagedb::defaultTrust() is called after 
> ScanDownloadedFiles() has already done its work.  solution.update() and 
> solution.trans2db() are called, and pkg->desired is set equal to an 
> inaccessible version pv (which has been previously removed from 
> pkg->versions).
> 
> I guess trans2db() should check that pv is in pkg->versions before 
> acting on an install transaction for pv.  And then we also have to make 
> sure to ignore the erase transaction for the current version of pkg.
> 
> Alternatively, can we just remove an inaccessible packageversion from 
> the libsolv pool, or at at least just tell libsolv that we don't want to 
> install it?

Still another alternative, and maybe the simplest, is to make sure that 
the chooser never shows a version that is not in pkg->versions. 
packagemeta::set_action (trusts const trust) almost does this, except 
for one useless desired.accessible() call.  So that should be fixed, as 
well as the setting of the initial action.

Ken

Re: setup: problems with local install

[Ken Brown]

On 3/8/2018 10:59 AM, Ken Brown wrote:
> On 3/7/2018 4:52 PM, Ken Brown wrote:
>> On 3/6/2018 1:47 PM, Jon Turney wrote:
>>> On 06/03/2018 15:18, Jon Turney wrote:
>>>> So yeah, I guess putting some complexity back in accessible() would 
>>>> work, or perhaps the attached?  (This doesn't do the right thing for 
>>>> a few packages, for reasons I'm still looking into...)
>>>
>>> To be specific it was doing the wrong thing for those few packages 
>>> with no source
>>>
>>>>   /* scan for local copies of package */
>>>> -void
>>>> +bool
>>>>   packagemeta::scan (const packageversion &pkg, bool mirror_mode)
>>>>   {
>>>> -  /* Already have something */
>>>> +  /* empty version */
>>>>     if (!pkg)
>>>> -    return;
>>>> +    return true;
>>>
>>> So, this needs to be 'return false', as the empty version is always 
>>> inaccessible, to get the same behaviour as before.
>>
>> I've found another problem with local installs: If a package needs 
>> upgrading, then the chooser will offer the upgraded version for 
>> install, even if there's no archive available.  As a result, the 
>> current version will get uninstalled, and then setup will discover 
>> that it doesn't have the archive to install the new version.
>>
>> The problem occurs because packagedb::defaultTrust() is called after 
>> ScanDownloadedFiles() has already done its work.  solution.update() 
>> and solution.trans2db() are called, and pkg->desired is set equal to 
>> an inaccessible version pv (which has been previously removed from 
>> pkg->versions).
>>
>> I guess trans2db() should check that pv is in pkg->versions before 
>> acting on an install transaction for pv.  And then we also have to 
>> make sure to ignore the erase transaction for the current version of pkg.
>>
>> Alternatively, can we just remove an inaccessible packageversion from 
>> the libsolv pool, or at at least just tell libsolv that we don't want 
>> to install it?
> 
> Still another alternative, and maybe the simplest, is to make sure that 
> the chooser never shows a version that is not in pkg->versions. 
> packagemeta::set_action (trusts const trust) almost does this, except 
> for one useless desired.accessible() call.  So that should be fixed, as 
> well as the setting of the initial action.

Sorry for this stream of consciousness series of posts, but I'll stop 
after this one.  I think it's possible that restoring the previous 
meaning of 'accessible()', at least for local installs, might solve this 
problem.

Ken

Re: setup: problems with local install

[Ken Brown]

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

On 3/8/2018 4:59 PM, Ken Brown wrote:
> On 3/8/2018 10:59 AM, Ken Brown wrote:
>> On 3/7/2018 4:52 PM, Ken Brown wrote:
>>> On 3/6/2018 1:47 PM, Jon Turney wrote:
>>>> On 06/03/2018 15:18, Jon Turney wrote:
>>>>> So yeah, I guess putting some complexity back in accessible() would 
>>>>> work, or perhaps the attached?  (This doesn't do the right thing 
>>>>> for a few packages, for reasons I'm still looking into...)
>>>>
>>>> To be specific it was doing the wrong thing for those few packages 
>>>> with no source
>>>>
>>>>>   /* scan for local copies of package */
>>>>> -void
>>>>> +bool
>>>>>   packagemeta::scan (const packageversion &pkg, bool mirror_mode)
>>>>>   {
>>>>> -  /* Already have something */
>>>>> +  /* empty version */
>>>>>     if (!pkg)
>>>>> -    return;
>>>>> +    return true;
>>>>
>>>> So, this needs to be 'return false', as the empty version is always 
>>>> inaccessible, to get the same behaviour as before.
>>>
>>> I've found another problem with local installs: If a package needs 
>>> upgrading, then the chooser will offer the upgraded version for 
>>> install, even if there's no archive available.  As a result, the 
>>> current version will get uninstalled, and then setup will discover 
>>> that it doesn't have the archive to install the new version.
>>>
>>> The problem occurs because packagedb::defaultTrust() is called after 
>>> ScanDownloadedFiles() has already done its work.  solution.update() 
>>> and solution.trans2db() are called, and pkg->desired is set equal to 
>>> an inaccessible version pv (which has been previously removed from 
>>> pkg->versions).
>>>
>>> I guess trans2db() should check that pv is in pkg->versions before 
>>> acting on an install transaction for pv.  And then we also have to 
>>> make sure to ignore the erase transaction for the current version of 
>>> pkg.
>>>
>>> Alternatively, can we just remove an inaccessible packageversion from 
>>> the libsolv pool, or at at least just tell libsolv that we don't want 
>>> to install it?
>>
>> Still another alternative, and maybe the simplest, is to make sure 
>> that the chooser never shows a version that is not in pkg->versions. 
>> packagemeta::set_action (trusts const trust) almost does this, except 
>> for one useless desired.accessible() call.  So that should be fixed, 
>> as well as the setting of the initial action.
> 
> Sorry for this stream of consciousness series of posts, but I'll stop 
> after this one.  I think it's possible that restoring the previous 
> meaning of 'accessible()', at least for local installs, might solve this 
> problem.

Patches attached (for the topic/fix-local-install branch).


[-- Attachment #2: 0001-Make-SolvableVersion-accessible-useful-for-local-ins.patch --]
[-- Type: text/plain, Size: 1732 bytes --]

From 1645f2b13616ec71054a5ad35bce864f1f3e0dca Mon Sep 17 00:00:00 2001
From: Ken Brown <kbrown@cornell.edu>
Date: Sun, 11 Mar 2018 10:43:06 -0400
Subject: [PATCH 1/2] Make SolvableVersion::accessible() useful for local
 installs

For a local install, make SolvableVersion::accessible() indicate
whether or not there is an archive available in the local cache.  This
is used in packagemeta::set_action() to prevent the chooser from
offering unavailable versions.
---
 libsolv.cc | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/libsolv.cc b/libsolv.cc
index 0dc7557..604ff7e 100644
--- a/libsolv.cc
+++ b/libsolv.cc
@@ -14,6 +14,7 @@
 #include "libsolv.h"
 #include "package_db.h"
 #include "package_meta.h"
+#include "resource.h"
 
 #include "solv/solver.h"
 #include "solv/solverdebug.h"
@@ -239,21 +240,16 @@ SolvableVersion::source() const
   return (packagesource *)repo_lookup_num(solvable->repo, id, psrc_attr, (unsigned long long)&empty_source);
 }
 
+// If we're doing a locall install, is there an archive available?
+// This assumes that ScanDownloadedFiles() has already been called.
 bool
 SolvableVersion::accessible () const
 {
-  // The 'accessible' check used to test if an archive was available locally or
-  // from a mirror.
-  //
-  // This seems utterly pointless. as binary packages which aren't 'accessible'
-  // never get to appear in the package list.
-  //
-  // For source packages, it's equivalent to the bool conversion operator.)
-  //
-  if (id != 0)
-    return TRUE;
-  else
-    return FALSE;
+  if (id == 0)
+    return false;
+  if (::source == IDC_SOURCE_LOCALDIR)
+    return source ()->Cached ();
+  return true;
 }
 
 package_stability_t
-- 
2.16.2


[-- Attachment #3: 0002-Don-t-put-inaccessible-packageversions-in-the-packag.patch --]
[-- Type: text/plain, Size: 1543 bytes --]

From 365fa9afb8a49c2360bf7cfd2a2c2557522f85e5 Mon Sep 17 00:00:00 2001
From: Ken Brown <kbrown@cornell.edu>
Date: Mon, 12 Mar 2018 09:13:41 -0400
Subject: [PATCH 2/2] Don't put inaccessible packageversions in the package
 database

In a local install, libsolv might choose an inaccessible version for
install.  Change SolverSolution::trans2db() so that such a version is
never used.
---
 libsolv.cc | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/libsolv.cc b/libsolv.cc
index 604ff7e..1342f3b 100644
--- a/libsolv.cc
+++ b/libsolv.cc
@@ -637,6 +637,8 @@ void
 SolverSolution::trans2db() const
 {
   packagedb db;
+  std::vector<packagemeta *> inaccessible;
+
   // First reset all packages to the "no change" state
   for (packagedb::packagecollection::iterator i = db.packages.begin();
        i != db.packages.end(); i++)
@@ -667,6 +669,8 @@ SolverSolution::trans2db() const
             {
               pkg->desired = pkg->default_version = pv;
               pkg->pick(true);
+	      if (!pv.accessible())
+		inaccessible.push_back (pkg);
             }
           else // source package
             pkg->srcpick(true);
@@ -680,6 +684,14 @@ SolverSolution::trans2db() const
           break;
         }
     }
+  for (std::vector<packagemeta *>::iterator i = inaccessible.begin();
+       i != inaccessible.end(); i++)
+    {
+      packagemeta *pkg = *i;
+      // Reset pkg to its "no change" state.
+      pkg->desired = pkg->default_version = pkg->installed;
+      pkg->pick(false);
+    }
 }
 
 void
-- 
2.16.2

Re: setup: problems with local install

[Jon Turney]

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

On 12/03/2018 13:22, Ken Brown wrote:
> On 3/8/2018 4:59 PM, Ken Brown wrote:
>> On 3/8/2018 10:59 AM, Ken Brown wrote:
>>> On 3/7/2018 4:52 PM, Ken Brown wrote:
>>>> On 3/6/2018 1:47 PM, Jon Turney wrote:
>>>>> On 06/03/2018 15:18, Jon Turney wrote:
>>>>>> So yeah, I guess putting some complexity back in accessible() 
>>>>>> would work, or perhaps the attached?  (This doesn't do the right 
>>>>>> thing for a few packages, for reasons I'm still looking into...)
>>>>>
>>>>> To be specific it was doing the wrong thing for those few packages 
>>>>> with no source
>>>>>
>>>>>>   /* scan for local copies of package */
>>>>>> -void
>>>>>> +bool
>>>>>>   packagemeta::scan (const packageversion &pkg, bool mirror_mode)
>>>>>>   {
>>>>>> -  /* Already have something */
>>>>>> +  /* empty version */
>>>>>>     if (!pkg)
>>>>>> -    return;
>>>>>> +    return true;
>>>>>
>>>>> So, this needs to be 'return false', as the empty version is always 
>>>>> inaccessible, to get the same behaviour as before.
>>>>
>>>> I've found another problem with local installs: If a package needs 
>>>> upgrading, then the chooser will offer the upgraded version for 
>>>> install, even if there's no archive available.  As a result, the 
>>>> current version will get uninstalled, and then setup will discover 
>>>> that it doesn't have the archive to install the new version.

Thanks very much for finding this.

>>>> The problem occurs because packagedb::defaultTrust() is called after 
>>>> ScanDownloadedFiles() has already done its work.  solution.update() 
>>>> and solution.trans2db() are called, and pkg->desired is set equal to 
>>>> an inaccessible version pv (which has been previously removed from 
>>>> pkg->versions).
>>>>
>>>> I guess trans2db() should check that pv is in pkg->versions before 
>>>> acting on an install transaction for pv.  And then we also have to 
>>>> make sure to ignore the erase transaction for the current version of 
>>>> pkg.
>>>>
>>>> Alternatively, can we just remove an inaccessible packageversion 
>>>> from the libsolv pool, or at at least just tell libsolv that we 
>>>> don't want to install it?

Attached is an attempt at that.

I think this is preferable, if it works correctly, as I think avoiding 
solutions with these unavailable versions is better than trying to 
massage the solution afterwards.

>>> Still another alternative, and maybe the simplest, is to make sure 
>>> that the chooser never shows a version that is not in pkg->versions. 
>>> packagemeta::set_action (trusts const trust) almost does this, except 
>>> for one useless desired.accessible() call.  So that should be fixed, 
>>> as well as the setting of the initial action.
>>
>> Sorry for this stream of consciousness series of posts, but I'll stop 
>> after this one.  I think it's possible that restoring the previous 
>> meaning of 'accessible()', at least for local installs, might solve 
>> this problem.

Thanks for the patches.



[-- Attachment #2: 0001-Remove-packages-not-found-by-scan-from-solver.patch --]
[-- Type: text/plain, Size: 1819 bytes --]

From 2ec3a89555409b43b35a2d5a8f161c41c1a993d9 Mon Sep 17 00:00:00 2001
From: Jon Turney <jon.turney@dronecode.org.uk>
Date: Wed, 14 Mar 2018 14:52:57 +0000
Subject: [PATCH setup] Remove packages not found by scan from solver

Remove (not installed) packages not found by scan from solver, as well as
from packagemeta, to avoid solutions including them from being proposed.

Signed-off-by: Jon Turney <jon.turney@dronecode.org.uk>
---
 libsolv.cc      | 10 ++++++++++
 libsolv.h       |  2 ++
 package_meta.cc |  3 +++
 3 files changed, 15 insertions(+)

diff --git a/libsolv.cc b/libsolv.cc
index 674d576..fc4e5ec 100644
--- a/libsolv.cc
+++ b/libsolv.cc
@@ -304,6 +304,16 @@ SolvableVersion::compareVersions(const SolvableVersion &a,
   return pool_evrcmp(pool, evra, evrb, EVRCMP_COMPARE);
 }
 
+void
+SolvableVersion::remove() const
+{
+  if (!id)
+    return;
+
+  Solvable *solvable = pool_id2solvable(pool, id);
+  repo_free_solvable(solvable->repo, id, 0);
+}
+
 // ---------------------------------------------------------------------------
 // implements class SolverPool
 //
diff --git a/libsolv.h b/libsolv.h
index fc68895..7bb0be2 100644
--- a/libsolv.h
+++ b/libsolv.h
@@ -88,6 +88,8 @@ class SolvableVersion
   bool operator > (SolvableVersion const &) const;
   bool operator >= (SolvableVersion const &) const;
 
+  void remove() const;
+
  private:
   Id id;
   Pool *pool;
diff --git a/package_meta.cc b/package_meta.cc
index 8a33bcb..7f8110d 100644
--- a/package_meta.cc
+++ b/package_meta.cc
@@ -724,7 +724,10 @@ packagemeta::ScanDownloadedFiles (bool mirror_mode)
 		pkg.curr = packageversion ();
 	      if (pkg.exp == *i)
 		pkg.exp = packageversion ();
+
+	      i->remove();
 	      pkg.versions.erase (i++);
+
 	      /* For now, leave the source version alone */
 	    }
 	  else
-- 
2.16.2

Re: setup: problems with local install

[Ken Brown]

On 3/14/2018 12:07 PM, Jon Turney wrote:
> On 12/03/2018 13:22, Ken Brown wrote:
>> On 3/8/2018 4:59 PM, Ken Brown wrote:
>>> On 3/8/2018 10:59 AM, Ken Brown wrote:
>>>> On 3/7/2018 4:52 PM, Ken Brown wrote:
>>>>> On 3/6/2018 1:47 PM, Jon Turney wrote:
>>>>>> On 06/03/2018 15:18, Jon Turney wrote:
>>>>>>> So yeah, I guess putting some complexity back in accessible() 
>>>>>>> would work, or perhaps the attached?  (This doesn't do the right 
>>>>>>> thing for a few packages, for reasons I'm still looking into...)
>>>>>>
>>>>>> To be specific it was doing the wrong thing for those few packages 
>>>>>> with no source
>>>>>>
>>>>>>>   /* scan for local copies of package */
>>>>>>> -void
>>>>>>> +bool
>>>>>>>   packagemeta::scan (const packageversion &pkg, bool mirror_mode)
>>>>>>>   {
>>>>>>> -  /* Already have something */
>>>>>>> +  /* empty version */
>>>>>>>     if (!pkg)
>>>>>>> -    return;
>>>>>>> +    return true;
>>>>>>
>>>>>> So, this needs to be 'return false', as the empty version is 
>>>>>> always inaccessible, to get the same behaviour as before.
>>>>>
>>>>> I've found another problem with local installs: If a package needs 
>>>>> upgrading, then the chooser will offer the upgraded version for 
>>>>> install, even if there's no archive available.  As a result, the 
>>>>> current version will get uninstalled, and then setup will discover 
>>>>> that it doesn't have the archive to install the new version.
> 
> Thanks very much for finding this.
> 
>>>>> The problem occurs because packagedb::defaultTrust() is called 
>>>>> after ScanDownloadedFiles() has already done its work.  
>>>>> solution.update() and solution.trans2db() are called, and 
>>>>> pkg->desired is set equal to an inaccessible version pv (which has 
>>>>> been previously removed from pkg->versions).
>>>>>
>>>>> I guess trans2db() should check that pv is in pkg->versions before 
>>>>> acting on an install transaction for pv.  And then we also have to 
>>>>> make sure to ignore the erase transaction for the current version 
>>>>> of pkg.
>>>>>
>>>>> Alternatively, can we just remove an inaccessible packageversion 
>>>>> from the libsolv pool, or at at least just tell libsolv that we 
>>>>> don't want to install it?
> 
> Attached is an attempt at that.
> 
> I think this is preferable, if it works correctly, as I think avoiding 
> solutions with these unavailable versions is better than trying to 
> massage the solution afterwards.

I agree, and it does seem to work correctly.  But there's one other case 
that it doesn't cover:  Suppose there's no archive for the installed 
version.  Even with your patch, the user can still choose 'Reinstall', 
which will fail.  The first of the patches from my previous email fixes 
this case.

Ken

Re: setup: problems with local install

[Jon Turney]

On 14/03/2018 19:24, Ken Brown wrote:
> On 3/14/2018 12:07 PM, Jon Turney wrote:
>> On 12/03/2018 13:22, Ken Brown wrote:
>>> On 3/8/2018 4:59 PM, Ken Brown wrote:
>>>> On 3/8/2018 10:59 AM, Ken Brown wrote:
>>>>> On 3/7/2018 4:52 PM, Ken Brown wrote:
>>>>>> On 3/6/2018 1:47 PM, Jon Turney wrote:
>>>>>>> On 06/03/2018 15:18, Jon Turney wrote:
>>>>>> I've found another problem with local installs: If a package needs 
>>>>>> upgrading, then the chooser will offer the upgraded version for 
>>>>>> install, even if there's no archive available.  As a result, the 
>>>>>> current version will get uninstalled, and then setup will discover 
>>>>>> that it doesn't have the archive to install the new version.
>>
>> Thanks very much for finding this.
>>
>>>>>> The problem occurs because packagedb::defaultTrust() is called 
>>>>>> after ScanDownloadedFiles() has already done its work. 
>>>>>> solution.update() and solution.trans2db() are called, and 
>>>>>> pkg->desired is set equal to an inaccessible version pv (which has 
>>>>>> been previously removed from pkg->versions).
>>>>>>
>>>>>> I guess trans2db() should check that pv is in pkg->versions before 
>>>>>> acting on an install transaction for pv.  And then we also have to 
>>>>>> make sure to ignore the erase transaction for the current version 
>>>>>> of pkg.
>>>>>>
>>>>>> Alternatively, can we just remove an inaccessible packageversion 
>>>>>> from the libsolv pool, or at at least just tell libsolv that we 
>>>>>> don't want to install it?
>>
>> Attached is an attempt at that.
>>
>> I think this is preferable, if it works correctly, as I think avoiding 
>> solutions with these unavailable versions is better than trying to 
>> massage the solution afterwards.
> 
> I agree, and it does seem to work correctly.  But there's one other case 
> that it doesn't cover:  Suppose there's no archive for the installed 
> version.  Even with your patch, the user can still choose 'Reinstall', 
> which will fail.  The first of the patches from my previous email fixes 
> this case.

Yes.  I applied that as well.

The subtlety I'd failed to grasp when I wrote that comment is that 
ScanDownloadedFiles() prunes versions which aren't available locally, 
*except* if they are already installed.

Thanks.