From dd6335facba4d2923ad41c7e91ce2f27f775decf Mon Sep 17 00:00:00 2001 From: Ankit Aggarwal Date: Tue, 11 Feb 2020 15:09:55 -0800 Subject: [PATCH 1/2] [PackageLoading] Better handling for directories with extension Package builder started considering directories inside a target as a file that can have a rule. However, this wasn't gated behind the tools version check and we were not handling directories that are explicitly declared as sources in the package manifest. --- .../PackageLoading/TargetSourcesBuilder.swift | 17 ++++++- .../PackageBuilderTests.swift | 47 +++++++++++++++++++ .../TargetSourcesBuilderTests.swift | 39 ++++++++++++++- 3 files changed, 100 insertions(+), 3 deletions(-) diff --git a/Sources/PackageLoading/TargetSourcesBuilder.swift b/Sources/PackageLoading/TargetSourcesBuilder.swift index 2aceefe4e08..2f716a860d4 100644 --- a/Sources/PackageLoading/TargetSourcesBuilder.swift +++ b/Sources/PackageLoading/TargetSourcesBuilder.swift @@ -221,6 +221,11 @@ public struct TargetSourcesBuilder { } } + /// Returns true if the given path is a declared source. + func isDeclaredSource(_ path: AbsolutePath) -> Bool { + return path == targetPath || declaredSources?.contains(path) == true + } + /// Compute the contents of the files in a target. /// /// This avoids recursing into certain directories like exclude or the @@ -258,14 +263,22 @@ public struct TargetSourcesBuilder { continue } - // Append and continue if the path doesn't have an extension or is not a directory. - if curr.extension != nil || !fs.isDirectory(curr) { + // Consider non-directories as source files. + if !fs.isDirectory(curr) { contents.append(curr) continue } // At this point, curr can only be a directory. // + // Starting tools version with resources, pick directories as + // sources that have an extension but are not explicitly + // declared as sources in the manifest. + if toolsVersion >= .vNext && curr.extension != nil && !isDeclaredSource(curr) { + contents.append(curr) + continue + } + // Check if the directory is marked to be copied. let directoryMarkedToBeCopied = target.resources.contains{ resource in let resourcePath = self.targetPath.appending(RelativePath(resource.path)) diff --git a/Tests/PackageLoadingTests/PackageBuilderTests.swift b/Tests/PackageLoadingTests/PackageBuilderTests.swift index 718b8ab176b..3950d0fe3a2 100644 --- a/Tests/PackageLoadingTests/PackageBuilderTests.swift +++ b/Tests/PackageLoadingTests/PackageBuilderTests.swift @@ -246,6 +246,53 @@ class PackageBuilderTests: XCTestCase { } } + func testDeclaredSourcesWithDot() throws { + let fs = InMemoryFileSystem(emptyFiles: + "/Sources/swift.lib/foo.swift", + "/Sources/swiftlib1/swift.lib/foo.swift", + "/Sources/swiftlib2/swift.lib/foo.swift", + "/Sources/swiftlib3/swift.lib/foo.swift", + "/Sources/swiftlib3/swift.lib/foo.bar/bar.swift", + "/done" + ) + + let manifest = Manifest.createV4Manifest( + name: "MyPackage", + targets: [ + TargetDescription( + name: "swift.lib" + ), + TargetDescription( + name: "swiftlib1", + path: "Sources/swiftlib1", + sources: ["swift.lib"] + ), + TargetDescription( + name: "swiftlib2", + path: "Sources/swiftlib2/swift.lib" + ), + TargetDescription( + name: "swiftlib3", + path: "Sources/swiftlib3/swift.lib" + ), + ] + ) + PackageBuilderTester(manifest, in: fs) { result, _ in + result.checkModule("swift.lib") { module in + module.checkSources(sources: ["foo.swift"]) + } + result.checkModule("swiftlib1") { module in + module.checkSources(sources: ["swift.lib/foo.swift"]) + } + result.checkModule("swiftlib2") { module in + module.checkSources(sources: ["foo.swift"]) + } + result.checkModule("swiftlib3") { module in + module.checkSources(sources: ["foo.bar/bar.swift", "foo.swift"]) + } + } + } + func testDeclaredExecutableProducts() { // Check that declaring executable product doesn't collide with the // inferred products. diff --git a/Tests/PackageLoadingTests/TargetSourcesBuilderTests.swift b/Tests/PackageLoadingTests/TargetSourcesBuilderTests.swift index c0caf0eba9a..e7adf56ee8d 100644 --- a/Tests/PackageLoadingTests/TargetSourcesBuilderTests.swift +++ b/Tests/PackageLoadingTests/TargetSourcesBuilderTests.swift @@ -60,7 +60,7 @@ class TargetSourcesBuilderTests: XCTestCase { XCTAssertEqual(contents, [ "/Bar.swift", "/Foo.swift", - "/Hello.something", + "/Hello.something/hello.txt", "/file", "/path/to/somefile.txt", "/some/path.swift", @@ -70,6 +70,43 @@ class TargetSourcesBuilderTests: XCTestCase { XCTAssertNoDiagnostics(diags) } + func testDirectoryWithExt() throws { + let target = TargetDescription( + name: "Foo", + path: nil, + exclude: ["some2"], + sources: nil, + publicHeadersPath: nil, + type: .regular + ) + + let fs = InMemoryFileSystem() + fs.createEmptyFiles(at: .root, files: [ + "/.some2/hello.swift", + "/Hello.something/hello.txt", + ]) + + let diags = DiagnosticsEngine() + + let builder = TargetSourcesBuilder( + packageName: "", + packagePath: .root, + target: target, + path: .root, + toolsVersion: .vNext, + fs: fs, + diags: diags + ) + + let contents = builder.computeContents().map{ $0.pathString }.sorted() + + XCTAssertEqual(contents, [ + "/Hello.something", + ]) + + XCTAssertNoDiagnostics(diags) + } + func testBasicRuleApplication() throws { let target = TargetDescription( name: "Foo", From 5b2d64e9260941893db9cf5ab603c637dce3687b Mon Sep 17 00:00:00 2001 From: Ankit Aggarwal Date: Fri, 14 Feb 2020 09:44:42 -0800 Subject: [PATCH 2/2] [PackageLoading] Handle duplicated declared sources A single source file can be declared multiple times in the manifest so we need to handle those cases. --- .../PackageLoading/TargetSourcesBuilder.swift | 4 +++ .../PackageBuilderTests.swift | 26 +++++++++++++++++++ 2 files changed, 30 insertions(+) diff --git a/Sources/PackageLoading/TargetSourcesBuilder.swift b/Sources/PackageLoading/TargetSourcesBuilder.swift index 2f716a860d4..011ee05b2f3 100644 --- a/Sources/PackageLoading/TargetSourcesBuilder.swift +++ b/Sources/PackageLoading/TargetSourcesBuilder.swift @@ -187,6 +187,10 @@ public struct TargetSourcesBuilder { } else { matchedRule = .compile } + // The source file might have been declared twice so + // exit on first match. + // FIXME: We should emitting warnings for duplicate// declarations. + break } } } diff --git a/Tests/PackageLoadingTests/PackageBuilderTests.swift b/Tests/PackageLoadingTests/PackageBuilderTests.swift index 3950d0fe3a2..7ed6522a0a7 100644 --- a/Tests/PackageLoadingTests/PackageBuilderTests.swift +++ b/Tests/PackageLoadingTests/PackageBuilderTests.swift @@ -293,6 +293,32 @@ class PackageBuilderTests: XCTestCase { } } + func testOverlappingDeclaredSources() throws { + let fs = InMemoryFileSystem(emptyFiles: + "/Sources/clib/subfolder/foo.h", + "/Sources/clib/subfolder/foo.c", + "/Sources/clib/bar.h", + "/Sources/clib/bar.c", + "/done" + ) + + let manifest = Manifest.createV4Manifest( + name: "MyPackage", + targets: [ + TargetDescription( + name: "clib", + path: "Sources", + sources: ["clib", "clib/subfolder"] + ), + ] + ) + PackageBuilderTester(manifest, in: fs) { result, _ in + result.checkModule("clib") { module in + module.checkSources(sources: ["clib/bar.c", "clib/subfolder/foo.c"]) + } + } + } + func testDeclaredExecutableProducts() { // Check that declaring executable product doesn't collide with the // inferred products.