Improve updating package database when loading library dependencies

* Make use of `PackageUpdater` more efficient by avoiding multiple calls of
  `addDepsAndProvidesFromOtherPackage`
* Fix leaving old provides in the database by splitting the update via new
  `beginUpdate()` and `endUpdate()` functions

libpkg/data/database.cpp

@@ -894,6 +894,34 @@ PackageSpec LibPkg::PackageUpdater::findPackageWithID(const std::string &package
     return m_database.m_storage->packageCache.retrieve(*m_database.m_storage, &m_d->packagesTxn, packageName);
 }
 
+/*!
+ * \brief Begins updating the existing \a package with the specified \a packageID.
+ * \remarks
+ * - Do not use this function when PackageUpdate has been constructed with clear=true.
+ * - Call this method before modifying \a package. Then modify the package. Then call endUpdate().
+ */
+void PackageUpdater::beginUpdate(StorageID packageID, const std::shared_ptr<Package> &package)
+{
+    m_d->update(packageID, true, package);
+}
+
+/*!
+ * \brief Ends updating the existing \a package with the specified \a packageID.
+ * \remarks
+ * - Do not use this function when PackageUpdate has been constructed with clear=true.
+ * - Call this method after callsing beginUpdate() and modifying \a package.
+ */
+void PackageUpdater::endUpdate(StorageID packageID, const std::shared_ptr<Package> &package)
+{
+    const auto &storage = m_database.m_storage;
+    storage->packageCache.store(*m_database.m_storage, m_d->packagesTxn, packageID, package);
+    m_d->update(packageID, false, package);
+}
+
+/*!
+ * \brief Updates the specified \a package. The \a package may or may not exist.
+ * \remarks If the package exists then provides/deps from the existing package are taken over if appropriate.
+ */
 StorageID PackageUpdater::update(const std::shared_ptr<Package> &package)
 {
     const auto &storage = m_database.m_storage;

libpkg/data/database.h

@@ -120,6 +120,8 @@ struct LIBPKG_EXPORT PackageUpdater {
     ~PackageUpdater();
 
     PackageSpec findPackageWithID(const std::string &packageName);
+    void beginUpdate(StorageID packageID, const std::shared_ptr<Package> &package);
+    void endUpdate(StorageID packageID, const std::shared_ptr<Package> &package);
     StorageID update(const std::shared_ptr<Package> &package);
     bool insertFromDatabaseFile(const std::string &databaseFilePath);
     void commit();

libpkg/data/package.cpp

@@ -367,17 +367,26 @@ void PackageBase::clear()
     description.clear();
 }
 
+/*!
+ * \brief Returns whether addDepsAndProvidesFromOtherPackage() would take over the information (without force).
+ */
+bool Package::canDepsAndProvidesFromOtherPackage(const Package &otherPackage) const
+{
+    return !((otherPackage.origin != PackageOrigin::PackageContents && otherPackage.origin != PackageOrigin::CustomSource)
+        || version != otherPackage.version
+        || !(!packageInfo || buildDate.isNull() || (otherPackage.packageInfo && buildDate == otherPackage.buildDate)));
+}
+
+/*!
+ * \brief Takes over deps/provides from \a otherPackage if appropriate.
+ * \remarks
+ * The information is not taken over if \a otherPackage does not match the current instance (only
+ * version and build date are considered) or if otherPackage has no info from package contents. Use
+ * the \a force parameter to avoid these checks.
+ */
 bool Package::addDepsAndProvidesFromOtherPackage(const Package &otherPackage, bool force)
 {
-    if (&otherPackage == this) {
-        return false;
-    }
-
-    // skip if otherPackage does not match the current instance (only version and build date are considered) or if otherPackage has no info from package contents
-    if (!force
-        && ((otherPackage.origin != PackageOrigin::PackageContents && otherPackage.origin != PackageOrigin::CustomSource)
-            || version != otherPackage.version
-            || !(!packageInfo || buildDate.isNull() || (otherPackage.packageInfo && buildDate == otherPackage.buildDate)))) {
+    if (&otherPackage == this || (!force && !canDepsAndProvidesFromOtherPackage(otherPackage))) {
         return false;
     }
 

libpkg/data/package.h

@@ -415,6 +415,7 @@ struct LIBPKG_EXPORT Package : public PackageBase,
         std::string_view directoryPath, const ArchiveFile &file, std::set<std::string> &dllsReferencedByImportLibs);
     void addDepsAndProvidesFromContents(const FileMap &contents);
     std::vector<std::string> processDllsReferencedByImportLibs(std::set<std::string> &&dllsReferencedByImportLibs);
+    bool canDepsAndProvidesFromOtherPackage(const Package &otherPackage) const;
     bool addDepsAndProvidesFromOtherPackage(const Package &otherPackage, bool force = false);
     bool isArchAny() const;
     std::vector<std::string> validate() const;

libpkg/data/storage.cpp

@@ -129,6 +129,11 @@ auto StorageCache<StorageEntriesType, StorageType, SpecType>::retrieve(Storage &
     return retrieve(storage, nullptr, entryName);
 }
 
+/*!
+ * \brief Stores the specified \a entry.
+ * \remarks The entry may exist or may not exist. A lookup for an existing entry is done to take over
+ *          deps/provides from the existing entry if it makes sense.
+ */
 template <typename StorageEntriesType, typename StorageType, typename SpecType>
 auto StorageCache<StorageEntriesType, StorageType, SpecType>::store(Storage &storage, RWTxn &txn, const std::shared_ptr<Entry> &entry) -> StoreResult
 {
@@ -149,6 +154,7 @@ auto StorageCache<StorageEntriesType, StorageType, SpecType>::store(Storage &sto
         entry->addDepsAndProvidesFromOtherPackage(*res.oldEntry);
     }
     lock.unlock();
+
     // check for package in storage
     if (!res.oldEntry) {
         res.oldEntry = std::make_shared<Entry>();
@@ -158,8 +164,10 @@ auto StorageCache<StorageEntriesType, StorageType, SpecType>::store(Storage &sto
             res.oldEntry.reset();
         }
     }
+
     // update package in storage
     res.id = txn.put(*entry, res.id);
+
     // update cache entry
     lock = std::unique_lock(m_mutex);
     if (cacheEntry) {
@@ -169,10 +177,31 @@ auto StorageCache<StorageEntriesType, StorageType, SpecType>::store(Storage &sto
     }
     cacheEntry->entry = entry;
     lock.unlock();
+
     res.updated = true;
     return res;
 }
 
+/*!
+ * \brief Stores the specified \a entry with the specified \a storageID.
+ * \remarks This is used to update an existing entry with a known ID.
+ */
+template <typename StorageEntriesType, typename StorageType, typename SpecType>
+auto StorageCache<StorageEntriesType, StorageType, SpecType>::store(
+    Storage &storage, RWTxn &txn, StorageID storageID, const std::shared_ptr<Entry> &entry) -> void
+{
+    // update package in storage
+    const auto id = txn.put(*entry, storageID);
+
+    // update cache entry
+    using CacheEntry = typename Entries::StorageEntry;
+    using CacheRef = typename Entries::Ref;
+    const auto ref = CacheRef(storage, entry);
+    const auto lock = std::unique_lock(m_mutex);
+    const auto cacheEntry = &m_entries.insert(CacheEntry(ref, id));
+    cacheEntry->entry = entry;
+}
+
 template <typename StorageEntriesType, typename StorageType, typename SpecType>
 bool StorageCache<StorageEntriesType, StorageType, SpecType>::invalidate(Storage &storage, const std::string &entryName)
 {

libpkg/data/storagegeneric.h

@@ -152,6 +152,7 @@ template <typename StorageEntriesType, typename StorageType, typename SpecType>
     SpecType retrieve(Storage &storage, RWTxn *, const std::string &entryName);
     SpecType retrieve(Storage &storage, const std::string &entryName);
     StoreResult store(Storage &storage, RWTxn &txn, const std::shared_ptr<Entry> &entry);
+    void store(Storage &storage, RWTxn &txn, StorageID storageID, const std::shared_ptr<Entry> &entry);
     bool invalidate(Storage &storage, const std::string &entryName);
     bool invalidateCacheOnly(Storage &storage, const std::string &entryName);
     void clear(Storage &storage);

librepomgr/buildactions/reloadlibrarydependencies.cpp

@@ -377,19 +377,19 @@ void ReloadLibraryDependencies::loadPackageInfoFromContents()
             }
             // find the package in the database again
             const auto [packageID, existingPackage] = updater.findPackageWithID(package.info.name);
-            if (!existingPackage) {
-                continue; // the package has been removed while we were loading package contents
+            // skip if the package has been removed meanwhile if it it does no longer match what's in the database
+            if (!existingPackage || !existingPackage->canDepsAndProvidesFromOtherPackage(package.info)) {
+                continue;
             }
             // add the dependencies/provides to the existing package
-            if (!existingPackage->addDepsAndProvidesFromOtherPackage(package.info)) {
-                continue; // the package does no longer match what's in the database
-            }
+            updater.beginUpdate(packageID, existingPackage);
+            existingPackage->addDepsAndProvidesFromOtherPackage(package.info, true);
             // update timestamp so we can skip this package on the next run
             if (existingPackage->timestamp < package.lastModified) {
                 existingPackage->timestamp = package.lastModified;
             }
             // add the new dependencies on database-level
-            updater.update(existingPackage);
+            updater.endUpdate(packageID, existingPackage);
             ++counter;
         }
         updater.commit();

librepomgr/tests/buildactions.cpp

@@ -314,6 +314,7 @@ void BuildActionsTests::testParsingInfoFromPkgFiles()
     auto &fooDb = config.databases[0];
     auto &barDb = config.databases[1];
     const auto harfbuzz = LibPkg::Package::fromPkgFileName("mingw-w64-harfbuzz-1.4.2-1-any.pkg.tar.xz");
+    harfbuzz->libprovides = { "harfbuzzlibrary.so" };
     const auto harfbuzzID = fooDb.updatePackage(harfbuzz);
     const auto syncthingtray = LibPkg::Package::fromPkgFileName("syncthingtray-0.6.2-1-x86_64.pkg.tar.xz");
     const auto syncthingtrayID = fooDb.updatePackage(syncthingtray);
@@ -322,6 +323,14 @@ void BuildActionsTests::testParsingInfoFromPkgFiles()
     barDb.updatePackage(cmake);
     CPPUNIT_ASSERT_EQUAL_MESSAGE("origin", LibPkg::PackageOrigin::PackageFileName, cmake->origin);
     barDb.localPkgDir = directory(testFilePath("repo/bar/cmake-3.8.2-1-x86_64.pkg.tar.xz"));
+    auto harfbuzzLibraryPresent = false;
+    fooDb.providingPackages("harfbuzzlibrary.so", false, [&](LibPkg::StorageID id, const std::shared_ptr<LibPkg::Package> &package) {
+        CPPUNIT_ASSERT_EQUAL(harfbuzzID, id);
+        CPPUNIT_ASSERT_EQUAL(harfbuzz->name, package->name);
+        harfbuzzLibraryPresent = true;
+        return true;
+    });
+    CPPUNIT_ASSERT_MESSAGE("harfbuzz found via \"harfbuzzlibrary.so\" before reload", harfbuzzLibraryPresent);
 
     auto buildAction = std::make_shared<BuildAction>(0, &m_setup);
     auto reloadLibDependencies = ReloadLibraryDependencies(m_setup, buildAction);
@@ -345,6 +354,11 @@ void BuildActionsTests::testParsingInfoFromPkgFiles()
     CPPUNIT_ASSERT_EQUAL(1_st, pkgsProvidingLibSyncthingConnector.size());
     CPPUNIT_ASSERT_EQUAL(syncthingtray->name, pkgsProvidingLibSyncthingConnector.front().pkg->name);
     CPPUNIT_ASSERT_EQUAL(syncthingtrayID, pkgsProvidingLibSyncthingConnector.front().id);
+
+    fooDb.providingPackages("harfbuzzlibrary.so", false, [&](LibPkg::StorageID, const std::shared_ptr<LibPkg::Package> &) {
+        CPPUNIT_FAIL("harfbuzz still found via \"harfbuzzlibrary.so\" after reload");
+        return true;
+    });
 }
 
 /*!