Re: [PATCH setup 00/14] Use libsolv, solve all our problems... (WIP)

[Jon Turney <jon.turney@dronecode.org.uk>]

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

On 24/12/2017 15:00, Ken Brown wrote:
> On 12/13/2017 5:31 PM, Ken Brown wrote:
>> On 12/13/2017 1:05 PM, Achim Gratz wrote:
>>> Ken Brown writes:
>>>> 1. Uninstall A.
>>>> 2. Don't uninstall B.
>>>>
>>>> On the surface, it would seem that libsolv chose 2 by default, because
>>>> it returned an empty transaction list.  This was reflected in the log
>>>> and was also clear when I selected 'Back'.

Yeah, I think what is actually happening here is that the solver returns 
a partial solution, without the problematic transaction.

But yeah, there's no real concept of a default solution, so (lacking a 
UI to choose, which I think is a bit out of scope for the moment), it's 
up to us to define one.

>>> I don't think there is a default in this case.  I also see in zypper
>>> that the order of the proposed solutions (there can be way more than two
>>> if the dependencies are more complicated) is not always the same, so
>>> there is no preference implied by the order as well.
>>>
>>>> Maybe we have to deal with this situation ourselves.  Whenever a
>>>> problem involves a missing dependency, we could choose as default
>>>> solution the one that installs/keeps the dependent package, as is
>>>> currently done.
>>>
>>> That solution unfortunately isn't always the one that causes the least
>>> amount of transactions or even the least amount of breakage.
>>
>> That may be true, but I still think it's a reasonable default.  The 
>> user doesn't have to accept it.  Also, it's consistent with what setup 
>> currently does, so it won't surprise anyone.
>>
>> The attached patch attempts to implement my suggestion.

I came up with a slightly different solution of just picking the first 
solution as a default.

After solving problems we also need to consider the 'install source for 
everything I install' flag, which unfortunately requires quite a bit of 
refactoring.

See attached.

