From 866fe2da30087b1a8524f2fa6e591277329b211e Mon Sep 17 00:00:00 2001 From: Paul LeMarquand Date: Tue, 25 Mar 2025 19:21:14 -0400 Subject: [PATCH 1/4] Move files when using `package add-target` on single target package Currently `swift package init --type executable` in a folder called `Example` creates a project with the following structure: ``` ./Example/Sources ./Example/Sources/main.swift ./Example/Package.swift ``` Following this up with a `swift package add-target Foo` produces a package that no longer builds. It now has the structure: ``` ./Example/Sources ./Example/Sources/main.swift ./Example/Sources/Foo/Foo.swift ./Example/Package.swift ``` Packages with multiple targets cannot have code directly in `./Sources`, and the user gets the error: `Source files for target Example should be located under 'Sources/Example'` To work around this in the AddTarget command we can check if a package is structured as a single target package with sources directly in `./Sources` and move these files into a new folder located at `./Sources/Example`. This allows the project to build after the new target has been added. Issue: #8410 --- .../Commands/PackageCommands/AddTarget.swift | 8 +++ Sources/PackageModelSyntax/AddTarget.swift | 56 +++++++++++++++++++ Tests/CommandsTests/PackageCommandTests.swift | 53 +++++++++++++++++- 3 files changed, 116 insertions(+), 1 deletion(-) diff --git a/Sources/Commands/PackageCommands/AddTarget.swift b/Sources/Commands/PackageCommands/AddTarget.swift index 01daf19f8a3..8d9bea9fe5a 100644 --- a/Sources/Commands/PackageCommands/AddTarget.swift +++ b/Sources/Commands/PackageCommands/AddTarget.swift @@ -89,6 +89,14 @@ extension SwiftPackageCommand { } } + // Move sources into their own folder if they're directly in `./Sources`. + try PackageModelSyntax.AddTarget.moveSingleTargetSources( + packagePath: packagePath, + manifest: manifestSyntax, + fileSystem: fileSystem, + verbose: !globalOptions.logging.quiet + ) + // Map the target type. let type: TargetDescription.TargetKind = switch self.type { case .library: .regular diff --git a/Sources/PackageModelSyntax/AddTarget.swift b/Sources/PackageModelSyntax/AddTarget.swift index 51a34323079..766bb2e99b8 100644 --- a/Sources/PackageModelSyntax/AddTarget.swift +++ b/Sources/PackageModelSyntax/AddTarget.swift @@ -57,6 +57,62 @@ public enum AddTarget { } } + // Check if the package has a single target with that target's sources located + // directly in `./Sources`. If so, move the sources into a folder named after + // the target before adding a new target. + public static func moveSingleTargetSources( + packagePath: AbsolutePath, + manifest: SourceFileSyntax, + fileSystem: any FileSystem, + verbose: Bool + ) throws { + // Make sure we have a suitable tools version in the manifest. + try manifest.checkEditManifestToolsVersion() + + guard let packageCall = manifest.findCall(calleeName: "Package") else { + throw ManifestEditError.cannotFindPackage + } + + if let arg = packageCall.findArgument(labeled: "targets") { + guard let argArray = arg.expression.findArrayArgument() else { + throw ManifestEditError.cannotFindArrayLiteralArgument( + argumentName: "targets", + node: Syntax(arg.expression) + ) + } + + // Check the contents of the `targets` array to see if there is only one target defined. + guard argArray.elements.count == 1, + let firstTarget = argArray.elements.first?.expression.as(FunctionCallExprSyntax.self), + let targetStringLiteral = firstTarget.arguments.first?.expression.as(StringLiteralExprSyntax.self) else { + return + } + + let targetName = targetStringLiteral.segments.description + let sourcesFolder = packagePath.appending("Sources") + let expectedTargetFolder = sourcesFolder.appending(targetName) + + // If there is one target then pull its name out and use that to look for a folder in `Sources/TargetName`. + // If the folder doesn't exist then we know we have a single target package and we need to migrate files + // into this folder before we can add a new target. + if !fileSystem.isDirectory(expectedTargetFolder) { + if verbose { + print("Moving existing files in \(sourcesFolder.relative(to: packagePath)) to \(expectedTargetFolder.relative(to: packagePath))...", terminator: "") + } + let contentsToMove = try fileSystem.getDirectoryContents(sourcesFolder) + try fileSystem.createDirectory(expectedTargetFolder) + for file in contentsToMove { + let source = sourcesFolder.appending(file) + let destination = expectedTargetFolder.appending(file) + try fileSystem.move(from: source, to: destination) + } + if verbose { + print(" done.") + } + } + } + } + /// Add the given target to the manifest, producing a set of edit results /// that updates the manifest and adds some source files to stub out the /// new target. diff --git a/Tests/CommandsTests/PackageCommandTests.swift b/Tests/CommandsTests/PackageCommandTests.swift index 2b83aae5934..2ce7794f15f 100644 --- a/Tests/CommandsTests/PackageCommandTests.swift +++ b/Tests/CommandsTests/PackageCommandTests.swift @@ -1120,7 +1120,6 @@ class PackageCommandTestCase: CommandsBuildProviderTestCase { } } - func testPackageAddTarget() async throws { try await testWithTemporaryDirectory { tmpPath in let fs = localFileSystem @@ -1152,6 +1151,58 @@ class PackageCommandTestCase: CommandsBuildProviderTestCase { } } + func testPackageAddTargetWithoutModuleSourcesFolder() async throws { + try await testWithTemporaryDirectory { tmpPath in + let fs = localFileSystem + let manifest = tmpPath.appending("Package.swift") + try fs.writeFileContents(manifest, string: + """ + // swift-tools-version: 5.9 + import PackageDescription + let package = Package( + name: "SimpleExecutable", + targets: [ + .executableTarget(name: "SimpleExecutable"), + ] + ) + """ + ) + + let sourcesFolder = tmpPath.appending("Sources") + try fs.createDirectory(sourcesFolder) + + try fs.writeFileContents(sourcesFolder.appending("main.swift"), string: + """ + print("Hello World") + """ + ) + + _ = try await execute(["add-target", "client"], packagePath: tmpPath) + + XCTAssertFileExists(manifest) + let contents: String = try fs.readFileContents(manifest) + + XCTAssertMatch(contents, .contains(#"targets:"#)) + XCTAssertMatch(contents, .contains(#".executableTarget"#)) + XCTAssertMatch(contents, .contains(#"name: "client""#)) + + let fileStructure = try fs.getDirectoryContents(sourcesFolder) + XCTAssertEqual(fileStructure, ["SimpleExecutable", "client"]) + XCTAssertTrue(fs.isDirectory(sourcesFolder.appending("SimpleExecutable"))) + XCTAssertTrue(fs.isDirectory(sourcesFolder.appending("client"))) + XCTAssertEqual(try fs.getDirectoryContents(sourcesFolder.appending("SimpleExecutable")), ["main.swift"]) + XCTAssertEqual(try fs.getDirectoryContents(sourcesFolder.appending("client")), ["client.swift"]) + } + } + + func testAddTargetWithoutManifestThrows() async throws { + try await testWithTemporaryDirectory { tmpPath in + await XCTAssertThrowsCommandExecutionError(try await execute(["add-target", "client"], packagePath: tmpPath)) { error in + XCTAssertMatch(error.stderr, .contains("error: Could not find Package.swift in this directory or any of its parent directories.")) + } + } + } + func testPackageAddTargetDependency() async throws { try await testWithTemporaryDirectory { tmpPath in let fs = localFileSystem From 2b8699a4e8f933f3bfde81bb995fe98dae10e8a4 Mon Sep 17 00:00:00 2001 From: Paul LeMarquand Date: Wed, 26 Mar 2025 09:48:02 -0400 Subject: [PATCH 2/4] Update Sources/PackageModelSyntax/AddTarget.swift Co-authored-by: Max Desiatov --- Sources/PackageModelSyntax/AddTarget.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Sources/PackageModelSyntax/AddTarget.swift b/Sources/PackageModelSyntax/AddTarget.swift index 766bb2e99b8..4e959ab1cc2 100644 --- a/Sources/PackageModelSyntax/AddTarget.swift +++ b/Sources/PackageModelSyntax/AddTarget.swift @@ -60,7 +60,7 @@ public enum AddTarget { // Check if the package has a single target with that target's sources located // directly in `./Sources`. If so, move the sources into a folder named after // the target before adding a new target. - public static func moveSingleTargetSources( + package static func moveSingleTargetSources( packagePath: AbsolutePath, manifest: SourceFileSyntax, fileSystem: any FileSystem, From 656c36b9f7d8a3c1dc5ea2a69fa192467fa8881c Mon Sep 17 00:00:00 2001 From: Paul LeMarquand Date: Wed, 26 Mar 2025 09:48:17 -0400 Subject: [PATCH 3/4] Update Sources/PackageModelSyntax/AddTarget.swift Co-authored-by: Max Desiatov --- Sources/PackageModelSyntax/AddTarget.swift | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/Sources/PackageModelSyntax/AddTarget.swift b/Sources/PackageModelSyntax/AddTarget.swift index 4e959ab1cc2..5dc1d2294fe 100644 --- a/Sources/PackageModelSyntax/AddTarget.swift +++ b/Sources/PackageModelSyntax/AddTarget.swift @@ -97,7 +97,16 @@ public enum AddTarget { // into this folder before we can add a new target. if !fileSystem.isDirectory(expectedTargetFolder) { if verbose { - print("Moving existing files in \(sourcesFolder.relative(to: packagePath)) to \(expectedTargetFolder.relative(to: packagePath))...", terminator: "") + print( + """ + Moving existing files from \( + sourcesFolder.relative(to: packagePath) + ) to \( + expectedTargetFolder.relative(to: packagePath) + )... + """, + terminator: "" + ) } let contentsToMove = try fileSystem.getDirectoryContents(sourcesFolder) try fileSystem.createDirectory(expectedTargetFolder) From b2a8c2a5cfc3eeabe1d2e15f4bcca352020ff281 Mon Sep 17 00:00:00 2001 From: Paul LeMarquand Date: Thu, 27 Mar 2025 11:28:17 -0400 Subject: [PATCH 4/4] Add unit tests for --- Sources/PackageModelSyntax/AddTarget.swift | 2 +- .../ManifestEditTests.swift | 136 ++++++++++++++++++ 2 files changed, 137 insertions(+), 1 deletion(-) diff --git a/Sources/PackageModelSyntax/AddTarget.swift b/Sources/PackageModelSyntax/AddTarget.swift index 5dc1d2294fe..addb4e4886a 100644 --- a/Sources/PackageModelSyntax/AddTarget.swift +++ b/Sources/PackageModelSyntax/AddTarget.swift @@ -64,7 +64,7 @@ public enum AddTarget { packagePath: AbsolutePath, manifest: SourceFileSyntax, fileSystem: any FileSystem, - verbose: Bool + verbose: Bool = false ) throws { // Make sure we have a suitable tools version in the manifest. try manifest.checkEditManifestToolsVersion() diff --git a/Tests/PackageModelSyntaxTests/ManifestEditTests.swift b/Tests/PackageModelSyntaxTests/ManifestEditTests.swift index 5ac024ddae6..bf76ac49683 100644 --- a/Tests/PackageModelSyntaxTests/ManifestEditTests.swift +++ b/Tests/PackageModelSyntaxTests/ManifestEditTests.swift @@ -615,6 +615,142 @@ class ManifestEditTests: XCTestCase { } } } +/// Assert that applying the moveSingleTargetSources operation to the manifest +/// produces the expected file system. +func assertFileSystemRefactor( + _ manifest: String, + inputFileLayout: [AbsolutePath] = [], + expectedFileLayout: [AbsolutePath] = [], + file: StaticString = #filePath, + line: UInt = #line +) throws { + let mockFileSystem = InMemoryFileSystem(); + for path in inputFileLayout { + try mockFileSystem.writeFileContents(path, string: "print(\"Hello, world!\")") + } + + try AddTarget.moveSingleTargetSources( + packagePath: AbsolutePath("/"), + manifest: Parser.parse(source: manifest), + fileSystem: mockFileSystem + ) + + for path in expectedFileLayout { + XCTAssertTrue(mockFileSystem.exists(path)) + } + + let unexpectedFiles = inputFileLayout.filter { !expectedFileLayout.contains($0) } + for path in unexpectedFiles { + XCTAssertFalse(mockFileSystem.exists(path)) + } +} + +class SingleTargetSourceTests: XCTestCase { + func testMoveSingleTargetSources() throws { + try assertFileSystemRefactor( + """ + // swift-tools-version: 5.5 + let package = Package( + name: "packages", + targets: [ + .executableTarget(name: "Foo"), + ] + ) + """, + inputFileLayout: [AbsolutePath("/Sources/Foo.swift")], + expectedFileLayout: [AbsolutePath("/Sources/Foo/Foo.swift")] + ) + } + + func testMoveSingleTargetSourcesNoTargets() throws { + try assertFileSystemRefactor( + """ + // swift-tools-version: 5.5 + let package = Package( + name: "packages" + ) + """, + inputFileLayout: [AbsolutePath("/Sources/Foo.swift")], + expectedFileLayout: [AbsolutePath("/Sources/Foo.swift")] + ) + } + + func testMoveSingleTargetSourcesAlreadyOrganized() throws { + try assertFileSystemRefactor( + """ + // swift-tools-version: 5.5 + let package = Package( + name: "packages", + targets: [ + .executableTarget(name: "Foo"), + ] + ) + """, + inputFileLayout: [AbsolutePath("/Sources/Foo/Foo.swift")], + expectedFileLayout: [AbsolutePath("/Sources/Foo/Foo.swift")] + ) + } + + func testMoveSingleTargetSourcesInvalidManifestTargets() throws { + XCTAssertThrowsError( + try assertFileSystemRefactor( + """ + // swift-tools-version: 5.5 + let package = Package( + name: "packages", + targets: "invalid" + ) + """ + ) + ) { error in + XCTAssertTrue(error is ManifestEditError) + } + } + + func testMoveSingleTargetSourcesInvalidManifestToolsVersion() throws { + XCTAssertThrowsError( + try assertFileSystemRefactor( + """ + // swift-tools-version: 5.4 + let package = Package( + name: "packages", + targets: [] + ) + """ + ) + ) { error in + XCTAssertTrue(error is ManifestEditError) + } + } + + func testMoveSingleTargetSourcesInvalidManifestTarget() throws { + XCTAssertThrowsError( + try assertFileSystemRefactor( + """ + // swift-tools-version: 5.4 + let package = Package( + name: "packages", + targets: [.executableTarget(123)] + ) + """ + ) + ) { error in + XCTAssertTrue(error is ManifestEditError) + } + } + + func testMoveSingleTargetSourcesInvalidManifestPackage() throws { + XCTAssertThrowsError( + try assertFileSystemRefactor( + """ + // swift-tools-version: 5.5 + """ + ) + ) { error in + XCTAssertTrue(error is ManifestEditError) + } + } +} // FIXME: Copy-paste from _SwiftSyntaxTestSupport