Check for the include directory for a library Clang target (#3583)

* Throw an error for a library clang target if include dir is not found

* Added toolchain version check / Added a test

* cleanup

* cleanup diagnostic msg

Author: elsh

Fixtures/CFamilyTargets/CLibraryNoIncludeDir/Cfactorial/Package.swift

@@ -0,0 +1,26 @@
+// swift-tools-version:5.5
+// The swift-tools-version declares the minimum version of Swift required to build this package.
+
+import PackageDescription
+
+let package = Package(
+    name: "Cfactorial",
+    products: [
+        // Products define the executables and libraries a package produces, and make them visible to other packages.
+        .library(
+            name: "Cfactorial",
+            targets: ["Cfactorial"]),
+    ],
+    dependencies: [
+        // Dependencies declare other packages that this package depends on.
+        // .package(url: /* package url */, from: "1.0.0"),
+    ],
+    targets: [
+        // Targets are the basic building blocks of a package. A target can define a module or a test suite.
+        // Targets can depend on other targets in this package, and on products in packages this package depends on.
+        .target(
+            name: "Cfactorial",
+            dependencies: [],
+	    path: "Sources/factorial"),
+    ]
+)

Fixtures/CFamilyTargets/CLibraryNoIncludeDir/Cfactorial/Sources/factorial/factorial.c

@@ -0,0 +1,6 @@
+//#include "include/factorial.h"
+#include "factorial.h"
+long factorial(int n) {
+    if (n == 0 || n == 1) return 1;
+    return n * factorial(n-1);
+}

Fixtures/CFamilyTargets/CLibraryNoIncludeDir/Cfactorial/Sources/factorial/factorial.h

@@ -0,0 +1,8 @@
+#ifndef factorial_h
+#define factorial_h
+
+#include <stdio.h>
+
+long factorial(int n);
+
+#endif /* factorial_h */

Fixtures/CFamilyTargets/CLibraryNoIncludeDir/Package.swift

@@ -0,0 +1,20 @@
+// swift-tools-version:5.5
+// The swift-tools-version declares the minimum version of Swift required to build this package.
+
+import PackageDescription
+
+let package = Package(
+    name: "Client",
+    dependencies: [
+        // Dependencies declare other packages that this package depends on.
+        // .package(url: /* package url */, from: "1.0.0"),
+	.package(path: "Cfactorial")
+    ],
+    targets: [
+        // Targets are the basic building blocks of a package. A target can define a module or a test suite.
+        // Targets can depend on other targets in this package, and on products in packages this package depends on.
+        .executableTarget(
+            name: "Client",
+            dependencies: ["Cfactorial"]),
+    ]
+)

Fixtures/CFamilyTargets/CLibraryNoIncludeDir/Sources/Client/main.swift

@@ -0,0 +1,4 @@
+import Cfactorial
+
+print(factorial(5))
+print("Hello, world!")

Sources/PackageLoading/PackageBuilder.swift

@@ -96,7 +96,7 @@ extension ModuleError: CustomStringConvertible {
                 (cycle.path + cycle.cycle).joined(separator: " -> ") +
                 " -> " + cycle.cycle[0]
         case .invalidPublicHeadersDirectory(let name):
-            return "public headers directory path for '\(name)' is invalid or not contained in the target"
+            return "public headers (\"include\") directory path for '\(name)' is invalid or not contained in the target"
         case .overlappingSources(let target, let sources):
             return "target '\(target)' has sources overlapping sources: " +
                 sources.map({ $0.description }).joined(separator: ", ")
@@ -914,13 +914,15 @@ public final class PackageBuilder {
             // It's not a Swift target, so it's a Clang target (those are the only two types of source target currently supported).
 
             // First determine the type of module map that will be appropriate for the target based on its header layout.
-            // FIXME: We should really be checking the target type to see whether it is one that can vend headers, not just check for the existence of the public headers path.  But right now we have now way of distinguishing between, for example, a library and an executable.  The semantics here should be to only try to detect the header layout of targets that can vend public headers.
             let moduleMapType: ModuleMapType
+            
             if fileSystem.exists(publicHeadersPath) {
                 let moduleMapGenerator = ModuleMapGenerator(targetName: potentialModule.name, moduleName: potentialModule.name.spm_mangledToC99ExtendedIdentifier(), publicHeadersDir: publicHeadersPath, fileSystem: fileSystem)
                 moduleMapType = moduleMapGenerator.determineModuleMapType(diagnostics: diagnostics)
-            }
-            else {
+            } else if targetType == .library, manifest.toolsVersion >= .v5_5 {
+                // If this clang target is a library, it must contain "include" directory.
+                throw ModuleError.invalidPublicHeadersDirectory(potentialModule.name)
+            } else {
                 moduleMapType = .none
             }
 

Tests/FunctionalTests/CFamilyTargetTests.swift

@@ -64,6 +64,17 @@ class CFamilyTargetTestCase: XCTestCase {
             XCTAssertDirectoryContainsFile(dir: debugPath, filename: "UmbrellaHeader.c.o")
         }
     }
+    
+    func testNoIncludeDirCheck() {
+        fixture(name: "CFamilyTargets/CLibraryNoIncludeDir") { prefix in
+            XCTAssertThrowsError(try executeSwiftBuild(prefix), "This build should throw an error") { err in
+                // The err.localizedDescription doesn't capture the detailed error string so interpolate
+                let errStr = "\(err)"
+                let missingIncludeDirStr = "\(ModuleError.invalidPublicHeadersDirectory("Cfactorial"))"
+                XCTAssert(errStr.contains(missingIncludeDirStr))
+            }
+        }
+    }
 
     func testCanForwardExtraFlagsToClang() {
         // Try building a fixture which needs extra flags to be able to build.

Tests/PackageLoadingTests/PackageBuilderTests.swift

@@ -1104,7 +1104,7 @@ class PackageBuilderTests: XCTestCase {
             )
 
             PackageBuilderTester(manifest, in: fs) { _, diagnostics in
-                diagnostics.check(diagnostic: "public headers directory path for 'Foo' is invalid or not contained in the target", behavior: .error)
+                diagnostics.check(diagnostic: "public headers (\"include\") directory path for 'Foo' is invalid or not contained in the target", behavior: .error)
             }
 
             manifest = Manifest.createV4Manifest(
@@ -1114,7 +1114,7 @@ class PackageBuilderTests: XCTestCase {
                 ]
             )
             PackageBuilderTester(manifest, in: fs) { _, diagnostics in
-                diagnostics.check(diagnostic: "public headers directory path for 'Bar' is invalid or not contained in the target", behavior: .error)
+                diagnostics.check(diagnostic: "public headers (\"include\") directory path for 'Bar' is invalid or not contained in the target", behavior: .error)
             }
         }