From 1cb7b2a3443c9d3283f5f3390df939db8f8f279e Mon Sep 17 00:00:00 2001 From: Oliver Hamlet Date: Fri, 11 May 2018 12:44:57 +0100 Subject: [PATCH] Improve cycle avoidance when adding group edges during sorting Fixes #22. If a cycle is found in group-isolation, ignore the source plugin when adding edges for plugins in the all the groups that form paths between the source and target plugins in the cycle, instead of just that pair of plugins. --- src/api/sorting/plugin_sorter.cpp | 158 ++++++++++++++++-- src/api/sorting/plugin_sorter.h | 1 + .../internals/sorting/plugin_sorter_test.h | 139 ++++++++------- 3 files changed, 223 insertions(+), 75 deletions(-) diff --git a/src/api/sorting/plugin_sorter.cpp b/src/api/sorting/plugin_sorter.cpp index 4a8a601e..fe89603c 100644 --- a/src/api/sorting/plugin_sorter.cpp +++ b/src/api/sorting/plugin_sorter.cpp @@ -226,9 +226,9 @@ void PluginSorter::AddPluginVertices(Game& game) { auto groupIt = groupPlugins.find(metadata.GetGroup()); if (groupIt == groupPlugins.end()) { - groupPlugins.emplace(metadata.GetGroup(), std::vector({ plugin->GetName() })); - } - else { + groupPlugins.emplace(metadata.GetGroup(), + std::vector({plugin->GetName()})); + } else { groupIt->second.push_back(plugin->GetName()); } @@ -242,13 +242,15 @@ void PluginSorter::AddPluginVertices(Game& game) { // Map sets of transitive group dependencies to sets of transitive plugin // dependencies. - auto groups = GetTransitiveAfterGroups(game.GetDatabase()->GetGroups()); + groups_ = game.GetDatabase()->GetGroups(); + auto groups = GetTransitiveAfterGroups(groups_); for (auto& group : groups) { std::unordered_set transitivePlugins; for (const auto& afterGroup : group.second) { auto pluginsIt = groupPlugins.find(afterGroup); if (pluginsIt != groupPlugins.end()) { - transitivePlugins.insert(pluginsIt->second.begin(), pluginsIt->second.end()); + transitivePlugins.insert(pluginsIt->second.begin(), + pluginsIt->second.end()); } } group.second = transitivePlugins; @@ -256,19 +258,21 @@ void PluginSorter::AddPluginVertices(Game& game) { // Add all transitive plugin dependencies for a group to the plugin's load // after metadata. - for (const auto& vertex : boost::make_iterator_range(boost::vertices(graph_))) { + for (const auto& vertex : + boost::make_iterator_range(boost::vertices(graph_))) { PluginSortingData& plugin = graph_[vertex]; if (logger_) { - logger_->trace("Plugin \"{}\" belongs to group \"{}\", setting after group plugins", - plugin.GetName(), plugin.GetGroup()); + logger_->trace( + "Plugin \"{}\" belongs to group \"{}\", setting after group plugins", + plugin.GetName(), + plugin.GetGroup()); } auto groupsIt = groups.find(plugin.GetGroup()); if (groupsIt == groups.end()) { throw UndefinedGroupError(plugin.GetGroup()); - } - else { + } else { plugin.SetAfterGroupPlugins(groupsIt->second); } } @@ -377,27 +381,133 @@ void PluginSorter::AddSpecificEdges() { } } +bool shouldIgnorePlugin( + const std::string& group, + const std::string& pluginName, + const std::map>& + groupPluginsToIgnore) { + auto pluginsToIgnore = groupPluginsToIgnore.find(group); + if (pluginsToIgnore != groupPluginsToIgnore.end()) { + return pluginsToIgnore->second.count(boost::to_lower_copy(pluginName)) > 0; + } + + return false; +} + +void ignorePlugin(const std::string& pluginName, + const std::unordered_set& groups, + std::map>& + groupPluginsToIgnore) { + auto lowercasePluginName = boost::to_lower_copy(pluginName); + + for (const auto& group : groups) { + auto pluginsToIgnore = groupPluginsToIgnore.find(group); + if (pluginsToIgnore != groupPluginsToIgnore.end()) { + pluginsToIgnore->second.insert(lowercasePluginName); + } else { + groupPluginsToIgnore.emplace( + group, std::unordered_set({lowercasePluginName})); + } + } +} + +// Look for paths to targetGroupName from group. Don't pass visitedGroups by +// reference as each after group should be able to record paths independently. +std::unordered_set pathfinder( + const Group& group, + const std::string& targetGroupName, + const std::unordered_set& groups, + std::unordered_set visitedGroups) { + // If the current group is the target group, return the set of groups in the + // path leading to it. + if (group.GetName() == targetGroupName) { + return visitedGroups; + } + + if (group.GetAfterGroups().empty()) { + return std::unordered_set(); + } + + visitedGroups.insert(group.GetName()); + + // Call pathfinder on each after group. We want to find all paths, so merge + // all return values. + std::unordered_set mergedVisitedGroups; + for (const auto& afterGroupName : group.GetAfterGroups()) { + auto afterGroup = *groups.find(Group(afterGroupName)); + + auto recursedVisitedGroups = + pathfinder(afterGroup, targetGroupName, groups, visitedGroups); + + mergedVisitedGroups.insert(recursedVisitedGroups.begin(), + recursedVisitedGroups.end()); + } + + // Return mergedVisitedGroups if it is empty, to indicate the current group's + // after groups had no path to the target group. + if (mergedVisitedGroups.empty()) { + return mergedVisitedGroups; + } + + // If any after groups had paths to the target group, mergedVisitedGroups + // will be non-empty. To ensure that it contains full paths, merge it + // with visitedGroups and return that merged set. + visitedGroups.insert(mergedVisitedGroups.begin(), mergedVisitedGroups.end()); + + return visitedGroups; +} + +std::unordered_set getGroupsInPaths( + const std::unordered_set& groups, + const std::string& firstGroupName, + const std::string& lastGroupName) { + // Groups are linked in reverse order, i.e. firstGroup can be found from + // lastGroup, but not the other way around. + auto lastGroup = *groups.find(Group(lastGroupName)); + + return pathfinder( + lastGroup, firstGroupName, groups, std::unordered_set()); +} + void PluginSorter::AddGroupEdges() { if (logger_) { logger_->trace("Adding group edges."); } std::vector> acyclicEdgePairs; + std::map> groupPluginsToIgnore; for (const vertex_t& vertex : boost::make_iterator_range(boost::vertices(graph_))) { if (logger_) { logger_->trace("Checking group edges for \"{}\".", - graph_[vertex].GetName()); + graph_[vertex].GetName()); } for (const auto& pluginName : graph_[vertex].GetAfterGroupPlugins()) { vertex_t parentVertex; if (GetVertexByName(pluginName, parentVertex)) { - if (EdgeCreatesCycle(parentVertex, vertex)) { + bool ignore = shouldIgnorePlugin(graph_[vertex].GetGroup(), + graph_[parentVertex].GetName(), + groupPluginsToIgnore); + + if (ignore || EdgeCreatesCycle(parentVertex, vertex)) { if (logger_) { - logger_->trace("Skipping edge from \"{}\" to \"{}\" as it would " - "create a cycle.", - graph_[parentVertex].GetName(), - graph_[vertex].GetName()); + logger_->trace( + "Skipping edge from \"{}\" to \"{}\" as it would " + "create a cycle.", + graph_[parentVertex].GetName(), + graph_[vertex].GetName()); } + // Get the groups of the two plugins, and find all groups that are + // in paths that sit between those two groups. Group-derived edges + // from the plugin at parentVertex to any plugins in those groups + // should then be ignored as they also create cycles. + auto groupsInPaths = getGroupsInPaths(groups_, + graph_[parentVertex].GetGroup(), + graph_[vertex].GetGroup()); + + ignorePlugin(graph_[parentVertex].GetName(), + groupsInPaths, + groupPluginsToIgnore); + continue; } @@ -407,10 +517,22 @@ void PluginSorter::AddGroupEdges() { } if (logger_) { - logger_->trace("Adding group edges that don't individually introduce cycles."); + logger_->trace( + "Adding group edges that don't individually introduce cycles."); } for (const auto& edgePair : acyclicEdgePairs) { - AddEdge(edgePair.first, edgePair.second); + bool ignore = shouldIgnorePlugin(graph_[edgePair.second].GetGroup(), + graph_[edgePair.first].GetName(), + groupPluginsToIgnore); + if (!ignore) { + AddEdge(edgePair.first, edgePair.second); + } else if (logger_) { + logger_->trace( + "Skipping edge from \"{}\" to \"{}\" as it would " + "create a multi-group cycle.", + graph_[edgePair.first].GetName(), + graph_[edgePair.second].GetName()); + } } } diff --git a/src/api/sorting/plugin_sorter.h b/src/api/sorting/plugin_sorter.h index 1fa8fada..e3a0dc8e 100644 --- a/src/api/sorting/plugin_sorter.h +++ b/src/api/sorting/plugin_sorter.h @@ -70,6 +70,7 @@ private: vertex_map_t vertexIndexMap_; std::vector oldLoadOrder_; std::shared_ptr logger_; + std::unordered_set groups_; }; } diff --git a/src/tests/api/internals/sorting/plugin_sorter_test.h b/src/tests/api/internals/sorting/plugin_sorter_test.h index 1b01bba6..671fc7c1 100644 --- a/src/tests/api/internals/sorting/plugin_sorter_test.h +++ b/src/tests/api/internals/sorting/plugin_sorter_test.h @@ -83,16 +83,16 @@ protected: boost::filesystem::ofstream masterlist(masterlistPath_); masterlist << "groups:" << endl - << " - name: group1" << endl - << " - name: group2" << endl - << " after:" << endl - << " - group1" << endl - << " - name: group3" << endl - << " after:" << endl - << " - group2" << endl - << " - name: group4" << endl - << " after:" << endl - << " - default" << endl; + << " - name: group1" << endl + << " - name: group2" << endl + << " after:" << endl + << " - group1" << endl + << " - name: group3" << endl + << " after:" << endl + << " - group2" << endl + << " - name: group4" << endl + << " after:" << endl + << " - default" << endl; masterlist.close(); } @@ -180,18 +180,18 @@ TEST_P(PluginSorterTest, sortingShouldResolveGroupsAsTransitiveLoadAfterSets) { PluginSorter ps; std::vector expectedSortedOrder({ - masterFile, - blankDifferentEsm, - blankEsm, - blankMasterDependentEsm, - blankDifferentMasterDependentEsm, - blankEsp, - blankDifferentEsp, - blankMasterDependentEsp, - blankDifferentMasterDependentEsp, - blankPluginDependentEsp, - blankDifferentPluginDependentEsp, - }); + masterFile, + blankDifferentEsm, + blankEsm, + blankMasterDependentEsm, + blankDifferentMasterDependentEsm, + blankEsp, + blankDifferentEsp, + blankMasterDependentEsp, + blankDifferentMasterDependentEsp, + blankPluginDependentEsp, + blankDifferentPluginDependentEsp, + }); if (GetParam() == GameType::fo4 || GetParam() == GameType::tes5se) { expectedSortedOrder.insert(expectedSortedOrder.begin() + 5, blankEsl); @@ -212,29 +212,7 @@ TEST_P(PluginSorterTest, sortingShouldThrowIfAPluginHasAGroupThatDoesNotExist) { EXPECT_THROW(ps.Sort(game_), UndefinedGroupError); } -TEST_P(PluginSorterTest, sortingShouldThrowIfAddingTwoGroupEdgesIntroducesACycle) { - ASSERT_NO_THROW(loadInstalledPlugins(game_, false)); - - GenerateMasterlist(); - game_.GetDatabase()->LoadLists(masterlistPath_.string()); - - PluginMetadata plugin(blankMasterDependentEsm); - plugin.SetGroup("group1"); - game_.GetDatabase()->SetPluginUserMetadata(plugin); - - plugin = PluginMetadata(blankDifferentEsm); - plugin.SetGroup("group2"); - game_.GetDatabase()->SetPluginUserMetadata(plugin); - - plugin = PluginMetadata(blankEsm); - plugin.SetGroup("group3"); - game_.GetDatabase()->SetPluginUserMetadata(plugin); - - PluginSorter ps; - EXPECT_THROW(ps.Sort(game_), CyclicInteractionError); -} - -TEST_P(PluginSorterTest, sortingShouldIgnoreAGroupEdgeIfItWouldCauseACycle) { +TEST_P(PluginSorterTest, sortingShouldIgnoreAGroupEdgeIfItWouldCauseACycleInIsolation) { ASSERT_NO_THROW(loadInstalledPlugins(game_, false)); GenerateMasterlist(); @@ -246,18 +224,18 @@ TEST_P(PluginSorterTest, sortingShouldIgnoreAGroupEdgeIfItWouldCauseACycle) { PluginSorter ps; std::vector expectedSortedOrder({ - masterFile, - blankDifferentEsm, - blankDifferentMasterDependentEsm, - blankEsm, - blankMasterDependentEsm, - blankEsp, - blankDifferentEsp, - blankMasterDependentEsp, - blankDifferentMasterDependentEsp, - blankPluginDependentEsp, - blankDifferentPluginDependentEsp, - }); + masterFile, + blankDifferentEsm, + blankDifferentMasterDependentEsm, + blankEsm, + blankMasterDependentEsm, + blankEsp, + blankDifferentEsp, + blankMasterDependentEsp, + blankDifferentMasterDependentEsp, + blankPluginDependentEsp, + blankDifferentPluginDependentEsp, + }); if (GetParam() == GameType::fo4 || GetParam() == GameType::tes5se) { expectedSortedOrder.insert(expectedSortedOrder.begin() + 3, blankEsl); @@ -267,6 +245,53 @@ TEST_P(PluginSorterTest, sortingShouldIgnoreAGroupEdgeIfItWouldCauseACycle) { EXPECT_EQ(expectedSortedOrder, sorted); } +TEST_P(PluginSorterTest, + sortingShouldIgnoreGroupsThatContradictAnotherGroupInCombinationWithMoreSpecificMetadata) { + ASSERT_NO_THROW(loadInstalledPlugins(game_, false)); + + GenerateMasterlist(); + game_.GetDatabase()->LoadLists(masterlistPath_.string()); + + PluginMetadata plugin(blankEsp); + plugin.SetGroup("group1"); + game_.GetDatabase()->SetPluginUserMetadata(plugin); + + plugin = PluginMetadata(blankDifferentMasterDependentEsp); + plugin.SetGroup("group1"); + plugin.SetLoadAfterFiles(std::set({File(blankMasterDependentEsp)})); + game_.GetDatabase()->SetPluginUserMetadata(plugin); + + plugin = PluginMetadata(blankDifferentEsp); + plugin.SetGroup("group2"); + game_.GetDatabase()->SetPluginUserMetadata(plugin); + + plugin = PluginMetadata(blankMasterDependentEsp); + plugin.SetGroup("group3"); + game_.GetDatabase()->SetPluginUserMetadata(plugin); + + PluginSorter ps; + std::vector expectedSortedOrder({ + masterFile, + blankEsm, + blankDifferentEsm, + blankMasterDependentEsm, + blankDifferentMasterDependentEsm, + blankEsp, + blankDifferentEsp, + blankMasterDependentEsp, + blankDifferentMasterDependentEsp, + blankPluginDependentEsp, + blankDifferentPluginDependentEsp, + }); + + if (GetParam() == GameType::fo4 || GetParam() == GameType::tes5se) { + expectedSortedOrder.insert(expectedSortedOrder.begin() + 5, blankEsl); + } + + std::vector sorted = ps.Sort(game_); + EXPECT_EQ(expectedSortedOrder, sorted); +} + TEST_P(PluginSorterTest, sortingShouldUseLoadAfterMetadataWhenDecidingRelativePluginPositions) { ASSERT_NO_THROW(loadInstalledPlugins(game_, false));