Fix circular dependencies #116
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -50,11 +50,21 @@ func toConfig(install, forceIgnored []*api.Package, targets []string, cmdline [] | |
| } | ||
|
|
||
| providers := collectProviders(forceIgnored, install) | ||
| slices.Sort(targets) | ||
|
Collaborator
There was a problem hiding this comment. why do we care about the order of the targets?
Collaborator
Author
There was a problem hiding this comment. To make the lock file generation consistent and stable, we don't want multiple runs of the same required packages against the same db to be changing due to sorting differences |
||
| sortedPackages := make([]*bazeldnf.RPM, 0, len(allPackages)) | ||
|
Collaborator
There was a problem hiding this comment. maybe move this down to where sortedPackages is constructed so it's a bit more clear what's happening |
||
|
|
||
| requested := make(map[string]bool) | ||
|
Collaborator
There was a problem hiding this comment. should use empty struct instead of bool here as the former uses zero bytes |
||
| for _, pkg := range targets { | ||
| requested[pkg] = true | ||
| } | ||
|
|
||
| // make sure all requested packages have their full dependency tree set | ||
| for _, name := range sortedKeys(allPackages) { | ||
| pkg := allPackages[name] | ||
| if _, requested := requested[name]; !requested { | ||
|
Collaborator
There was a problem hiding this comment. should use |
||
| continue | ||
| } | ||
| deps, err := collectDependencies(name, pkg.Dependencies, providers, ignored, allPackages) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
@@ -64,6 +74,22 @@ func toConfig(install, forceIgnored []*api.Package, targets []string, cmdline [] | |
| sortedPackages = append(sortedPackages, pkg) | ||
| } | ||
|
|
||
| // now for non requested packages make sure we don't get cycles | ||
| for _, name := range sortedKeys(allPackages) { | ||
| pkg := allPackages[name] | ||
| if _, requested := requested[name]; requested { | ||
| continue | ||
| } | ||
|
|
||
| pkg.SetDependencies(nil) | ||
|
Collaborator
There was a problem hiding this comment. why set dependencies to nil here? |
||
|
|
||
| sortedPackages = append(sortedPackages, pkg) | ||
| } | ||
|
|
||
| slices.SortFunc(sortedPackages, func(a, b *bazeldnf.RPM) int { | ||
| return cmp.Compare(a.Name, b.Name) | ||
|
Collaborator
There was a problem hiding this comment. isn't the sorting already implicit by virtue of having constructed sortedPackages with the sorted keys? |
||
| }) | ||
|
|
||
| lockFile := bazeldnf.Config{ | ||
| CommandLineArguments: cmdline, | ||
| ForceIgnored: sortedKeys(ignored), | ||
|
|
@@ -92,9 +118,19 @@ func collectProviders(pkgSets ...[]*api.Package) map[string]string { | |
| return providers | ||
| } | ||
|
|
||
| func collectDependencies(pkg string, requires []string, providers map[string]string, ignored map[string]bool, allPackages map[string]*bazeldnf.RPM) ([]string, error) { | ||
|
Collaborator
There was a problem hiding this comment. can we use something like https://pkg.go.dev/github.com/dominikbraun/graph to do a topological sort of the dependencies here and just get the result of that? |
||
| logrus.Debugf("Collecting dependencies for %s", pkg) | ||
| depSet := make(map[string]bool) | ||
| explored := make(map[string]bool) | ||
| for len(requires) > 0 { | ||
|
Collaborator
There was a problem hiding this comment. Can we just use something like https://pkg.go.dev/github.com/dominikbraun/graph and do a topological sort? |
||
| req := requires[0] | ||
| requires = requires[1:] | ||
| logrus.Debugf("Processing dependency %s, pending %d", req, len(requires)) | ||
| if explored[req] { | ||
| logrus.Debugf("Ignoring already explored %s", req) | ||
| continue | ||
| } | ||
| explored[req] = true | ||
| if ignored[req] { | ||
| logrus.Debugf("Ignoring dependency %s", req) | ||
| continue | ||
|
|
@@ -104,28 +140,71 @@ func collectDependencies(pkg string, requires []string, providers map[string]str | |
| if !ok { | ||
| return nil, fmt.Errorf("could not find provider for %s", req) | ||
| } | ||
| if ignored[provider] { | ||
| logrus.Debugf("Ignoring provider %s", provider) | ||
| continue | ||
| } | ||
| depSet[provider] = true | ||
| requires = append(requires, allPackages[provider].Dependencies...) | ||
| } | ||
| return sortedKeys(depSet), nil | ||
| } | ||
|
|
||
| func removeCyclicDependencies(targets []string, allPackages []*bazeldnf.RPM) []*bazeldnf.RPM { | ||
| allPackagesMap := make(map[string]*bazeldnf.RPM) | ||
|
|
||
| for _, installPackage := range allPackages { | ||
| allPackagesMap[installPackage.Name] = installPackage | ||
| } | ||
|
|
||
| for _, target := range targets { | ||
| visitedMap := make(map[string]bool) | ||
| recursionStack := make(map[string]bool) | ||
|
|
||
| removeCyclicDependenciesHelper(allPackagesMap, target, visitedMap, recursionStack) | ||
| } | ||
|
|
||
| return allPackages | ||
| } | ||
|
|
||
| func removeCyclicDependenciesHelper(allPackages map[string]*bazeldnf.RPM, pkg string, visitedMap, recursionStack map[string]bool) bool { | ||
| /* | ||
| * This is a recursive function that removes cyclic dependencies from the | ||
| * dependency graph in the case cycles are found | ||
| */ | ||
| visitedMap[pkg] = true | ||
| recursionStack[pkg] = true | ||
|
|
||
| if _, ok := allPackages[pkg]; !ok { | ||
| return false | ||
| } | ||
|
|
||
| if allPackages[pkg].Dependencies == nil { | ||
| return false | ||
| } | ||
|
|
||
| cleanDependencies := make([]string, 0, len(allPackages[pkg].Dependencies)) | ||
|
|
||
| for _, dep := range allPackages[pkg].Dependencies { | ||
| if _, visited := visitedMap[dep]; !visited { | ||
| if removeCyclicDependenciesHelper(allPackages, dep, visitedMap, recursionStack) { | ||
| // ignore cycle | ||
| logrus.Debugf("Ignoring cyclic dependency %s -> %s", pkg, dep) | ||
| continue | ||
| } | ||
| } else if _, recursed := recursionStack[dep]; recursed { | ||
| // ignore cycle | ||
| logrus.Debugf("Ignoring cyclic dependency in recursion stack %s -> %s", pkg, dep) | ||
| continue | ||
| } | ||
| cleanDependencies = append(cleanDependencies, dep) | ||
| } | ||
|
|
||
| recursionStack[pkg] = false | ||
|
|
||
| newPkg := allPackages[pkg].Clone() | ||
| newPkg.SetDependencies(cleanDependencies) | ||
| allPackages[pkg] = newPkg | ||
|
|
||
| return false | ||
| } | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.