Skip to content

Move files when using package add-target on single target package #8413

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 4 commits into from
Mar 31, 2025
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions Sources/Commands/PackageCommands/AddTarget.swift
Original file line number Diff line number Diff line change
@@ -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
65 changes: 65 additions & 0 deletions Sources/PackageModelSyntax/AddTarget.swift
Original file line number Diff line number Diff line change
@@ -57,6 +57,71 @@ 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.
package static func moveSingleTargetSources(
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion (possibly-blocking): can we write automated test that validate this function in isolation, where we can test various code paths?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added some unit tests to cover off this function

packagePath: AbsolutePath,
manifest: SourceFileSyntax,
fileSystem: any FileSystem,
verbose: Bool = false
) 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 from \(
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.
53 changes: 52 additions & 1 deletion Tests/CommandsTests/PackageCommandTests.swift
Original file line number Diff line number Diff line change
@@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion (possibly-blocking): Although the current tests are great in validating the file system structure, can we add a test that validate the Package can still be built, and maybe even ensure the tests pass, if we run swift package add-target foo on an package containing a single source?

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
136 changes: 136 additions & 0 deletions Tests/PackageModelSyntaxTests/ManifestEditTests.swift
Original file line number Diff line number Diff line change
@@ -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