-
-
Notifications
You must be signed in to change notification settings - Fork 29
ADFA-4329: Guard module-graph traversal against dependency cycles (StackOverflow) #1408
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: stage
Are you sure you want to change the base?
Changes from all commits
deb93ee
0103296
3bc14fe
8269ee9
b9b4279
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -141,8 +141,18 @@ open class AndroidModule( | |
| addAll(getSelectedVariant()?.mainArtifact?.classJars ?: emptyList()) | ||
| } | ||
|
|
||
| override fun getCompileClasspaths(excludeSourceGeneratedClassPath: Boolean): Set<File> { | ||
| override fun getCompileClasspaths( | ||
| excludeSourceGeneratedClassPath: Boolean, | ||
| visited: MutableSet<String>, | ||
| ): Set<File> { | ||
| val project = IProjectManager.getInstance().workspace ?: return emptySet() | ||
|
|
||
| // Guard against cyclic project-dependency graphs: contribute each module's classpaths at most | ||
| // once so a cycle (:a -> :b -> :a) terminates instead of recursing until the stack overflows. | ||
| if (!visited.add(path)) { | ||
| return emptySet() | ||
| } | ||
|
|
||
| val result = mutableSetOf<File>() | ||
| if (excludeSourceGeneratedClassPath) { | ||
| // TODO: The mainArtifact.classJars are technically generated from source files | ||
|
|
@@ -159,6 +169,8 @@ open class AndroidModule( | |
| libraries = variantDependencies.mainArtifact?.compileDependencyList ?: emptyList(), | ||
| result = result, | ||
| excludeSourceGeneratedClassPath = excludeSourceGeneratedClassPath, | ||
| visited = HashSet(), | ||
| moduleVisited = visited, | ||
| ) | ||
| return result | ||
| } | ||
|
|
@@ -234,54 +246,106 @@ open class AndroidModule( | |
| return result | ||
| } | ||
|
|
||
| /** | ||
| * Recursively collect the compile classpath entries contributed by the given dependency-graph | ||
| * [libraries] into [result], guarding against cycles. | ||
| * | ||
| * @param root The workspace used to resolve project dependencies by path. | ||
| * @param libraries The dependency-graph nodes to expand at this level. | ||
| * @param result The accumulating set of classpath files. | ||
| * @param excludeSourceGeneratedClassPath Whether to exclude source-generated classpath entries. | ||
| * @param visited Keys of dependency-graph nodes already expanded within this module; each node is | ||
| * expanded at most once so a cyclic graph terminates. | ||
| * @param moduleVisited Module paths already expanded across modules; threaded into cross-module | ||
| * recursion so a cyclic PROJECT graph terminates. | ||
| */ | ||
| private fun collectLibraries( | ||
| root: Workspace, | ||
| libraries: List<AndroidModels.GraphItem>, | ||
| result: MutableSet<File>, | ||
| excludeSourceGeneratedClassPath: Boolean = false, | ||
| excludeSourceGeneratedClassPath: Boolean, | ||
| visited: MutableSet<String>, | ||
| moduleVisited: MutableSet<String>, | ||
| ) { | ||
| val libraryMap = variantDependencies.librariesMap | ||
| for (library in libraries) { | ||
| // Guard against cyclic dependency graphs within this module: expand each graph node once. | ||
| if (!visited.add(library.key)) { | ||
| continue | ||
| } | ||
|
|
||
| val lib = libraryMap[library.key] ?: continue | ||
| if (lib.type == AndroidModels.LibraryType.Project) { | ||
| val module = root.findByPath(lib.projectInfo!!.projectPath) ?: continue | ||
| if (module !is ModuleProject) { | ||
| continue | ||
| } | ||
|
|
||
| result.addAll(module.getCompileClasspaths(excludeSourceGeneratedClassPath)) | ||
| // Cross-module recursion threads moduleVisited so a cyclic PROJECT graph terminates. | ||
| result.addAll(module.getCompileClasspaths(excludeSourceGeneratedClassPath, moduleVisited)) | ||
| } else if (lib.type == AndroidModels.LibraryType.ExternalAndroidLibrary && lib.hasAndroidLibraryData()) { | ||
| result.addAll(lib.androidLibraryData.compileJarFiles) | ||
| } else if (lib.type == AndroidModels.LibraryType.ExternalJavaLibrary && lib.hasArtifactPath()) { | ||
| result.add(lib.artifact) | ||
| } | ||
|
|
||
| collectLibraries(root, library.dependencyList, result) | ||
| // Note: the recursive call intentionally keeps the original behavior of not propagating | ||
| // `excludeSourceGeneratedClassPath` (it defaults to false here); only the visited-set | ||
| // cycle guard is added. | ||
| collectLibraries( | ||
| root = root, | ||
| libraries = library.dependencyList, | ||
| result = result, | ||
| excludeSourceGeneratedClassPath = false, | ||
| visited = visited, | ||
| moduleVisited = moduleVisited, | ||
| ) | ||
|
Comment on lines
+292
to
+302
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The previous behavior was an oversight, so the |
||
| } | ||
| } | ||
|
|
||
| override fun getCompileModuleProjects(): List<ModuleProject> { | ||
| override fun getCompileModuleProjects( | ||
| visited: MutableSet<String>, | ||
| recursionPath: ArrayDeque<String>, | ||
| ): List<ModuleProject> { | ||
| val root = IProjectManager.getInstance().workspace ?: return emptyList() | ||
| val result = mutableListOf<ModuleProject>() | ||
|
|
||
| val libraries = variantDependencies.mainArtifact.compileDependencyList | ||
| val libraryMap = variantDependencies.librariesMap | ||
| for (library in libraries) { | ||
| val lib = libraryMap[library.key] ?: continue | ||
| if (lib.type != AndroidModels.LibraryType.Project) { | ||
| continue | ||
| } | ||
| // True cycle: this module is already an ancestor on the current recursion path | ||
| // (e.g. :a -> :b -> :a). Report it and break the cycle. | ||
| if (recursionPath.contains(path)) { | ||
| reportDependencyCycle(recursionPath, path) | ||
| return emptyList() | ||
| } | ||
|
|
||
| val module = root.findByPath(lib.projectInfo!!.projectPath) ?: continue | ||
| if (module !is ModuleProject) { | ||
| continue | ||
| // Already fully expanded elsewhere (a diamond / shared dependency, not a cycle): dedup. | ||
| if (!visited.add(path)) { | ||
| return emptyList() | ||
| } | ||
|
|
||
| recursionPath.addLast(path) | ||
| try { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here |
||
| val result = mutableListOf<ModuleProject>() | ||
|
|
||
| val libraries = variantDependencies.mainArtifact.compileDependencyList | ||
| val libraryMap = variantDependencies.librariesMap | ||
| for (library in libraries) { | ||
| val lib = libraryMap[library.key] ?: continue | ||
| if (lib.type != AndroidModels.LibraryType.Project) { | ||
| continue | ||
| } | ||
|
|
||
| val module = root.findByPath(lib.projectInfo!!.projectPath) ?: continue | ||
| if (module !is ModuleProject) { | ||
| continue | ||
| } | ||
|
|
||
| result.add(module) | ||
| result.addAll(module.getCompileModuleProjects(visited, recursionPath)) | ||
| } | ||
|
|
||
| result.add(module) | ||
| result.addAll(module.getCompileModuleProjects()) | ||
| return result | ||
| } finally { | ||
| recursionPath.removeLast() | ||
| } | ||
|
|
||
| return result | ||
| } | ||
|
|
||
| override fun hasExternalDependency( | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The newly added functions accept |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you refactor to avoid the nested structure?