[Package/ModuleGraph] Allow cyclic package dependencies if they don't introduce a cycle in a build graph #7530
Conversation
This file contains 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
…topologicalSort It's too early to diagnose cycles here because cyclic package-level dependencies don't necessary lead to cycles in build graph, only target/product element cycles are important.
`load` builds a list of packages in topological order already and that's exactly what ModulesGraph wants, let's pass resolved packages to it directly instead of having to re-build the list.
…dentifier` Another step towards recursive dependency support - `ResolvedPackage` identifies its dependencies but refers back to the `ModulesGraph` to fetch underlying packages. This helps to avoid possible infinite recursion while building `ResolvedPackage` via existing builder pattern. There are really only two places where recursive walk over dependencies is needed - `PluginContextSerializer` and `ShowDependencies` command, both of which have module graph available.
…using package set Since `packages` is now an identifiable set no additional storage is needed to find package from a module or a product it contains.
Avoids duplicating the work and allows dependencies to have cyclic entries because cyclic product and/or target references are the only problematic cases for the build since they cannot be planned properly.
It cannot be done sooner because `Package` only knows about package names of products but doesn't attempt to resolve them and attempting to build resolved packages with cycles would result in an infinite recusion because builders are reference types.
|
@swift-ci please test |
…ageIdentifier)` API
|
@swift-ci please test |
xedin
added a commit
to xedin/sourcekit-lsp
that referenced
this pull request
May 7, 2024
`ModulesGraph.init` was changed by swiftlang/swift-package-manager#7530 to accept `packages` as a way to avoid having to recompute the full list of packages by walking roots.
|
swiftlang/sourcekit-lsp#1233 |
MaxDesiatov
reviewed
May 7, 2024
MaxDesiatov
approved these changes
May 7, 2024
MaxDesiatov
added
modules graph
|
@xedin would you mind updating |
|
Will do! |
xedin
force-pushed
the
fix-cross-package-cycles
branch
from
e2525b3
to
e3a16d4
Compare
|
swiftlang/sourcekit-lsp#1233 |
|
swiftlang/sourcekit-lsp#1233 |
|
@MaxDesiatov All done! |
|
Windows failure seems to be spurious |
|
@swift-ci test windows |
|
swiftlang/sourcekit-lsp#1233 |
2 similar comments
|
swiftlang/sourcekit-lsp#1233 |
|
swiftlang/sourcekit-lsp#1233 |
xedin
added a commit
to xedin/swift-package-manager
that referenced
this pull request
May 8, 2024
… introduce a cycle in a build graph (swiftlang#7530) It should be possible for packages to depend on each other if such dependence doesn't introduce cycles in the build graph. - Introduced a new DFS method to walk over graphs that breaks cycles. - Replaces use of `findCycle` + `topologicalSort` with `DFS` while building manifest and package graphs. This allows cycles in dependencies to be modeled correctly. - Removes some of the redundant data transformations from modules graph. - Modifies `ResolvedPackage` to carry identities of its dependencies instead of resolved dependencies themselves. This helps to simplify logic in `createResolvedPackages`. - Adds detection of target cycles across packages. Makes it possible for package A to depend on package B and B to depend on A if their targets don't form a cycle. (cherry picked from commit 8b12909)
xedin
added a commit
to xedin/swift-package-manager
that referenced
this pull request
May 8, 2024
… introduce a cycle in a build graph (swiftlang#7530) It should be possible for packages to depend on each other if such dependence doesn't introduce cycles in the build graph. - Introduced a new DFS method to walk over graphs that breaks cycles. - Replaces use of `findCycle` + `topologicalSort` with `DFS` while building manifest and package graphs. This allows cycles in dependencies to be modeled correctly. - Removes some of the redundant data transformations from modules graph. - Modifies `ResolvedPackage` to carry identities of its dependencies instead of resolved dependencies themselves. This helps to simplify logic in `createResolvedPackages`. - Adds detection of target cycles across packages. Makes it possible for package A to depend on package B and B to depend on A if their targets don't form a cycle. (cherry picked from commit 8b12909)
xedin
added a commit
to xedin/sourcekit-lsp
that referenced
this pull request
May 8, 2024
`ModulesGraph.init` was changed by swiftlang/swift-package-manager#7530 to accept `packages` as a way to avoid having to recompute the full list of packages by walking roots. (cherry picked from commit 2270631)
furby-tm
pushed a commit
to wabiverse/swift-package-manager
that referenced
this pull request
May 15, 2024
… introduce a cycle in a build graph (swiftlang#7530) ### Motivation: It should be possible for packages to depend on each other if such dependence doesn't introduce cycles in the build graph. ### Modifications: - Introduced a new DFS method to walk over graphs that breaks cycles. - Replaces use of `findCycle` + `topologicalSort` with `DFS` while building manifest and package graphs. This allows cycles in dependencies to be modeled correctly. - Removes some of the redundant data transformations from modules graph. - Modifies `ResolvedPackage` to carry identities of its dependencies instead of resolved dependencies themselves. This helps to simplify logic in `createResolvedPackages`. - Adds detection of target cycles across packages. ### Result: Makes it possible for package A to depend on package B and B to depend on A if their targets don't form a cycle.
furby-tm
pushed a commit
to wabiverse/swift-package-manager
that referenced
this pull request
May 15, 2024
… introduce a cycle in a build graph (swiftlang#7530) ### Motivation: It should be possible for packages to depend on each other if such dependence doesn't introduce cycles in the build graph. ### Modifications: - Introduced a new DFS method to walk over graphs that breaks cycles. - Replaces use of `findCycle` + `topologicalSort` with `DFS` while building manifest and package graphs. This allows cycles in dependencies to be modeled correctly. - Removes some of the redundant data transformations from modules graph. - Modifies `ResolvedPackage` to carry identities of its dependencies instead of resolved dependencies themselves. This helps to simplify logic in `createResolvedPackages`. - Adds detection of target cycles across packages. ### Result: Makes it possible for package A to depend on package B and B to depend on A if their targets don't form a cycle.
xedin
added a commit
to xedin/swift-package-manager
that referenced
this pull request
May 18, 2024
…manifests Follow-up to swiftlang#7530 Otherwise it might be suprising for package authors to discover that their packages cannot be used with older tools because they inadvertently introduced a cyclic dependency in a new version.
xedin
added a commit
to xedin/swift-package-manager
that referenced
this pull request
May 20, 2024
…manifests Follow-up to swiftlang#7530 Otherwise it might be suprising for package authors to discover that their packages cannot be used with older tools because they inadvertently introduced a cyclic dependency in a new version.
xedin
added a commit
that referenced
this pull request
May 22, 2024
#7579) …manifests ### Motivation: Follow-up to #7530 Otherwise it might be suprising for package authors to discover that their packages cannot be used with older tools because they inadvertently introduced a cyclic dependency in a new version. ### Modifications: `ModulesGraph.load` has been adjusted to run `findCycle` some of the root manifests support tools version that is older than 6.0. New diagnostic was added to help guide package authors to update their versions if they have cyclic package-level dependencies. ### Result: Attempts to introduce cyclic package-level dependencies to manifests that support tools-versions less than 6.0 would result in a failure.
xedin
added a commit
to xedin/swift-package-manager
that referenced
this pull request
May 29, 2024
… introduce a cycle in a build graph (swiftlang#7530) It should be possible for packages to depend on each other if such dependence doesn't introduce cycles in the build graph. - Introduced a new DFS method to walk over graphs that breaks cycles. - Replaces use of `findCycle` + `topologicalSort` with `DFS` while building manifest and package graphs. This allows cycles in dependencies to be modeled correctly. - Removes some of the redundant data transformations from modules graph. - Modifies `ResolvedPackage` to carry identities of its dependencies instead of resolved dependencies themselves. This helps to simplify logic in `createResolvedPackages`. - Adds detection of target cycles across packages. Makes it possible for package A to depend on package B and B to depend on A if their targets don't form a cycle. (cherry picked from commit 8b12909)
xedin
added a commit
to xedin/sourcekit-lsp
that referenced
this pull request
May 29, 2024
`ModulesGraph.init` was changed by swiftlang/swift-package-manager#7530 to accept `packages` as a way to avoid having to recompute the full list of packages by walking roots. (cherry picked from commit 2270631) (cherry picked from commit b13244f)
xedin
added a commit
that referenced
this pull request
May 30, 2024
…don't introduce a cycle in a build graph (#7541) - Explanation: It should be possible for packages to depend on each other if such dependence doesn't introduce cycles in the build graph. - Introduced a new DFS method to walk over graphs that breaks cycles. - Replaces use of `findCycle` + `topologicalSort` with `DFS` while building manifest and package graphs. This allows cycles in dependencies to be modeled correctly. - Removes some of the redundant data transformations from modules graph. - Modifies `ResolvedPackage` to carry identities of its dependencies instead of resolved dependencies themselves. This helps to simplify logic in `createResolvedPackages`. - Adds detection of target cycles across packages. Makes it possible for package A to depend on package B and B to depend on A if their targets don't form a cycle. - Scope: Package graph - Main Branch PRs: #7530 - Risk: Low - Reviewed By: @MaxDesiatov - Testing: Added new test-cases to the test suite
xedin
added a commit
to xedin/swift-package-manager
that referenced
this pull request
May 31, 2024
…manifests Follow-up to swiftlang#7530 Otherwise it might be suprising for package authors to discover that their packages cannot be used with older tools because they inadvertently introduced a cyclic dependency in a new version. (cherry picked from commit 3098b2d)
xedin
added a commit
that referenced
this pull request
Jun 1, 2024
… 6.0 … (#7617) …manifests - Explanation: Follow-up to #7530 Otherwise it might be surprising for package authors to discover that their packages cannot be used with older tools because they inadvertently introduced a cyclic dependency in a new version. - Scope: dependency resolution. - Main Branch PR: #7579 - Resolves: rdar://126217172 - Risk: Low - Reviewed By: @bnbarham - Testing: New tests added to the test suite. (cherry picked from commit 3098b2d)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Motivation:
It should be possible for packages to depend on each other if such dependence doesn't introduce cycles in the build graph.
Modifications:
findCycle+topologicalSortwithDFSwhile building manifest and package graphs. This allows cycles in dependencies to be modeled correctly.ResolvedPackageto carry identities of its dependencies instead of resolved dependencies themselves. This helps to simplify logic increateResolvedPackages.Result:
Makes it possible for package A to depend on package B and B to depend on A if their targets don't form a cycle.