[SR-13235] libSwiftPM should vend a ClangTarget's module map generation information to clients #2838
Conversation
abertelrud
requested review from
aciidgh,
friedbunny and
neonichu
as code owners
abertelrud
force-pushed
the
eng/SR-13235-libswiftpm-should-vend-modulemap-type
branch
from
6bb68a8 to
16ac7cc
Compare
|
@swift-ci please smoke test |
| @@ -71,6 +71,49 @@ public struct ModuleMapGenerator { | |||
| self.diagnostics = diagnostics | |||
| } | |||
|
|
|||
| /// Determine the type of module map that applies to the specifed public-headers directory. | |||
| public static func determineModuleMapType(includeDir: AbsolutePath, moduleName: String, fileSystem: FileSystem) -> ModuleMapType { | |||
There was a problem hiding this comment.
What do you think about refactoring and moving the type computation logic from generateModuleMap so we don't have two implementations?
There was a problem hiding this comment.
Absolutely. This was always the plan, and was noted in the PR description. But the intent was to do it in two phases so that one could be more safely pulled back to 5.3, with the switch for SwiftPM to use the module type being for the next release. But I think it's probably too late to nominate for 5.3 in any case, so I've converted this back to a draft PR until I've put the second part in place.
But to be clear, it was never the intent to leave both implementations in the long run. :)
There was a problem hiding this comment.
I've updated the PR to do this.
| @@ -356,6 +356,9 @@ public class ClangTarget: Target { | |||
|
|
|||
| /// The path to include directory. | |||
| public let includeDir: AbsolutePath | |||
|
|
|||
| /// The target's module map type; determined by the file system layout, and controls whether the build system will use a custom module map, generate a module map, etc. | |||
| public let moduleMapType: ModuleMapType | |||
There was a problem hiding this comment.
I am not really sure that it makes sense to encode the module map type in the target. Clients which need to find out the module map type can directly call ModuleMapGenerator API, right?
There was a problem hiding this comment.
I think it makes sense to separate the decision from the generator, so clients could provide their own generator if desired, but could still use the type as determined by libSwiftPM.
There was a problem hiding this comment.
Right, the longer-term goal here is to have the information vended by libSwiftPM include all the semantic information that's relevant about the target. One of those is the disposition of its headers. It still makes sense to have a module map generation facility in SwiftPM (at least while it has its own custom build system), but I expect that many clients that can already generate module maps would want to use their own facility and just needs to know the decisions that SwiftPM made. This can also include things like showing this information to users in some form, e.g. when inspecting a package-defined target to find out how SwiftPM interpreted it. The plan is also to add this to the describe command.
As I mentioned in the previous response, this PR has the minimal support that could be pulled back to 5.3, but as it is probably too late for that kind of change in 5.3, I've converted this to a draft PR will I fill in the rest of the changes to complete the picture here. I'll probably still keep them as separate independently testable commits, for clarity.
There was a problem hiding this comment.
This looks great, this is pretty much exactly what I was envisioning w.r.t 50872333 👍
|
Since this is not going into 5.3 anyway at this point, I will rework this so that we use the same module map implementation all around (ideally also for the XCBuild support). |
abertelrud
force-pushed
the
eng/SR-13235-libswiftpm-should-vend-modulemap-type
branch
from
16ac7cc to
18da011
Compare
…on info This also modifies module map generation to use the unified logic for determining the module map type, and cleans up diagnostics reporting. With detection of module map type (based on a target's header layout) separate from generation of a module map, the existing ModuleMapGenerator API of having a struct that gets initialized only to perform the one operation (either determining module map type of generating a module map) is a bit clunky. We may want to refine that to make these standalone functions. Vending the target's module map layout information provides more semantic information to the client than just vending the raw contents of the generated module map. This semantic information can then (for example) be shown in the user interface of an IDE, etc. rdar://65678318
abertelrud
force-pushed
the
eng/SR-13235-libswiftpm-should-vend-modulemap-type
branch
from
18da011 to
ee1ab4c
Compare
|
@swift-ci please smoke test |
|
Not sure if the Linux failure is related, but it might well be. Looking into it.
|
|
The same test passed in the self-hosted test, however. Also, the failure is in one of the overlay shims modules, not affected by whether or not a particular Clang target exports module a from its headers. |
|
@swift-ci please smoke test linux |
This also modifies module map generation to use the unified logic for determining the module map type, and cleans up diagnostics reporting.
With detection of module map type (based on a target's header layout) separate from generation of a module map, the existing ModuleMapGenerator API of having a struct that gets initialized only to perform the one operation (either determining module map type of generating a module map) is a bit clunky. We may want to refine that to make these standalone functions, but I think that can be done as a separate PR.
Vending the target's module map layout information provides more semantic information to the client than just vending the raw contents of the generated module map. This semantic information can then (for example) be shown in the user interface of an IDE, etc.
rdar://65678318