From 99d5f9e234eb09c47e1af60c2b778db70326328c Mon Sep 17 00:00:00 2001 From: Ankit Aggarwal Date: Tue, 1 Aug 2017 02:32:03 +0530 Subject: [PATCH] Revert "Allow build & run to reference unreachable targets" --- .../UnreachableTargets/A/Package.swift | 17 ------ .../A/Sources/ATarget/main.swift | 3 - .../UnreachableTargets/B/Package.swift | 13 ---- .../B/Sources/BTarget1/BTarget1.swift | 3 - .../B/Sources/BTarget2/main.swift | 1 - .../UnreachableTargets/C/Package.swift | 11 ---- .../C/Sources/CTarget/main.swift | 1 - Sources/Build/BuildPlan.swift | 4 +- Sources/Build/llbuild.swift | 41 +++++-------- Sources/Commands/SwiftRunTool.swift | 27 ++++----- Sources/Commands/SwiftTool.swift | 4 +- Sources/PackageGraph/PackageGraph.swift | 23 +++----- Tests/BuildTests/BuildPlanTests.swift | 59 ------------------- Tests/CommandsTests/BuildToolTests.swift | 58 +----------------- Tests/CommandsTests/RunToolTests.swift | 16 +---- 15 files changed, 43 insertions(+), 238 deletions(-) delete mode 100644 Fixtures/Miscellaneous/UnreachableTargets/A/Package.swift delete mode 100644 Fixtures/Miscellaneous/UnreachableTargets/A/Sources/ATarget/main.swift delete mode 100644 Fixtures/Miscellaneous/UnreachableTargets/B/Package.swift delete mode 100644 Fixtures/Miscellaneous/UnreachableTargets/B/Sources/BTarget1/BTarget1.swift delete mode 100644 Fixtures/Miscellaneous/UnreachableTargets/B/Sources/BTarget2/main.swift delete mode 100644 Fixtures/Miscellaneous/UnreachableTargets/C/Package.swift delete mode 100644 Fixtures/Miscellaneous/UnreachableTargets/C/Sources/CTarget/main.swift diff --git a/Fixtures/Miscellaneous/UnreachableTargets/A/Package.swift b/Fixtures/Miscellaneous/UnreachableTargets/A/Package.swift deleted file mode 100644 index 7c05636fa8a..00000000000 --- a/Fixtures/Miscellaneous/UnreachableTargets/A/Package.swift +++ /dev/null @@ -1,17 +0,0 @@ -// swift-tools-version:4.0 -import PackageDescription - -let package = Package( - name: "A", - products: [ - .executable(name: "aexec", targets: ["ATarget"]) - ], - dependencies: [ - .package(url: "../B", from: "1.0.0"), - .package(url: "../C", from: "1.0.0") - ], - targets: [ - .target(name: "ATarget", dependencies: [ - .product(name: "BLibrary") - ]) - ]) diff --git a/Fixtures/Miscellaneous/UnreachableTargets/A/Sources/ATarget/main.swift b/Fixtures/Miscellaneous/UnreachableTargets/A/Sources/ATarget/main.swift deleted file mode 100644 index 6abff7a1c58..00000000000 --- a/Fixtures/Miscellaneous/UnreachableTargets/A/Sources/ATarget/main.swift +++ /dev/null @@ -1,3 +0,0 @@ -import BTarget1 - -BTarget1() diff --git a/Fixtures/Miscellaneous/UnreachableTargets/B/Package.swift b/Fixtures/Miscellaneous/UnreachableTargets/B/Package.swift deleted file mode 100644 index dc2553656b8..00000000000 --- a/Fixtures/Miscellaneous/UnreachableTargets/B/Package.swift +++ /dev/null @@ -1,13 +0,0 @@ -// swift-tools-version:4.0 -import PackageDescription - -let package = Package( - name: "B", - products: [ - .library(name: "BLibrary", targets: ["BTarget1"]), - .executable(name: "bexec", targets: ["BTarget2"]) - ], - targets: [ - .target(name: "BTarget1", dependencies: []), - .target(name: "BTarget2", dependencies: []) - ]) diff --git a/Fixtures/Miscellaneous/UnreachableTargets/B/Sources/BTarget1/BTarget1.swift b/Fixtures/Miscellaneous/UnreachableTargets/B/Sources/BTarget1/BTarget1.swift deleted file mode 100644 index 9058324cce7..00000000000 --- a/Fixtures/Miscellaneous/UnreachableTargets/B/Sources/BTarget1/BTarget1.swift +++ /dev/null @@ -1,3 +0,0 @@ -public struct BTarget1 { - public init() {} -} diff --git a/Fixtures/Miscellaneous/UnreachableTargets/B/Sources/BTarget2/main.swift b/Fixtures/Miscellaneous/UnreachableTargets/B/Sources/BTarget2/main.swift deleted file mode 100644 index 8b87e07b824..00000000000 --- a/Fixtures/Miscellaneous/UnreachableTargets/B/Sources/BTarget2/main.swift +++ /dev/null @@ -1 +0,0 @@ -print("BTarget2") diff --git a/Fixtures/Miscellaneous/UnreachableTargets/C/Package.swift b/Fixtures/Miscellaneous/UnreachableTargets/C/Package.swift deleted file mode 100644 index 2c17ea77dde..00000000000 --- a/Fixtures/Miscellaneous/UnreachableTargets/C/Package.swift +++ /dev/null @@ -1,11 +0,0 @@ -// swift-tools-version:4.0 -import PackageDescription - -let package = Package( - name: "C", - products: [ - .executable(name: "cexec", targets: ["CTarget"]) - ], - targets: [ - .target(name: "CTarget", dependencies: []) - ]) diff --git a/Fixtures/Miscellaneous/UnreachableTargets/C/Sources/CTarget/main.swift b/Fixtures/Miscellaneous/UnreachableTargets/C/Sources/CTarget/main.swift deleted file mode 100644 index 333d235c2b7..00000000000 --- a/Fixtures/Miscellaneous/UnreachableTargets/C/Sources/CTarget/main.swift +++ /dev/null @@ -1 +0,0 @@ -print("CTarget") diff --git a/Sources/Build/BuildPlan.swift b/Sources/Build/BuildPlan.swift index d94d34b38d8..7ea260ff02d 100644 --- a/Sources/Build/BuildPlan.swift +++ b/Sources/Build/BuildPlan.swift @@ -474,7 +474,7 @@ public class BuildPlan { // Create build target description for each target which we need to plan. var targetMap = [ResolvedTarget: TargetDescription]() - for target in graph.allTargets { + for target in graph.targets { switch target.underlyingTarget { case is SwiftTarget: targetMap[target] = .swift(SwiftTargetDescription(target: target, buildParameters: buildParameters)) @@ -511,7 +511,7 @@ public class BuildPlan { var productMap: [ResolvedProduct: ProductBuildDescription] = [:] // Create product description for each product we have in the package graph except // for automatic libraries because they don't produce any output. - for product in graph.allProducts where product.type != .library(.automatic) { + for product in graph.products where product.type != .library(.automatic) { productMap[product] = ProductBuildDescription( product: product, buildParameters: buildParameters) } diff --git a/Sources/Build/llbuild.swift b/Sources/Build/llbuild.swift index 310ce9a8ad0..a036d63f3e2 100644 --- a/Sources/Build/llbuild.swift +++ b/Sources/Build/llbuild.swift @@ -51,7 +51,7 @@ public struct LLBuildManifestGenerator { private var otherTargets: [Target] = [] /// Append a command. - mutating func append(_ target: Target, buildByDefault: Bool, isTest: Bool) { + mutating func append(_ target: Target, isTest: Bool) { // Create a phony command with a virtual output node that represents the target. let virtualNodeName = "<\(target.name)>" let phonyTool = PhonyTool(inputs: target.outputs, outputs: [virtualNodeName]) @@ -63,17 +63,14 @@ public struct LLBuildManifestGenerator { newTarget.cmds.insert(phonyCommand) otherTargets.append(newTarget) - if buildByDefault { - if !isTest { - main.outputs += newTarget.outputs - main.cmds += newTarget.cmds - } - - // Always build everything for the test target. - test.outputs += newTarget.outputs - test.cmds += newTarget.cmds + if !isTest { + main.outputs += newTarget.outputs + main.cmds += newTarget.cmds } + // Always build everything for the test target. + test.outputs += newTarget.outputs + test.cmds += newTarget.cmds allCommands += newTarget.cmds } } @@ -83,26 +80,18 @@ public struct LLBuildManifestGenerator { var targets = Targets() // Create commands for all target description in the plan. - for (target, description) in plan.targetMap { - switch description { - case .swift(let description): - // Only build targets by default if they are reachabe from a root target. - targets.append(createSwiftCompileTarget(description), - buildByDefault: plan.graph.targets.contains(target), - isTest: description.isTestTarget) - case .clang(let description): - targets.append(createClangCompileTarget(description), - buildByDefault: plan.graph.targets.contains(target), - isTest: description.isTestTarget) + for buildTarget in plan.targets { + switch buildTarget { + case .swift(let target): + targets.append(createSwiftCompileTarget(target), isTest: target.isTestTarget) + case .clang(let target): + targets.append(createClangCompileTarget(target), isTest: target.isTestTarget) } } // Create command for all products in the plan. - for (product, description) in plan.productMap { - // Only build products by default if they are reachabe from a root target. - targets.append(createProductTarget(description), - buildByDefault: plan.graph.products.contains(product), - isTest: product.type == .test) + for buildProduct in plan.buildProducts { + targets.append(createProductTarget(buildProduct), isTest: buildProduct.product.type == .test) } // Write the manifest. diff --git a/Sources/Commands/SwiftRunTool.swift b/Sources/Commands/SwiftRunTool.swift index 7e2623311d0..a2a23a3c541 100644 --- a/Sources/Commands/SwiftRunTool.swift +++ b/Sources/Commands/SwiftRunTool.swift @@ -126,30 +126,27 @@ public class SwiftRunTool: SwiftTool { /// Returns the path to the correct executable based on options. private func findProduct(in plan: BuildPlan) throws -> ResolvedProduct { + let executableProducts = plan.graph.products.filter({ $0.type == .executable }) + + // Error out if the product contains no executable. + guard executableProducts.count > 0 else { + throw RunError.noExecutableFound + } + if let executable = options.executable { - // If the exectuable is explicitly specified, search through all products. - guard let executableProduct = plan.graph.allProducts.first(where: { - $0.type == .executable && $0.name == executable - }) else { + // If the exectuable is explicitly specified, verify that it exists. + guard let executableProduct = executableProducts.first(where: { $0.name == executable }) else { throw RunError.executableNotFound(executable) } return executableProduct } else { - // If the executably is implicit, search through root products. - let rootExecutables = plan.graph.rootPackages.flatMap({ $0.products }).filter({ $0.type == .executable }) - - // Error out if the package contains no executables. - guard rootExecutables.count > 0 else { - throw RunError.noExecutableFound - } - // Only implicitly deduce the executable if it is the only one. - guard rootExecutables.count == 1 else { - throw RunError.multipleExecutables(rootExecutables.map({ $0.name })) + guard executableProducts.count == 1 else { + throw RunError.multipleExecutables(executableProducts.map({ $0.name })) } - return rootExecutables[0] + return executableProducts[0] } } diff --git a/Sources/Commands/SwiftTool.swift b/Sources/Commands/SwiftTool.swift index e27d9e92bcf..e6f08548279 100644 --- a/Sources/Commands/SwiftTool.swift +++ b/Sources/Commands/SwiftTool.swift @@ -568,13 +568,13 @@ extension BuildSubset { case .allIncludingTests: return LLBuildManifestGenerator.llbuildTestTargetName case .product(let productName): - guard let product = graph.allProducts.first(where: { $0.name == productName }) else { + guard let product = graph.products.first(where: { $0.name == productName }) else { diagnostics.emit(data: ProductNotFoundDiagnostic(productName: productName)) return nil } return product.llbuildTargetName case .target(let targetName): - guard let target = graph.allTargets.first(where: { $0.name == targetName }) else { + guard let target = graph.targets.first(where: { $0.name == targetName }) else { diagnostics.emit(data: TargetNotFoundDiagnostic(targetName: targetName)) return nil } diff --git a/Sources/PackageGraph/PackageGraph.swift b/Sources/PackageGraph/PackageGraph.swift index 1b7cbb6fbca..12528f4a22c 100644 --- a/Sources/PackageGraph/PackageGraph.swift +++ b/Sources/PackageGraph/PackageGraph.swift @@ -21,17 +21,11 @@ public struct PackageGraph { public let packages: [ResolvedPackage] /// Returns list of all targets (reachable from root targets) in the graph. - public let targets: Set + public let targets: [ResolvedTarget] /// Contains all the products of the root packages and the product dependencies of the root targets. /// i.e. this array will not contain the products which are not needed to be built. - public let products: Set - - /// Returns all the targets in the graph, regardless if they are reachable from the root targets or not. - public let allTargets: Set - - /// Returns all the products in the graph, regardless if they are reachable from the root targets or not. - public var allProducts: Set + public let products: [ResolvedProduct] /// Returns true if a given target is present in root packages. public func isInRootPackages(_ target: ResolvedTarget) -> Bool { @@ -44,8 +38,6 @@ public struct PackageGraph { self.rootPackages = rootPackages let inputPackages = rootPackages + rootDependencies self.packages = try! topologicalSort(inputPackages, successors: { $0.dependencies }) - allTargets = Set(packages.flatMap({ $0.targets })) - allProducts = Set(packages.flatMap({ $0.products })) // Compute the input targets. let inputTargets = inputPackages.flatMap({ $0.targets }).map(ResolvedTarget.Dependency.target) @@ -53,15 +45,18 @@ public struct PackageGraph { let dependencies = try! topologicalSort(inputTargets, successors: { $0.dependencies }) // Separate out the products and targets but maintain their topological order. - var targets: Set = [] - var products = Set(inputPackages.flatMap({ $0.products })) + var targets: [ResolvedTarget] = [] + var products = inputPackages.flatMap({ $0.products }) + let rootDependencyProductSet = Set(rootDependencies.flatMap({ $0.products })) for dependency in dependencies { switch dependency { case .target(let target): - targets.insert(target) + targets.append(target) case .product(let product): - products.insert(product) + // Avoid re-adding any product from the root dependencies. + if rootDependencyProductSet.contains(product) { continue } + products.append(product) } } diff --git a/Tests/BuildTests/BuildPlanTests.swift b/Tests/BuildTests/BuildPlanTests.swift index bb5eb1470d8..4ba986a7fe8 100644 --- a/Tests/BuildTests/BuildPlanTests.swift +++ b/Tests/BuildTests/BuildPlanTests.swift @@ -458,64 +458,6 @@ final class BuildPlanTests: XCTestCase { } - func testNonReachableProductsAndTargets() throws { - let fileSystem = InMemoryFileSystem(emptyFiles: - "/A/Sources/ATarget/main.swift", - "/B/Sources/BTarget1/BTarget1.swift", - "/B/Sources/BTarget2/main.swift", - "/C/Sources/CTarget/main.swift" - ) - - let diagnostics = DiagnosticsEngine() - let graph = loadMockPackageGraph4([ - "/A": Package( - name: "A", - products: [ - .executable(name: "aexec", targets: ["ATarget"]) - ], - dependencies: [ - .package(url: "/B", from: "1.0.0"), - .package(url: "/C", from: "1.0.0") - ], - targets: [ - .target(name: "ATarget", dependencies: [ - .product(name: "BLibrary") - ]) - ]), - "/B": Package( - name: "B", - products: [ - .library(name: "BLibrary", type: .static, targets: ["BTarget1"]), - .executable(name: "bexec", targets: ["BTarget2"]) - ], - targets: [ - .target(name: "BTarget1", dependencies: []), - .target(name: "BTarget2", dependencies: []) - ]), - "/C": Package( - name: "C", - products: [ - .executable(name: "cexec", targets: ["CTarget"]) - ], - targets: [ - .target(name: "CTarget", dependencies: []) - ]) - ], root: "/A", diagnostics: diagnostics, in: fileSystem) - - XCTAssertEqual(Set(graph.products.map({ $0.name })), ["aexec", "BLibrary"]) - XCTAssertEqual(Set(graph.targets.map({ $0.name })), ["ATarget", "BTarget1"]) - XCTAssertEqual(Set(graph.allProducts.map({ $0.name })), ["aexec", "BLibrary", "bexec", "cexec"]) - XCTAssertEqual(Set(graph.allTargets.map({ $0.name })), ["ATarget", "BTarget1", "BTarget2", "CTarget"]) - - let result = BuildPlanResult(plan: try BuildPlan( - buildParameters: mockBuildParameters(), - graph: graph, - fileSystem: fileSystem)) - - XCTAssertEqual(Set(result.productMap.keys), ["aexec", "BLibrary", "bexec", "cexec"]) - XCTAssertEqual(Set(result.targetMap.keys), ["ATarget", "BTarget1", "BTarget2", "CTarget"]) - } - static var allTests = [ ("testBasicClangPackage", testBasicClangPackage), ("testBasicReleasePackage", testBasicReleasePackage), @@ -526,7 +468,6 @@ final class BuildPlanTests: XCTestCase { ("testDynamicProducts", testDynamicProducts), ("testSwiftCMixed", testSwiftCMixed), ("testTestModule", testTestModule), - ("testNonReachableProductsAndTargets", testNonReachableProductsAndTargets) ] } diff --git a/Tests/CommandsTests/BuildToolTests.swift b/Tests/CommandsTests/BuildToolTests.swift index 8858b310dfe..7c3cb488fca 100644 --- a/Tests/CommandsTests/BuildToolTests.swift +++ b/Tests/CommandsTests/BuildToolTests.swift @@ -111,67 +111,11 @@ final class BuildToolTests: XCTestCase { } } - func testNonReachableProductsAndTargetsFunctional() { - fixture(name: "Miscellaneous/UnreachableTargets") { path in - do { - let aPath = path.appending(component: "A") - let output = try execute([], packagePath: aPath) - XCTAssert(!output.contains("bexec")) - XCTAssert(!output.contains("BTarget2")) - XCTAssert(!output.contains("cexec")) - XCTAssert(!output.contains("CTarget")) - } catch SwiftPMProductError.executionFailure(_, _, let stderr) { - XCTFail(stderr) - } - - let buildPath = path.appending(component: ".build") - try localFileSystem.removeFileTree(buildPath) - - // Dependency contains a dependent product - - do { - let aPath = path.appending(component: "A") - let output = try execute(["--product", "bexec"], packagePath: aPath) - XCTAssert(output.contains("Compile") && output.contains("BTarget2")) - XCTAssert(output.contains("Linking") && output.contains("bexec")) - XCTAssert(!output.contains("aexec")) - XCTAssert(!output.contains("ATarget")) - XCTAssert(!output.contains("BLibrary")) - XCTAssert(!output.contains("BTarget1")) - XCTAssert(!output.contains("cexec")) - XCTAssert(!output.contains("CTarget")) - } catch SwiftPMProductError.executionFailure(_, _, let stderr) { - XCTFail(stderr) - } - - try localFileSystem.removeFileTree(buildPath) - - // Dependency does not contain a dependent product - - do { - let aPath = path.appending(component: "A") - let output = try execute(["--target", "CTarget"], packagePath: aPath) - XCTAssert(output.contains("Compile") && output.contains("CTarget")) - XCTAssert(!output.contains("Linking")) - XCTAssert(!output.contains("aexec")) - XCTAssert(!output.contains("ATarget")) - XCTAssert(!output.contains("BLibrary")) - XCTAssert(!output.contains("bexec")) - XCTAssert(!output.contains("BTarget1")) - XCTAssert(!output.contains("BTarget2")) - XCTAssert(!output.contains("cexec")) - } catch SwiftPMProductError.executionFailure(_, _, let stderr) { - XCTFail(stderr) - } - } - } - static var allTests = [ ("testUsage", testUsage), ("testVersion", testVersion), ("testBinPath", testBinPath), ("testBackwardsCompatibilitySymlink", testBackwardsCompatibilitySymlink), - ("testProductAndTarget", testProductAndTarget), - ("testNonReachableProductsAndTargetsFunctional", testNonReachableProductsAndTargetsFunctional) + ("testProductAndTarget", testProductAndTarget) ] } diff --git a/Tests/CommandsTests/RunToolTests.swift b/Tests/CommandsTests/RunToolTests.swift index ef11c64ed9f..90167cf324f 100644 --- a/Tests/CommandsTests/RunToolTests.swift +++ b/Tests/CommandsTests/RunToolTests.swift @@ -28,7 +28,7 @@ final class RunToolTests: XCTestCase { XCTAssert(try execute(["--version"]).contains("Swift Package Manager")) } - func testUnkownProductAndArgumentPassing() throws { + func testFunctional() throws { fixture(name: "Miscellaneous/EchoExecutable") { path in do { _ = try execute(["unknown"], packagePath: path) @@ -43,9 +43,7 @@ final class RunToolTests: XCTestCase { "\(getcwd())" "1" "--hello" "world" """) } - } - func testMultipleExecutableAndExplicitExecutable() throws { fixture(name: "Miscellaneous/MultipleExecutables") { path in do { _ = try execute([], packagePath: path) @@ -63,14 +61,6 @@ final class RunToolTests: XCTestCase { } } - func testUnreachableExecutable() throws { - fixture(name: "Miscellaneous/UnreachableTargets") { path in - let output = try execute(["bexec"], packagePath: path.appending(component: "A")) - let outputLines = output.split(separator: "\n") - XCTAssertEqual(outputLines.last!, "BTarget2") - } - } - func testFileDeprecation() throws { fixture(name: "Miscellaneous/EchoExecutable") { path in let filePath = AbsolutePath(path, "Sources/secho/main.swift").asString @@ -85,9 +75,7 @@ final class RunToolTests: XCTestCase { static var allTests = [ ("testUsage", testUsage), ("testVersion", testVersion), - ("testUnkownProductAndArgumentPassing", testUnkownProductAndArgumentPassing), - ("testMultipleExecutableAndExplicitExecutable", testMultipleExecutableAndExplicitExecutable), - ("testUnreachableExecutable", testUnreachableExecutable), + ("testFunctional", testFunctional), ("testFileDeprecation", testFileDeprecation) ] }