[[PATCH setup topic/libsolv] 1/2] packagedb::removeEmptyCategories: Don't remove "Base"

[[PATCH setup topic/libsolv] 1/2] packagedb::removeEmptyCategories: Don't remove "Base"

[Ken Brown]

This can be empty if no setup.ini files are found.  Removing it causes
setup to hang.
---
 package_db.cc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/package_db.cc b/package_db.cc
index ac9387c..b104073 100644
--- a/package_db.cc
+++ b/package_db.cc
@@ -596,7 +596,7 @@ packagedb::removeEmptyCategories()
 {
   for (packagedb::categoriesType::iterator n = packagedb::categories.begin();
        n != packagedb::categories.end(); ++n)
-    if (!n->second.size())
+    if (!n->second.size() && n->first != "Base")
       {
         Log (LOG_BABBLE) << "Removing empty category " << n->first << endLog;
         packagedb::categories.erase (n++);
-- 
2.14.2

[[PATCH setup topic/libsolv] 2/2] Avoid clobbering installed.db when no setup.ini is found

[Ken Brown]

Move the calls to packagedb::read and other packagedb functions from
do_ini_thread to ChooserPage::OnInit.  If no setup.ini is found,
do_ini_thread is never called.  But we need to ensure that
packagedb::read is called, or else installed.db gets emptied.
---
 choose.cc | 5 +++++
 ini.cc    | 7 -------
 2 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/choose.cc b/choose.cc
index 619d7db..013a30a 100644
--- a/choose.cc
+++ b/choose.cc
@@ -268,6 +268,11 @@ ChooserPage::OnInit ()
     packagemeta::ScanDownloadedFiles (MirrorOption);
 
   packagedb db;
+  db.makeBase();
+  db.read();
+  db.upgrade();
+  db.fixup_source_package_ids();
+  db.removeEmptyCategories();
   db.setExistence ();
   db.fillMissingCategory ();
 
diff --git a/ini.cc b/ini.cc
index 5089e8b..0f8b927 100644
--- a/ini.cc
+++ b/ini.cc
@@ -352,13 +352,6 @@ do_ini_thread (HINSTANCE h, HWND owner)
   else
     ini_count = do_remote_ini (owner);
 
-  packagedb db;
-  db.makeBase();
-  db.read();
-  db.upgrade();
-  db.fixup_source_package_ids();
-  db.removeEmptyCategories();
-
   if (ini_count == 0)
     return false;
 
-- 
2.14.2

Re: [[PATCH setup topic/libsolv] 2/2] Avoid clobbering installed.db when no setup.ini is found

[Ken Brown]

On 10/28/2017 8:29 AM, Ken Brown wrote:
> Move the calls to packagedb::read and other packagedb functions from
> do_ini_thread to ChooserPage::OnInit.  If no setup.ini is found,
> do_ini_thread is never called.  But we need to ensure that
> packagedb::read is called, or else installed.db gets emptied.
> ---
>   choose.cc | 5 +++++
>   ini.cc    | 7 -------
>   2 files changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/choose.cc b/choose.cc
> index 619d7db..013a30a 100644
> --- a/choose.cc
> +++ b/choose.cc
> @@ -268,6 +268,11 @@ ChooserPage::OnInit ()
>       packagemeta::ScanDownloadedFiles (MirrorOption);
>   
>     packagedb db;
> +  db.makeBase();
> +  db.read();
> +  db.upgrade();
> +  db.fixup_source_package_ids();
> +  db.removeEmptyCategories();
>     db.setExistence ();
>     db.fillMissingCategory ();

Sorry, this isn't quite right.  The new calls need happen before 
ScanDownLoadedFiles is called, to avoid a crash when the latter looks 
for the installation tarballs.

I'll send a revised patch shortly.

Ken

Re: [[PATCH setup topic/libsolv] 1/2] packagedb::removeEmptyCategories: Don't remove "Base"

[Jon Turney]

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

On 28/10/2017 13:29, Ken Brown wrote:
> This can be empty if no setup.ini files are found.  Removing it causes
> setup to hang.
> ---
>   package_db.cc | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/package_db.cc b/package_db.cc
> index ac9387c..b104073 100644
> --- a/package_db.cc
> +++ b/package_db.cc
> @@ -596,7 +596,7 @@ packagedb::removeEmptyCategories()
>   {
>     for (packagedb::categoriesType::iterator n = packagedb::categories.begin();
>          n != packagedb::categories.end(); ++n)
> -    if (!n->second.size())
> +    if (!n->second.size() && n->first != "Base")
>         {
>           Log (LOG_BABBLE) << "Removing empty category " << n->first << endLog;
>           packagedb::categories.erase (n++);
> 

Hmm... now I remember my other concerns about this piece of code: as 
written, it's just wrong.

1. Applying erase to packagedb:categories invalidates the iterator
2. We're incrementing the iterator after doing an erase, so even if the 
iterator was still valid, we skip checking if the following category is 
empty

So maybe the right way to fix this is as attached:

I need to stare as this a bit more to understand where the 'base' 
category is coming from when we have no setup.ini...

[-- Attachment #2: 0001-Fix-invalid-iterator-use-in-packagedb-removeEmptyCat.patch --]
[-- Type: text/plain, Size: 1300 bytes --]

From db1028c2ec19fd84366663b1384888dd33241202 Mon Sep 17 00:00:00 2001
From: Jon Turney <jon.turney@dronecode.org.uk>
Date: Sun, 29 Oct 2017 17:20:51 +0000
Subject: [PATCH setup] Fix invalid iterator use in
 packagedb::removeEmptyCategories()

---
 package_db.cc | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/package_db.cc b/package_db.cc
index 4d2ef4d..9de0875 100644
--- a/package_db.cc
+++ b/package_db.cc
@@ -593,13 +593,22 @@ packagedb::defaultTrust (SolverTasks &q, SolverSolution::updateMode mode, bool t
 void
 packagedb::removeEmptyCategories()
 {
+  std::vector<std::string> empty;
+
   for (packagedb::categoriesType::iterator n = packagedb::categories.begin();
        n != packagedb::categories.end(); ++n)
     if (!n->second.size())
       {
-        Log (LOG_BABBLE) << "Removing empty category " << n->first << endLog;
-        packagedb::categories.erase (n++);
+        empty.push_back(n->first);
       }
+
+  for (unsigned int i = 0; i < empty.size(); ++i)
+    {
+      packagedb::categoriesType::iterator n = packagedb::categories.find(empty[i]);
+      Log (LOG_BABBLE) << "Removing empty category " << empty[i] << endLog;
+      if (n != packagedb::categories.end())
+        packagedb::categories.erase(n);
+    }
 }
 
 void
-- 
2.14.3

Re: [[PATCH setup topic/libsolv] 1/2] packagedb::removeEmptyCategories: Don't remove "Base"

[Ken Brown]

On 10/29/2017 1:24 PM, Jon Turney wrote:
> On 28/10/2017 13:29, Ken Brown wrote:
>> This can be empty if no setup.ini files are found.  Removing it causes
>> setup to hang.
>> ---
>>   package_db.cc | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/package_db.cc b/package_db.cc
>> index ac9387c..b104073 100644
>> --- a/package_db.cc
>> +++ b/package_db.cc
>> @@ -596,7 +596,7 @@ packagedb::removeEmptyCategories()
>>   {
>>     for (packagedb::categoriesType::iterator n = 
>> packagedb::categories.begin();
>>          n != packagedb::categories.end(); ++n)
>> -    if (!n->second.size())
>> +    if (!n->second.size() && n->first != "Base")
>>         {
>>           Log (LOG_BABBLE) << "Removing empty category " << n->first 
>> << endLog;
>>           packagedb::categories.erase (n++);
>>
> 
> Hmm... now I remember my other concerns about this piece of code: as 
> written, it's just wrong.
> 
> 1. Applying erase to packagedb:categories invalidates the iterator
> 2. We're incrementing the iterator after doing an erase, so even if the 
> iterator was still valid, we skip checking if the following category is 
> empty
> 
> So maybe the right way to fix this is as attached:

Yes.  I wrongly jumped to the conclusion that removing Base was the issue.

> I need to stare as this a bit more to understand where the 'base' 
> category is coming from when we have no setup.ini...

I guess it's created implicitly in the 'for' statement in 
packagedb::makeBase().  That's probably a mistake.

Ken

Re: [[PATCH setup topic/libsolv] 1/2] packagedb::removeEmptyCategories: Don't remove "Base"

[Jon Turney]

On 29/10/2017 21:18, Ken Brown wrote:
> On 10/29/2017 1:24 PM, Jon Turney wrote:
>> Hmm... now I remember my other concerns about this piece of code: as 
>> written, it's just wrong.
>>
>> 1. Applying erase to packagedb:categories invalidates the iterator
>> 2. We're incrementing the iterator after doing an erase, so even if 
>> the iterator was still valid, we skip checking if the following 
>> category is empty
>>
>> So maybe the right way to fix this is as attached:
> 
> Yes.  I wrongly jumped to the conclusion that removing Base was the issue.
> 
>> I need to stare as this a bit more to understand where the 'base' 
>> category is coming from when we have no setup.ini...
> 
> I guess it's created implicitly in the 'for' statement in 
> packagedb::makeBase().  That's probably a mistake.

Ah yes, I guess this could check if the 'base' category exists first.

This is all a bit broken though.  Because we've forgotten the categories 
of installed packages, if we're run without a setup.ini, we'll merrily 
let packages in the base category get uninstalled without complaint...

Re: [[PATCH setup topic/libsolv] 1/2] packagedb::removeEmptyCategories: Don't remove "Base"

[Ken Brown]

On 10/30/2017 11:59 AM, Jon Turney wrote:
> This is all a bit broken though.  Because we've forgotten the categories 
> of installed packages, if we're run without a setup.ini, we'll merrily 
> let packages in the base category get uninstalled without complaint...

Maybe we need to add something to the existing warning.  It currently 
says, "You can still use setup-<arch>.exe to remove installed packages, 
but there will be nothing to install.  Press OK if that's what you 
wanted or Cancel to choose a different directory."  We could add that 
they risk uninstalling required packages if they proceed.

Of course, the same thing applies even if one or more setup.ini files 
are found but don't include the base packages or the dependencies of 
installed packages.  In this case, however, it means that the user has a 
homemade setup.ini, so we probably have to assume that they know what 
they're doing.

Ken