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
Verify that only targets that are manifest dependencies can be imported. #3562
Changes from all commits
29509a7
ef4b0c9
08725a9
01eb92d
a4114eb
e5020d8
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 |
---|---|---|
@@ -0,0 +1,20 @@ | ||
// swift-tools-version:5.1 | ||
import PackageDescription | ||
|
||
let package = Package( | ||
name: "VerificationTestPackage", | ||
products: [ | ||
.executable(name: "BExec", targets: ["B"]), | ||
], | ||
dependencies: [ | ||
|
||
], | ||
targets: [ | ||
.target( | ||
name: "A", | ||
dependencies: []), | ||
.target( | ||
name: "B", | ||
dependencies: []), | ||
] | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
import B | ||
public func bar(x: Int) -> Int { | ||
return 11 + foo(x: x) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
public func foo(x: Int) -> Int { | ||
return 11 + x | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
print(baz(x: 11)) | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,6 +25,8 @@ import class TSCUtility.MultiLineNinjaProgressAnimation | |
import class TSCUtility.NinjaProgressAnimation | ||
import protocol TSCUtility.ProgressAnimationProtocol | ||
|
||
@_implementationOnly import SwiftDriver | ||
|
||
public final class BuildOperation: PackageStructureDelegate, SPMBuildCore.BuildSystem, BuildErrorAdviceProvider { | ||
|
||
/// The delegate used by the build system. | ||
|
@@ -156,12 +158,80 @@ public final class BuildOperation: PackageStructureDelegate, SPMBuildCore.BuildS | |
buildSystem?.cancel() | ||
} | ||
|
||
// Emit a warning if a target imports another target in this build | ||
// without specifying it as a dependency in the manifest | ||
private func verifyTargetImports(in description: BuildDescription) throws { | ||
let checkingMode = description.explicitTargetDependencyImportCheckingMode | ||
guard checkingMode != .none else { | ||
return | ||
} | ||
// Ensure the compiler supports the import-scan operation | ||
guard SwiftTargetBuildDescription.checkSupportedFrontendFlags(flags: ["import-prescan"], fileSystem: localFileSystem) else { | ||
return | ||
} | ||
|
||
for (target, commandLine) in description.swiftTargetScanArgs { | ||
do { | ||
guard let dependencies = description.targetDependencyMap[target] else { | ||
// Skip target if no dependency information is present | ||
continue | ||
} | ||
let targetDependenciesSet = Set(dependencies) | ||
guard !description.generatedSourceTargetSet.contains(target), | ||
targetDependenciesSet.intersection(description.generatedSourceTargetSet).isEmpty else { | ||
// Skip targets which contain, or depend-on-targets, with generated source-code. | ||
// Such as test discovery targets and targets with plugins. | ||
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. For a future iteration, could we do this verification in a later step so that any prebuild commands from plug-ins will already have been run (to generate the source files)? For the in-build commands, we do have their declared input and output paths, so perhaps we could find a way to use them. Not for this PR, but we should track improving this, since I think there are possible solutions. |
||
continue | ||
} | ||
let resolver = try ArgsResolver(fileSystem: localFileSystem) | ||
let executor = SPMSwiftDriverExecutor(resolver: resolver, | ||
fileSystem: localFileSystem, | ||
env: ProcessEnv.vars) | ||
|
||
let consumeDiagnostics: DiagnosticsEngine = DiagnosticsEngine(handlers: []) | ||
var driver = try Driver(args: commandLine, | ||
diagnosticsEngine: consumeDiagnostics, | ||
fileSystem: localFileSystem, | ||
executor: executor) | ||
guard !consumeDiagnostics.hasErrors else { | ||
// If we could not init the driver with this command, something went wrong, | ||
// proceed without checking this target. | ||
continue | ||
} | ||
let imports = try driver.performImportPrescan().imports | ||
let nonDependencyTargetsSet = | ||
Set(description.targetDependencyMap.keys.filter { !targetDependenciesSet.contains($0) }) | ||
let importedTargetsMissingDependency = Set(imports).intersection(nonDependencyTargetsSet) | ||
if let missedDependency = importedTargetsMissingDependency.first { | ||
switch checkingMode { | ||
case .error: | ||
self.observabilityScope.emit(error: "Target \(target) imports another target (\(missedDependency)) in the package without declaring it a dependency.") | ||
case .warn: | ||
self.observabilityScope.emit(warning: "Target \(target) imports another target (\(missedDependency)) in the package without declaring it a dependency.") | ||
case .none: | ||
fatalError("Explicit import checking is disabled.") | ||
} | ||
} | ||
} catch { | ||
// The above verification is a best-effort attempt to warn the user about a potential manifest | ||
// error. If something went wrong during the import-prescan, proceed silently. | ||
return | ||
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. is there a need to record the issue somewhere / somehow? |
||
} | ||
} | ||
} | ||
|
||
/// Perform a build using the given build description and subset. | ||
public func build(subset: BuildSubset) throws { | ||
|
||
let buildStartTime = DispatchTime.now() | ||
|
||
// Get the build description (either a cached one or newly created). | ||
let buildDescription = try self.getBuildDescription() | ||
|
||
// Get the build description | ||
let buildDescription = try getBuildDescription() | ||
|
||
// Verify dependency imports on the described targers | ||
try verifyTargetImports(in: buildDescription) | ||
|
||
// Create the build system. | ||
let buildSystem = try self.createBuildSystem(buildDescription: buildDescription) | ||
|
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.
@artemcm @neonichu this test started failing recently, due to
but only on macOS. I'm not sure how it was suposed to work in the first place and how it worked before and still works on other platforms, since
baz(x:)
is not defined anywhere?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.
CI failure log for now available at https://ci.swift.org/job/swift-package-manager-with-xcode-self-hosted-PR-osx/2994/console
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.
I think the error is expected, right? The test is using
XCTAssertThrowsError
in all cases.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.
I think the problem is that
Target A imports another target (B) in the package without declaring it a dependency.
isn't present in the output at all.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.
My assumption would be that
<unknown>:0: error: unknown argument: '-dwarf-version=4
interferes with the import detection, e.g. we may not get a list of imports at all and from that follows the test failure.AFAIK, we haven't updated the installed Swift version, so it's a bit of a mystery how this broke?
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.
Actually maybe this will be the fix? apple/swift-driver#1502