[-- Attachment #2: 0001-Apply-default-solution-s.patch --]
[-- Type: text/plain, Size: 11230 bytes --]

From a60bbd19aa5504e0e7da6722dc7f3b81ac3afd6b Mon Sep 17 00:00:00 2001
From: Jon Turney <jon.turney@dronecode.org.uk>
Date: Wed, 6 Dec 2017 17:52:45 +0000
Subject: [PATCH setup] Apply default solution(s)

Refactoring of SolverSolution::update() so we can apply the default
solution.

Also:

Break out logging of the task list, so we can show it in the "dependency
problems exists, but don't use the default solution, just do what I ask"
case.

Break out 'include-source' process, so it can have effect in the case where
dependency problems exist.
---
 libsolv.cc    | 83 ++++++++++++++++++++++++++++++++++++++++++++---------------
 libsolv.h     | 11 ++++++--
 package_db.cc |  2 +-
 prereq.cc     | 28 ++++++++++++++++----
 prereq.h      |  4 +++
 5 files changed, 99 insertions(+), 29 deletions(-)

diff --git a/libsolv.cc b/libsolv.cc
index 0fb39c7..34af50b 100644
--- a/libsolv.cc
+++ b/libsolv.cc
@@ -544,6 +544,7 @@ SolverTasks::setTasks()
 {
   // go through all packages, adding changed ones to the solver task list
   packagedb db;
+  tasks.clear();
 
   for (packagedb::packagecollection::iterator p = db.packages.begin ();
        p != db.packages.end (); ++p)
@@ -602,6 +603,11 @@ SolverPool::use_test_packages(bool use_test_packages)
 // A wrapper around the libsolv solver
 // ---------------------------------------------------------------------------
 
+SolverSolution::SolverSolution(SolverPool &_pool) : pool(_pool), solv(NULL)
+{
+  queue_init(&job);
+}
+
 SolverSolution::~SolverSolution()
 {
   clear();
@@ -615,6 +621,7 @@ SolverSolution::clear()
       solver_free(solv);
       solv = NULL;
     }
+  queue_free(&job);
 }
 
 void
@@ -713,18 +720,9 @@ std::ostream &operator<<(std::ostream &stream,
   return stream;
 }
 
-bool
-SolverSolution::update(SolverTasks &tasks, updateMode update, bool use_test_packages, bool include_source)
+void
+SolverSolution::tasksToJobs(SolverTasks &tasks, updateMode update, Queue &job)
 {
-  Log (LOG_PLAIN) << "solving: " << tasks.tasks.size() << " tasks," <<
-    " update: " << (update ? "yes" : "no") << "," <<
-    " use test packages: " << (use_test_packages ? "yes" : "no") << "," <<
-    " include_source: " << (include_source ? "yes" : "no") << endLog;
-
-  pool.use_test_packages(use_test_packages);
-
-  Queue job;
-  queue_init(&job);
   // solver accepts a queue containing pairs of (cmd, id) tasks
   // cmd is job and selection flags ORed together
   for (SolverTasks::taskList::const_iterator i = tasks.tasks.begin();
@@ -745,11 +743,11 @@ SolverSolution::update(SolverTasks &tasks, updateMode update, bool use_test_pack
           // we don't know how to ask solver for this, so we just add the erase
           // and install later
           break;
-	case SolverTasks::taskKeep:
-	  queue_push2(&job, SOLVER_LOCK | SOLVER_SOLVABLE, sv.id);
-	  break;
-	case SolverTasks::taskSkip:
-	  queue_push2(&job, SOLVER_LOCK | SOLVER_SOLVABLE_NAME, sv.name_id());
+        case SolverTasks::taskKeep:
+          queue_push2(&job, SOLVER_LOCK | SOLVER_SOLVABLE, sv.id);
+          break;
+        case SolverTasks::taskSkip:
+          queue_push2(&job, SOLVER_LOCK | SOLVER_SOLVABLE_NAME, sv.name_id());
           break;
         default:
           Log (LOG_PLAIN) << "unknown task " << (*i).second << endLog;
@@ -776,6 +774,19 @@ SolverSolution::update(SolverTasks &tasks, updateMode update, bool use_test_pack
 
   // Ask solver to check dependencies of installed packages.
   queue_push2(&job, SOLVER_VERIFY | SOLVER_SOLVABLE_ALL, 0);
+}
+
+bool
+SolverSolution::update(SolverTasks &tasks, updateMode update, bool use_test_packages)
+{
+  Log (LOG_PLAIN) << "solving: " << tasks.tasks.size() << " tasks," <<
+    " update: " << (update ? "yes" : "no") << "," <<
+    " use test packages: " << (use_test_packages ? "yes" : "no") << "," << endLog;
+
+  pool.use_test_packages(use_test_packages);
+
+  queue_free(&job);
+  tasksToJobs(tasks, update, job);
 
   if (!solv)
     solv = solver_create(pool.pool);
@@ -785,11 +796,18 @@ SolverSolution::update(SolverTasks &tasks, updateMode update, bool use_test_pack
   solver_set_flag(solv, SOLVER_FLAG_DUP_ALLOW_VENDORCHANGE, 1);
   solver_set_flag(solv, SOLVER_FLAG_DUP_ALLOW_DOWNGRADE, 1);
   solver_solve(solv, &job);
-  queue_free(&job);
 
   int pcnt = solver_problem_count(solv);
   solver_printdecisions(solv);
 
+  solutionToTransactionList();
+
+  return (pcnt == 0);
+}
+
+void
+SolverSolution::solutionToTransactionList()
+{
   // get transactions for solution
   Transaction *t = solver_create_transaction(solv);
   transaction_order(t, 0);
@@ -797,7 +815,6 @@ SolverSolution::update(SolverTasks &tasks, updateMode update, bool use_test_pack
 
   // massage into SolverTransactions form
   trans.clear();
-
   for (int i = 0; i < t->steps.count; i++)
     {
       Id id = t->steps.elements[i];
@@ -806,6 +823,14 @@ SolverSolution::update(SolverTasks &tasks, updateMode update, bool use_test_pack
         trans.push_back(SolverTransaction(SolvableVersion(id, pool.pool), tt));
     }
 
+  transaction_free(t);
+
+  dumpTransactionList();
+}
+
+void
+SolverSolution::augmentTasks(SolverTasks &tasks)
+{
   // add install and remove tasks for anything marked as reinstall
   for (SolverTasks::taskList::const_iterator i = tasks.tasks.begin();
        i != tasks.tasks.end();
@@ -821,7 +846,11 @@ SolverSolution::update(SolverTasks &tasks, updateMode update, bool use_test_pack
                                             SolverTransaction::transInstall));
         }
     }
+}
 
+void
+SolverSolution::addSource(bool include_source)
+{
   // if include_source mode is on, also install source for everything we are
   // installing
   if (include_source)
@@ -840,11 +869,15 @@ SolverSolution::update(SolverTasks &tasks, updateMode update, bool use_test_pack
             }
         }
     }
+}
 
+void
+SolverSolution::dumpTransactionList() const
+{
   if (trans.size())
     {
       Log (LOG_PLAIN) << "Augmented Transaction List:" << endLog;
-      for (SolverTransactionList::iterator i = trans.begin ();
+      for (SolverTransactionList::const_iterator i = trans.begin ();
            i != trans.end ();
            ++i)
         {
@@ -854,10 +887,17 @@ SolverSolution::update(SolverTasks &tasks, updateMode update, bool use_test_pack
                           << std::setw(20) << i->version.Canonical_version() << endLog;
         }
     }
+  else
+    Log (LOG_PLAIN) << "Augmented Transaction List: is empty" << endLog;
+}
 
-  transaction_free(t);
+void SolverSolution::applyDefaultProblemSolutions()
+{
+  int pcnt = solver_problem_count(solv);
+  for (Id problem = 1; problem <= pcnt; problem++)
+    solver_take_solution(solv, problem, 1, &job);
 
-  return (pcnt == 0);
+  solutionToTransactionList();
 }
 
 const SolverTransactionList &
@@ -891,6 +931,7 @@ SolverSolution::report() const
       for (Id solution = 1; solution <= scnt; solution++)
         {
           r += "Solution " + std::to_string(solution) + "/" + std::to_string(scnt);
+          if (solution == 1) r += " (default)";
           r += "\n";
 
           Id p, rp, element;
diff --git a/libsolv.h b/libsolv.h
index cddf76f..78b6881 100644
--- a/libsolv.h
+++ b/libsolv.h
@@ -236,7 +236,7 @@ typedef std::vector<SolverTransaction> SolverTransactionList;
 class SolverSolution
 {
  public:
-  SolverSolution(SolverPool &_pool) : pool(_pool), solv(NULL) {};
+  SolverSolution(SolverPool &_pool);
   ~SolverSolution();
   void clear();
 
@@ -252,16 +252,23 @@ class SolverSolution
     updateBest,  // update to best version
     updateForce, // distupdate: downgrade if necessary to best version in repo
   };
-  bool update(SolverTasks &tasks, updateMode update, bool use_test_packages, bool include_source);
+  bool update(SolverTasks &tasks, updateMode update, bool use_test_packages);
+  void augmentTasks(SolverTasks &tasks);
+  void addSource(bool include_source);
+  void applyDefaultProblemSolutions();
   std::string report() const;
 
   const SolverTransactionList &transactions() const;
+  void dumpTransactionList() const;
 
  private:
   static SolverTransaction::transType type(Transaction *trans, int pos);
+  void tasksToJobs(SolverTasks &tasks, updateMode update, Queue &job);
+  void solutionToTransactionList();
 
   SolverPool &pool;
   Solver *solv;
+  Queue job;
   SolverTransactionList trans;
 };
 
diff --git a/package_db.cc b/package_db.cc
index 92fe4f9..e2443bf 100644
--- a/package_db.cc
+++ b/package_db.cc
@@ -602,7 +602,7 @@ packagedb::fillMissingCategory ()
 void
 packagedb::defaultTrust (SolverTasks &q, SolverSolution::updateMode mode, bool test)
 {
-  solution.update(q, mode, test, FALSE);
+  solution.update(q, mode, test);
 
   // reflect that task list into packagedb
   solution.trans2db();
diff --git a/prereq.cc b/prereq.cc
index bf7661a..5b3e785 100644
--- a/prereq.cc
+++ b/prereq.cc
@@ -90,6 +90,7 @@ long
 PrereqPage::OnNext ()
 {
   HWND h = GetHWND ();
+  packagedb db;
 
   if (!IsDlgButtonChecked (h, IDC_PREREQ_CHECK))
     {
@@ -109,10 +110,16 @@ PrereqPage::OnNext ()
             "NOTE!  User refused the default solutions!  "
             "Expect some packages to give errors or not function at all." << endLog;
 	  // Change the solver's transaction list to reflect the user's choices.
-	  packagedb db;
 	  db.solution.db2trans();
 	}
     }
+  else
+    {
+      db.solution.applyDefaultProblemSolutions();
+    }
+
+  PrereqChecker p;
+  p.finalize();
 
   return whatNext();
 }
@@ -157,8 +164,9 @@ PrereqPage::OnUnattended ()
 // implements class PrereqChecker
 // ---------------------------------------------------------------------------
 
-// instantiate the static member
+// instantiate the static members
 bool PrereqChecker::use_test_packages;
+SolverTasks PrereqChecker::q;
 
 bool
 PrereqChecker::isMet ()
@@ -170,11 +178,19 @@ PrereqChecker::isMet ()
   Progress.SetText3 ("");
 
   // Create task list corresponding to current state of package database
-  SolverTasks q;
   q.setTasks();
 
-  // apply solver to those tasks and global state (use test, include source)
-  return db.solution.update(q, SolverSolution::keep, use_test_packages, IncludeSource);
+  // apply solver to those tasks and global state (use test or not)
+  return db.solution.update(q, SolverSolution::keep, use_test_packages);
+}
+
+void
+PrereqChecker::finalize ()
+{
+  packagedb db;
+  db.solution.augmentTasks(q);
+  db.solution.addSource(IncludeSource);
+  db.solution.dumpTransactionList();
 }
 
 /* Formats problems and solutions as a string for display to the user.  */
@@ -205,6 +221,8 @@ do_prereq_check_thread(HINSTANCE h, HWND owner)
 
   if (p.isMet ())
     {
+      p.finalize();
+
       if (source == IDC_SOURCE_LOCALDIR)
 	Progress.SetActivateTask (WM_APP_START_INSTALL);  // install
       else
diff --git a/prereq.h b/prereq.h
index 5ae9323..f1561fa 100644
--- a/prereq.h
+++ b/prereq.h
@@ -40,10 +40,14 @@ public:
   // formats 'unmet' as a string for display
   void getUnmetString (std::string &s);
 
+  // finialize the transaction list
+  void finalize ();
+
   static void setTestPackages (bool t) { use_test_packages = t; };
 
 private:
   static bool use_test_packages;
+  static SolverTasks q;
 };
 
 #endif /* SETUP_PREREQ_H */
-- 
2.15.1