Re: [PATCH] Revert "Don't override a Keep selection"

[Ken Brown <kbrown@cornell.edu>]

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

On 10/19/2017 11:05 AM, Ken Brown wrote:
> On 10/18/2017 11:01 PM, Ken Brown wrote:
>> On 10/17/2017 3:31 PM, Ken Brown wrote:
>>> On 10/17/2017 2:43 PM, Jon Turney wrote:
>>>> On 16/10/2017 20:13, Ken Brown wrote:
>>>>> This reverts (the rest of) commit b43b697.  Part of that commit was
>>>>> already reverted in commit ff0bb3d.  The rest is not needed either
>>>>> since we no longer send the upgrade flag to the solver after the user
>>>>> has made their selections.
>>>>> ---
>>>>>   libsolv.cc     | 14 +++-----------
>>>>>   libsolv.h      |  1 -
>>>>>   package_meta.h |  2 --
>>>>>   3 files changed, 3 insertions(+), 14 deletions(-)
>>>>
>>>> Hmm... not sure about this.
>>>>
>>>> Say the initial upgrade solution had something like: package A 1.0 
>>>> -> 1.1, and A 1.1 has a dependency on package B 2.0, where currently 
>>>> B 1.0 is installed (so B 1.0 -> 2.0)
>>>>
>>>> If the user then changes B to 'keep' (at 1.0), we should report a 
>>>> conflict?
>>>
>>> Good point.  I wasn't thinking about dependencies with version 
>>> relations.
>>
>> I just tested this scenario, and it turns out that putting a lock on B 
>> 1.0 overrides the dependency of A 1.1 on B 2.0.  There's no problem 
>> reported.  On the other hand, if there's no lock, then the dependency 
>> overrides the Keep choice, again with no problem reported.
>>
>> Back to the drawing board.
> 
> Thinking about this further, using SOLVER_LOCK is probably the right 
> thing to do in this situation, even if no problem is reported.  We can't 
> insist that users install the recommended version.  For example, if I 
> choose to downgrade binutils while keeping the current gcc, then I'm 
> responsible for any breakage this might cause, with or without a warning 
> from setup.
> 
> I'll prepare a new patch that restores the SOLVER_LOCK functionality 
> that was inadvertently removed in commit ff0bb3d.

Attached.  I kept making mistakes while doing this, so I hope I got it 
right in the end.

Here's a related question.  Currently if libsolv decides I should 
install something and I choose Skip instead, it will get installed 
anyway (with no problem report).  Maybe we should have a taskSkip that 
generates a SOLVER_LOCK in that case, analogous to taskKeep?

Ken

[-- Attachment #2: 0001-Fix-the-functionality-of-taskKeep.patch --]
[-- Type: text/plain, Size: 3883 bytes --]

From 610426eabeccb3c04bbd7f49862248beb82b280a Mon Sep 17 00:00:00 2001
From: Ken Brown <kbrown@cornell.edu>
Date: Thu, 19 Oct 2017 14:44:45 -0400
Subject: [PATCH] Fix the functionality of taskKeep

A taskKeep is generated whenever the user wants to keep an installed
version that's different from the default_version, where the latter is
whatever the solver has chosen.  We need to make sure that
default_version is set appropriately wherever it is needed.
---
 choose.cc      |  7 +++++++
 libsolv.cc     | 11 ++++++-----
 package_meta.h |  2 +-
 prereq.cc      |  3 +--
 4 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/choose.cc b/choose.cc
index f0e6802..ad37639 100644
--- a/choose.cc
+++ b/choose.cc
@@ -420,6 +420,13 @@ ChooserPage::changeTrust(int button, bool test, bool initial)
     {
       // but initially we want a task list with any package changes caused by
       // command line options
+      // and we don't want to generate spurious Keep tasks
+      for (packagedb::packagecollection::iterator p = db.packages.begin ();
+	   p != db.packages.end (); ++p)
+	{
+	  packagemeta *pkg = p->second;
+	  pkg->default_version = pkg->installed;
+	}
       q.setTasks();
     }
   db.defaultTrust(q, mode, test);
diff --git a/libsolv.cc b/libsolv.cc
index 289f19c..e623555 100644
--- a/libsolv.cc
+++ b/libsolv.cc
@@ -526,7 +526,8 @@ SolverTasks::setTasks()
 
       // decode UI state to action
       // skip and keep don't change dependency solution
-      // except when we want to keep an old version
+      // except when we want to keep a version different from the one
+      // chosen by the solver
       if (pkg->installed != pkg->desired)
         {
           if (pkg->desired)
@@ -538,7 +539,7 @@ SolverTasks::setTasks()
 	{
 	  if (pkg->picked())
 	    add(pkg->installed, taskReinstall); // reinstall
-	  else if (pkg->installed < pkg->default_version)
+	  else if (pkg->installed != pkg->default_version)
 	    add(pkg->installed, taskKeep); // keep
 	}
       // only install action makes sense for source packages
@@ -591,7 +592,7 @@ SolverSolution::trans2db() const
        i != db.packages.end(); i++)
     {
       packagemeta *pkg = i->second;
-      pkg->desired = pkg->installed;
+      pkg->desired = pkg->default_version = pkg->installed;
       pkg->pick(false);
     }
   // Now make changes according to trans.  transErase requires some
@@ -614,7 +615,7 @@ SolverSolution::trans2db() const
 	case SolverTransaction::transInstall:
 	  if (pv.Type() == package_binary)
 	    {
-	      pkg->desired  = pv;
+	      pkg->desired = pkg->default_version = pv;
 	      pkg->pick(true);
 	    }
 	  else			// source package
@@ -623,7 +624,7 @@ SolverSolution::trans2db() const
 	case SolverTransaction::transErase:
 	  // Only relevant if pkg is still in its "no change" state
 	  if (pkg->desired == pkg->installed && !pkg->picked())
-	    pkg->desired = packageversion();
+	    pkg->desired = pkg->default_version = packageversion();
 	  break;
 	default:
 	  break;
diff --git a/package_meta.h b/package_meta.h
index d91f7c9..98253d0 100644
--- a/package_meta.h
+++ b/package_meta.h
@@ -131,7 +131,7 @@ public:
   packageversion curr;
   /* ditto for "test" (experimental) */
   packageversion exp;
-  /* which one is the default according to the chooser global state */
+  /* which one is the default according to the solver */
   packageversion default_version;
   /* Now for the user stuff :] */
   /* What version does the user want ? */
diff --git a/prereq.cc b/prereq.cc
index c5a1fd5..bf7661a 100644
--- a/prereq.cc
+++ b/prereq.cc
@@ -136,8 +136,7 @@ PrereqPage::whatNext ()
 long
 PrereqPage::OnBack ()
 {
-  // Reset the package database to correspond to the solver's default
-  // solution
+  // Reset the package database to correspond to the solver's solution
   packagedb db;
   db.solution.trans2db();
 
-- 
2.14.2