Skip to content

[SE-0226]: Remove test target dependencies from dependency resolution #2424

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

Merged
merged 1 commit into from
Dec 13, 2019

Conversation

hartbit
Copy link
Contributor

@hartbit hartbit commented Nov 26, 2019

This is a fairly large Pull Request so here's a recap of the changes:

  • Introduce a allRequiredTargets cached property on Manifest that calculates which targets are required to build the manifest's products. All other targets (automatically including test targets) are only used for that package's development, will never be importable from other packages and therefore don't participate in dependency resolution and are not required to build the package graph.
  • Introduce a allRequiredDependencies cached property which returns the package dependencies required to build all manifest's products. All other dependencies are only necessary for development, will never be importable from other packages and therefore don't participate in dependency resolution and are not required to build the package graph.
  • Introduce a isRoot property on PackageReference and Manifest to keep track of which packages/manifests are root, as this is necessary to calculate the previous two properties: indeed, all targets and all dependencies are visible from root packages.
  • Use allRequiredDependencies during dependency resolution to avoid cloning dependencies which are not needed.
  • Use allRequiredTargets and allRequiredDependencies during package graph building to omit non-required targets and dependencies from the graph. If not, the graph will fail for dependencies which were not cloned during dependency resolution.
  • Write unit tests in ManifestTests to verify that allRequiredTargets and allRequiredDependencies return the expected targets and dependencies.
  • Write "integration tests" in WorkspaceTests with a complex graph to make sure that dependency resolution and graph building works around different complicated cases.

This PR depends on #2423 being merged first.

@hartbit hartbit requested a review from aciidgh as a code owner November 26, 2019 16:23
@hartbit hartbit changed the title A first iteration on SE-0226 WIP: A first iteration on SE-0226 Nov 26, 2019
@hartbit
Copy link
Contributor Author

hartbit commented Nov 26, 2019

Question: Should we treat PackageGraphRootInput.dependencies like other dependencies, in that we only resolve target and dependencies which are reachable from products, or should they behave as root packages, in that we need all targets and dependencies from them?

The reason I ask that is that this PR makes the testCanResolveWithIncompatiblePins test fail because its setup with no root packages, and only a single root dependency. I can refactor it to have a root package with a single dependency, but I want to make sure I'm not hiding a regression.

@hartbit hartbit force-pushed the se0226-test-targets branch 5 times, most recently from 49ccb32 to 61c9db6 Compare November 27, 2019 08:01
@hartbit hartbit force-pushed the se0226-test-targets branch from 61c9db6 to 8f1f330 Compare December 6, 2019 09:35
@aciidgh
Copy link
Contributor

aciidgh commented Dec 9, 2019

Looks great in general but I'll do a deeper review later.

@hartbit hartbit force-pushed the se0226-test-targets branch 2 times, most recently from 9c1bc87 to 43bcf96 Compare December 10, 2019 10:08
@hartbit hartbit changed the title WIP: A first iteration on SE-0226 Implement part of SE-0226 Dec 10, 2019
@hartbit hartbit force-pushed the se0226-test-targets branch 5 times, most recently from cc01a15 to 818cb73 Compare December 10, 2019 22:21
@hartbit
Copy link
Contributor Author

hartbit commented Dec 10, 2019

@aciidb0mb3r I applied review comments, the largest of them being to move to a enum on PackageReference and Manifest to store the kind of package instead of two isLocal and isRoot properties. But it was worth it.

@hartbit hartbit changed the title Implement part of SE-0226 [SE-0226]: Remove test target dependencies from dependency resolution Dec 11, 2019
@hartbit
Copy link
Contributor Author

hartbit commented Dec 12, 2019

I made improvements to this PR by using the fact that we now always have a package name (defaulting to a non-lowercased identity) to use that instead of identity in all user-facing messages.

@hartbit hartbit force-pushed the se0226-test-targets branch 2 times, most recently from 37da39e to ec75953 Compare December 12, 2019 09:51
Copy link
Contributor

@aciidgh aciidgh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks!!

@hartbit hartbit force-pushed the se0226-test-targets branch 3 times, most recently from 5be8b5e to 8e8f285 Compare December 13, 2019 09:57
@hartbit
Copy link
Contributor Author

hartbit commented Dec 13, 2019

@swift-ci please smoke test

@hartbit hartbit force-pushed the se0226-test-targets branch from 8e8f285 to 9b58830 Compare December 13, 2019 11:27
@hartbit
Copy link
Contributor Author

hartbit commented Dec 13, 2019

@swift-ci please smoke test

@hartbit hartbit merged commit 2278a8e into swiftlang:master Dec 13, 2019
@hartbit hartbit deleted the se0226-test-targets branch December 17, 2019 22:28
neonichu added a commit to neonichu/swift-package-manager that referenced this pull request Jan 6, 2021
This brings back the 5.3 implementation of `dependenciesRequired(for:)` (slightly modified) and also makes it so that `targetsRequired(for:)` for non-root packages only includes targets required by any products. Together this brings the behaviour of dependency resolution to the state of swiftlang#2424.
neonichu added a commit to neonichu/swift-package-manager that referenced this pull request Jan 6, 2021
This brings back the 5.3 implementation of `dependenciesRequired(for:)` (slightly modified) and also makes it so that `targetsRequired(for:)` for non-root packages only includes targets required by any products. Together this brings the behaviour of dependency resolution to the state of swiftlang#2424.
neonichu added a commit that referenced this pull request Jan 7, 2021
This brings back the 5.3 implementation of `dependenciesRequired(for:)` (slightly modified) and also makes it so that `targetsRequired(for:)` for non-root packages only includes targets required by any products. Together this brings the behaviour of dependency resolution to the state of #2424.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants