Prevent non-targets from depending on test targets #8513
Conversation
johnbute
requested review from
bnbarham,
MaxDesiatov,
jakepetroules,
xedin,
dschaefer2,
shawnhyam,
bripeticca,
plemarquand,
owenv,
bkhouri,
cmcgee1024 and
daveyc123
as code owners
| for dependency in dependencies { | ||
| if let depTarget = dependency.module, depTarget.type == .test { | ||
| self.observabilityScope.emit(.invalidDependencyOnTestTarget( | ||
| dependency: dependency.name, |
There was a problem hiding this comment.
suggestion (non-blocking): consider passing the entire structure to the dependency and targetName argument to allow the invalidDependencyOnTestTarget to have additional control on possibly using "other" information.
There was a problem hiding this comment.
Done partially, I cannot pass the entire target structure since the target is of type PotentialModule and not Module
johnbute
force-pushed
the
non-target-dependency-fix
branch
from
067c46c to
c21461c
Compare
| @@ -309,6 +309,12 @@ extension Basics.Diagnostic { | |||
| return .error("\(messagePrefix) in dependencies of target '\(targetName)'; valid packages are: \(validPackages.map{ "\($0.descriptionForValidation)" }.joined(separator: ", "))") | |||
| } | |||
|
|
|||
| static func invalidDependencyOnTestTarget(dependency: Module.Dependency, targetName: String) -> Self { | |||
There was a problem hiding this comment.
question: How best to manage packages out there that might be doing this already? This might need to be an opt-in feature either by the swift-tools comment at the top of the package so that packages can adopt the stricter checking.
There was a problem hiding this comment.
Currently, if you do this, you get the following error and your package does not build:
❯ swift build --package-path Fixtures/Miscellaneous/DependencyTargetDependsOnTestTarget
[1/1] Planning build
Building for debugging...
error: emit-module command failed with exit code 1 (use -v to see invocation)
/Users/bkhouri/Documents/git/public/swiftlang/swift-package-manager/Fixtures/Miscellaneous/DependencyTargetDependsOnTestTarget/Sources/DependencyTargetDependsOnTestTarget/DependencyTargetDependsOnTestTarget.swift:4:8: error: compiling for macOS 10.13, but module 'DependencyTargetDependsOnTestTargetTests' has a minimum deployment target of macOS 14.0: /Users/bkhouri/Documents/git/public/swiftlang/swift-package-manager/Fixtures/Miscellaneous/DependencyTargetDependsOnTestTarget/.build/arm64-apple-macosx/debug/Modules/DependencyTargetDependsOnTestTargetTests.swiftmodule
2 | // https://docs.swift.org/swift-book
3 |
4 | import DependencyTargetDependsOnTestTargetTests
| `- error: compiling for macOS 10.13, but module 'DependencyTargetDependsOnTestTargetTests' has a minimum deployment target of macOS 14.0: /Users/bkhouri/Documents/git/public/swiftlang/swift-package-manager/Fixtures/Miscellaneous/DependencyTargetDependsOnTestTarget/.build/arm64-apple-macosx/debug/Modules/DependencyTargetDependsOnTestTargetTests.swiftmodule
5 |
/Users/bkhouri/Documents/git/public/swiftlang/swift-package-manager/Fixtures/Miscellaneous/DependencyTargetDependsOnTestTarget/Sources/DependencyTargetDependsOnTestTarget/DependencyTargetDependsOnTestTarget.swift:4:8: error: compiling for macOS 10.13, but module 'DependencyTargetDependsOnTestTargetTests' has a minimum deployment target of macOS 14.0: /Users/bkhouri/Documents/git/public/swiftlang/swift-package-manager/Fixtures/Miscellaneous/DependencyTargetDependsOnTestTarget/.build/arm64-apple-macosx/debug/Modules/DependencyTargetDependsOnTestTargetTests.swiftmodule
2 | // https://docs.swift.org/swift-book
3 |
4 | import DependencyTargetDependsOnTestTargetTests
| `- error: compiling for macOS 10.13, but module 'DependencyTargetDependsOnTestTargetTests' has a minimum deployment target of macOS 14.0: /Users/bkhouri/Documents/git/public/swiftlang/swift-package-manager/Fixtures/Miscellaneous/DependencyTargetDependsOnTestTarget/.build/arm64-apple-macosx/debug/Modules/DependencyTargetDependsOnTestTargetTests.swiftmodule
5 |
There was a problem hiding this comment.
If packages are already doing this, they should be updated as it does not make sense, to me, having a non-test target depend on a test target.
Maybe we should add something to the "change log" as a possibly breaking change.
|
@swift-ci please test |
|
@swift-ci test macOS |
|
@swift-ci test windows |
|
@swift-ci test windows |
|
@swift-ci Please smoke test macOS platform |
|
@swift-ci please test macos |
Prevent non-targets from depending on test targets
Motivation:
Fix for rdar://149007214
Currently, only test targets are allowed to have dependencies on other test targets.
Modifications:
Simply checked for each non-test target, if their dependency is of type test. If it is, throw an error.
Result:
Swift package manager will give an error explaining that only testTargets can depend on other testTargets
Fixes #8478