Check for the include directory for a library Clang target #3583
Conversation
elsh
requested review from
abertelrud,
friedbunny,
neonichu and
tomerd
as code owners
Sources/PackageModel/Target.swift
Outdated
| var isDirectory: Bool { | ||
| var isDir: ObjCBool = false | ||
| let exists = FileManager.default.fileExists(atPath: self.pathString, isDirectory: &isDir) | ||
| return exists && isDir.boolValue |
There was a problem hiding this comment.
I think we need to go through the FileSystem abstraction here instead of checking the local file system directly. It might be a bit awkward to get a handle to the currently used file system object here, one option could be moving the check to TargetSourcesBuilder.
There was a problem hiding this comment.
Moved a check out of this
Sources/PackageModel/Target.swift
Outdated
| @@ -427,7 +428,8 @@ public final class ClangTarget: Target { | |||
| dependencies: [Target.Dependency] = [], | |||
| buildSettings: BuildSettings.AssignmentTable = .init() | |||
| ) { | |||
| assert(includeDir.contains(sources.root), "\(includeDir) should be contained in the source root \(sources.root)") | |||
|
|
|||
| assert(includeDir.isDirectory && includeDir.contains(sources.root), "\(includeDir) should be contained in the source root \(sources.root)") | |||
There was a problem hiding this comment.
Should this be an assertion or is this a user error? Assertions are for programming errors, and are compiled out in release builds.
There was a problem hiding this comment.
Moved a check out of this and throw an error per below
elsh
force-pushed
the
es-assert
branch
2 times, most recently
from
7d37ffb to
5722c8e
Compare
| @@ -916,6 +916,10 @@ public final class PackageBuilder { | |||
| // 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 targetType == .library, !fileSystem.exists(publicHeadersPath) { | |||
There was a problem hiding this comment.
@abertelrud @neonichu Is this target type check enough here? A quick sanity check shows it is but the comment above seems to suggest otherwise.
|
@swift-ci smoke test |
elsh
changed the title
elsh
changed the title
|
@swift-ci smoke test |
|
@swift-ci smoke test |
|
@swift-ci smoke test |
|
Thanks @elsh, this looks good to me. |
|
@swift-ci smoke test |
Motivation:
Resolves rdar://52162539 ([SR-10975]: Improve the error message when a C target is missing the include directory).
Modifications:
Added a check to verify the include dir exists for a library clang target if toolchain is 5.5+.
Result:
With the change, it now properly throws an error indicating missing include directory.