[PATCH setup 0/3] Improve the handling of packagesource::sites

[PATCH setup 0/3] Improve the handling of packagesource::sites

[Ken Brown]

If several setup.ini files contain the same version of a package, we
currently create several packageversions in the libsolv pool, each of
which knows about one site.  This patchset consolidates those
packageversions into a single one, whose packagesource::sites vector
contains all the sites.

In the course of working on this, I found a problem with the way the
IniDBBuilderPackage destructor is called.  The first patch fixes that
problem.

Ken Brown (3):
  Make sure that the IniDBBuilderPackage destructor is called when
    needed
  Internalize the libsolv repo attribute data after each setup.ini
  Keep track of all known sites for a given version of a package

 IniDBBuilderPackage.cc | 13 ++++++++++++-
 ini.cc                 |  8 ++++----
 package_db.cc          |  2 --
 package_meta.cc        |  8 ++++++++
 4 files changed, 24 insertions(+), 7 deletions(-)

-- 
2.16.2

[PATCH setup 3/3] Keep track of all known sites for a given version of a package

[Ken Brown]

When adding a packageversion for an entry in setup.ini, make its
packagesource::sites vector include the sites from previously
processed ini files.

Also remove from the libsolv pool the previous packageversions, so
that libsolv will always find the one that lists all the sites.
---
 IniDBBuilderPackage.cc | 11 ++++++++++-
 package_meta.cc        |  8 ++++++++
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/IniDBBuilderPackage.cc b/IniDBBuilderPackage.cc
index 324f1bf..0b84def 100644
--- a/IniDBBuilderPackage.cc
+++ b/IniDBBuilderPackage.cc
@@ -148,6 +148,11 @@ IniDBBuilderPackage::buildPackageInstall (const std::string& path,
   // set archive path, size, mirror, hash
   cbpv.archive.set_canonical(path.c_str());
   cbpv.archive.size = atoi(size.c_str());
+  // do we already have some sites from previously read ini files?
+  packagedb db;
+  packageversion pv = db.findBinaryVersion(PackageSpecification(name, cbpv.version));
+  if (pv)
+    cbpv.archive.sites = pv.source()->sites;
   cbpv.archive.sites.push_back(site(parse_mirror));
 
   switch (type) {
@@ -190,6 +195,11 @@ IniDBBuilderPackage::buildPackageSource (const std::string& path,
   cspv.archive = packagesource();
   cspv.archive.set_canonical(path.c_str());
   cspv.archive.size = atoi(size.c_str());
+  // do we already have some sites from previously read ini files?
+  packagedb db;
+  packageversion pv = db.findBinaryVersion(PackageSpecification(name, cbpv.version));
+  if (pv)
+    cspv.archive.sites = pv.sourcePackage().source()->sites;
   cspv.archive.sites.push_back(site(parse_mirror));
 
   switch (type) {
@@ -210,7 +220,6 @@ IniDBBuilderPackage::buildPackageSource (const std::string& path,
     break;
   }
 
-  packagedb db;
   packageversion spkg_id = db.addSource (name + "-src", cspv);
 
   /* create relationship between binary and source packageversions */
diff --git a/package_meta.cc b/package_meta.cc
index 7f8110d..89b67bc 100644
--- a/package_meta.cc
+++ b/package_meta.cc
@@ -140,6 +140,14 @@ packagemeta::add_version (packageversion & thepkg, const SolverPool::addPackageD
   set <packageversion>::iterator i = versions.find(thepkg);
   if (i != versions.end())
     {
+      if (pkgdata.reponame != "_installed")
+	{
+	  if (curr == *i)
+	    curr = thepkg;
+	  if (exp == *i)
+	    exp = thepkg;
+	  i->remove();
+	}
       versions.erase(i);
     }
 
-- 
2.16.2

[PATCH setup 1/3] Make sure that the IniDBBuilderPackage destructor is called when needed

[Ken Brown]

The IniDBBuilderPackage destructor contains code that is intended to
be run after each setup.ini file is processed.  But the
IniDBBuilderPackage variables in do_local_ini() and do_remote_ini were
declared outside the loop that processed the files.  Move the
declaration inside the loop so that the destructor is called after
each iteration.
---
 ini.cc | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/ini.cc b/ini.cc
index d807ed6..7afeba2 100644
--- a/ini.cc
+++ b/ini.cc
@@ -209,13 +209,13 @@ static bool
 do_local_ini (HWND owner)
 {
   bool ini_error = false;
-  GuiParseFeedback myFeedback;
-  IniDBBuilderPackage aBuilder (myFeedback);
   io_stream *ini_file, *ini_sig_file;
   // iterate over all setup files found in do_from_local_dir
   for (IniList::const_iterator n = found_ini_list.begin ();
        n != found_ini_list.end (); ++n)
     {
+      GuiParseFeedback myFeedback;
+      IniDBBuilderPackage aBuilder (myFeedback);
       bool sig_fail = false;
       std::string current_ini_ext, current_ini_name, current_ini_sig_name;
 
@@ -268,8 +268,6 @@ static bool
 do_remote_ini (HWND owner)
 {
   bool ini_error = false;
-  GuiParseFeedback myFeedback;
-  IniDBBuilderPackage aBuilder (myFeedback);
   io_stream *ini_file = NULL, *ini_sig_file;
 
   /* FIXME: Get rid of this io_stream pointer travesty.  The need to
@@ -279,6 +277,8 @@ do_remote_ini (HWND owner)
   for (SiteList::const_iterator n = site_list.begin ();
        n != site_list.end (); ++n)
     {
+      GuiParseFeedback myFeedback;
+      IniDBBuilderPackage aBuilder (myFeedback);
       bool sig_fail = false;
       std::string current_ini_ext, current_ini_name, current_ini_sig_name;
       // iterate over known extensions for setup
-- 
2.16.2

[PATCH setup 2/3] Internalize the libsolv repo attribute data after each setup.ini

[Ken Brown]

Call SolverPool::internalize() in the IniDBBuilderPackage destructor.
This makes attribute data from all previously processed setup.ini
files available when the next setup.ini is being processed.

Remove the now unneeded call to SolverPool::internalize() at the
beginning of packagedb::read().
---
 IniDBBuilderPackage.cc | 2 ++
 package_db.cc          | 2 --
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/IniDBBuilderPackage.cc b/IniDBBuilderPackage.cc
index d560cb7..324f1bf 100644
--- a/IniDBBuilderPackage.cc
+++ b/IniDBBuilderPackage.cc
@@ -37,6 +37,8 @@ currentSpec (0), _feedback (aFeedback){}
 IniDBBuilderPackage::~IniDBBuilderPackage()
 {
   process();
+  packagedb db;
+  db.solver.internalize();
 }
 
 void
diff --git a/package_db.cc b/package_db.cc
index 072b419..d12e841 100644
--- a/package_db.cc
+++ b/package_db.cc
@@ -71,8 +71,6 @@ packagedb::read ()
 {
   if (!installeddbread)
     {
-      solver.internalize();
-
       /* Read in the local installation database. */
       io_stream *db = 0;
       db = io_stream::open ("cygfile:///etc/setup/installed.db", "rt", 0);
-- 
2.16.2

Re: [PATCH setup 1/3] Make sure that the IniDBBuilderPackage destructor is called when needed

[Jon Turney]

On 17/03/2018 14:59, Ken Brown wrote:
> The IniDBBuilderPackage destructor contains code that is intended to
> be run after each setup.ini file is processed.  But the
> IniDBBuilderPackage variables in do_local_ini() and do_remote_ini were
> declared outside the loop that processed the files.  Move the
> declaration inside the loop so that the destructor is called after
> each iteration.

Applied, thanks.

I need to think a bit more about 2 & 3 in this series and the concerns I 
had in [1]

[1 ]https://sourceware.org/ml/cygwin-apps/2018-03/msg00026.html

Re: [PATCH setup 3/3] Keep track of all known sites for a given version of a package

[Jon Turney]

On 17/03/2018 14:59, Ken Brown wrote:
> When adding a packageversion for an entry in setup.ini, make its
> packagesource::sites vector include the sites from previously
> processed ini files.
> 
> Also remove from the libsolv pool the previous packageversions, so
> that libsolv will always find the one that lists all the sites.

Thanks.

I think that this might be better done in packagemeta::add_version() as 
you originally proposed, although that does involve moving more things 
around.

A couple of patches which replace this one to do that follow.

> diff --git a/package_meta.cc b/package_meta.cc
> index 7f8110d..89b67bc 100644
> --- a/package_meta.cc
> +++ b/package_meta.cc
> @@ -140,6 +140,14 @@ packagemeta::add_version (packageversion & thepkg, const SolverPool::addPackageD
>     set <packageversion>::iterator i = versions.find(thepkg);
>     if (i != versions.end())
>       {
> +      if (pkgdata.reponame != "_installed")
> +	{
> +	  if (curr == *i)
> +	    curr = thepkg;
> +	  if (exp == *i)
> +	    exp = thepkg;
> +	  i->remove();
> +	}

A comment would have been useful here.  It took a bit of head scratching 
and testing with this removed to understand what this part is doing.

[PATCH setup 2/2] Keep track of all known sites for a given version of a package

[Jon Turney]

When adding a packageversion, extend it's packagesource::sites vector to
include the sites from any similar packageversions previously processed.

Also remove those packageversions from the libsolv pool so that libsolv will
always find the one that lists all the sites.

This is needed for:

- Correct handling of local installs where required packages are spread
between cache directories for more than one mirror

- Correctly falling back to a second site when multiple mirrors are used and
a problem occurs connecting to the first mirror tried.
---
 package_db.cc   | 14 +++------
 package_meta.cc | 80 ++++++++++++++++++++++++++++++++++++++-----------
 package_meta.h  |  2 +-
 3 files changed, 67 insertions(+), 29 deletions(-)

diff --git a/package_db.cc b/package_db.cc
index 945b339..b74aafd 100644
--- a/package_db.cc
+++ b/package_db.cc
@@ -245,11 +245,8 @@ packagedb::addBinary (const std::string &pkgname,
       packages.insert (packagedb::packagecollection::value_type(pkgname, pkg));
     }
 
-  /* Create the SolvableVersion  */
-  SolvableVersion sv = solver.addPackage(pkgname, pkgdata);
-
-  /* Register it in packagemeta */
-  pkg->add_version (sv, pkgdata);
+  /* Create the SolvableVersion and register it in packagemeta */
+  pkg->add_version (pkgdata);
 
   return pkg;
 }
@@ -266,11 +263,8 @@ packagedb::addSource (const std::string &pkgname,
       sourcePackages.insert (packagedb::packagecollection::value_type(pkgname, pkg));
     }
 
-  /* Create the SolvableVersion  */
-  SolvableVersion sv = solver.addPackage(pkgname, pkgdata);
-
-  /* Register it in packagemeta */
-  pkg->add_version (sv, pkgdata);
+  /* Create the SolvableVersion and register it in packagemeta */
+  SolvableVersion sv = pkg->add_version (pkgdata);
 
   return sv;
 }
diff --git a/package_meta.cc b/package_meta.cc
index a651d28..f765baf 100644
--- a/package_meta.cc
+++ b/package_meta.cc
@@ -123,11 +123,26 @@ packagemeta::~packagemeta()
   versions.clear ();
 }
 
-void
-packagemeta::add_version (packageversion & thepkg, const SolverPool::addPackageData &pkgdata)
+SolvableVersion
+packagemeta::add_version (const SolverPool::addPackageData &inpkgdata)
 {
+  SolverPool::addPackageData pkgdata = inpkgdata;
+
+  packageversion *v = NULL;
+  switch (pkgdata.stability)
+    {
+    case TRUST_CURR:
+      v = &(this->curr);
+      break;
+    case TRUST_TEST:
+      v = &(this->exp);
+      break;
+    default:
+      break;
+    }
+
   /*
-    If a packageversion for the same version number is already present,allow
+    If a packageversion for the same version number is already present, allow
     this version to replace it.
 
     There is a problem where multiple repos provide a package.  It's never been
@@ -137,12 +152,52 @@ packagemeta::add_version (packageversion & thepkg, const SolverPool::addPackageD
     We rely on this by adding packages from installed.db last.
    */
 
-  set <packageversion>::iterator i = versions.find(thepkg);
-  if (i != versions.end())
+  for (set <packageversion>::iterator i = versions.begin();
+       i != versions.end();
+       i++)
     {
+      if (i->Canonical_version() != pkgdata.version)
+        continue;
+
+      if (pkgdata.vendor == i->Vendor())
+        {
+          /* Merge the site-list from any existing packageversion with the same
+             repository 'release:' label */
+          pkgdata.archive.sites.insert(pkgdata.archive.sites.end(),
+                                       i->source()->sites.begin(),
+                                       i->source()->sites.end());
+
+          /* Installed packages do not supersede repo packages */
+          if (pkgdata.reponame != "_installed")
+            {
+              /* Ensure a stability level doesn't point to a version we're about
+                 to remove */
+              if (v && (*v == *i))
+                *v = packageversion();
+
+              i->remove();
+            }
+        }
+      else
+        {
+          /* Otherwise... if we had a way to set repo priorities, that could be
+             used to control which packageversion the solver picks. For the
+             moment, just warn that you might not be getting what you think you
+             should... */
+          Log (LOG_PLAIN) << "Version " << pkgdata.version << " of package " <<
+            name << " is present in releases labelled " << pkgdata.vendor <<
+            " and " << i->Vendor() << endLog;
+        }
+
       versions.erase(i);
+
+      break;
     }
 
+  /* Create the SolvableVersion  */
+  packagedb db;
+  SolvableVersion thepkg = db.solver.addPackage(name, pkgdata);
+
   /* Add the version */
   std::pair<std::set <packageversion>::iterator, bool> result = versions.insert (thepkg);
 
@@ -154,19 +209,6 @@ packagemeta::add_version (packageversion & thepkg, const SolverPool::addPackageD
 #endif
 
   /* Record the highest version at a given stability level */
-  packageversion *v = NULL;
-  switch (pkgdata.stability)
-    {
-    case TRUST_CURR:
-      v = &(this->curr);
-      break;
-    case TRUST_TEST:
-      v = &(this->exp);
-      break;
-    default:
-      break;
-    }
-
   if (v)
     {
       /* Any version is always greater than no version */
@@ -184,6 +226,8 @@ packagemeta::add_version (packageversion & thepkg, const SolverPool::addPackageD
           *v = thepkg;
         }
     }
+
+  return thepkg;
 }
 
 bool
diff --git a/package_meta.h b/package_meta.h
index 600a163..8db10e2 100644
--- a/package_meta.h
+++ b/package_meta.h
@@ -43,7 +43,7 @@ public:
 
   ~packagemeta ();
 
-  void add_version (packageversion &, const SolverPool::addPackageData &);
+  SolvableVersion add_version (const SolverPool::addPackageData &);
   void set_installed_version (const std::string &);
   void addToCategoryBase();
   bool hasNoCategories() const;
-- 
2.17.0

[PATCH setup 1/2] Add Vendor() accessor method to SolvableVersion

[Jon Turney]

---
 libsolv.cc | 17 +++++++++++++++++
 libsolv.h  |  1 +
 2 files changed, 18 insertions(+)

diff --git a/libsolv.cc b/libsolv.cc
index 63942b2..955a1b2 100644
--- a/libsolv.cc
+++ b/libsolv.cc
@@ -209,6 +209,23 @@ SolvableVersion::sourcePackageName () const
   return std::string(pool_id2str(pool, spkg));
 }
 
+const std::string
+SolvableVersion::Vendor () const
+{
+  if (!id)
+    return "";
+
+  // extract vendor
+  Solvable *solvable = pool_id2solvable(pool, id);
+  Id vendor = repo_lookup_id(solvable->repo, id, SOLVABLE_VENDOR);
+
+  // has no such attribute
+  if (!vendor)
+    return "";
+
+  return std::string(pool_id2str(pool, vendor));
+}
+
 SolvableVersion
 SolvableVersion::sourcePackage () const
 {
diff --git a/libsolv.h b/libsolv.h
index c218ab8..f394e65 100644
--- a/libsolv.h
+++ b/libsolv.h
@@ -73,6 +73,7 @@ class SolvableVersion
   SolvableVersion sourcePackage () const;
   // where this package archive can be obtained from
   packagesource *source() const;
+  const std::string Vendor () const;
 
   // fixup spkg_id
   void fixup_spkg_id(SolvableVersion spkg_id) const;
-- 
2.17.0

Re: [PATCH setup 3/3] Keep track of all known sites for a given version of a package

[Ken Brown]

On 7/9/2018 2:17 PM, Jon Turney wrote:
> On 17/03/2018 14:59, Ken Brown wrote:
>> When adding a packageversion for an entry in setup.ini, make its
>> packagesource::sites vector include the sites from previously
>> processed ini files.
>>
>> Also remove from the libsolv pool the previous packageversions, so
>> that libsolv will always find the one that lists all the sites.
> 
> Thanks.
> 
> I think that this might be better done in packagemeta::add_version() as 
> you originally proposed, although that does involve moving more things 
> around. >
> A couple of patches which replace this one to do that follow.

I agree that your approach (or my original approach?) is better.  I'm 
not sure why I gave up on it.

>> diff --git a/package_meta.cc b/package_meta.cc
>> index 7f8110d..89b67bc 100644
>> --- a/package_meta.cc
>> +++ b/package_meta.cc
>> @@ -140,6 +140,14 @@ packagemeta::add_version (packageversion & 
>> thepkg, const SolverPool::addPackageD
>>     set <packageversion>::iterator i = versions.find(thepkg);
>>     if (i != versions.end())
>>       {
>> +      if (pkgdata.reponame != "_installed")
>> +    {
>> +      if (curr == *i)
>> +        curr = thepkg;
>> +      if (exp == *i)
>> +        exp = thepkg;
>> +      i->remove();
>> +    }
> 
> A comment would have been useful here.  It took a bit of head scratching 
> and testing with this removed to understand what this part is doing.

Sorry about that.  I remember that it took me several tries to get it 
right, and then I didn't think to document it.

Ken