Skip to content
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

Allow clang source module targets to be documented using the plugin #95

Merged
merged 5 commits into from
Sep 5, 2024

Conversation

cmcgee1024
Copy link
Member

@cmcgee1024 cmcgee1024 commented Aug 28, 2024

Summary

With this change you can now treat C/C++ targets through clang in the same way as Swift. They can be included in generated doccarchive and previewed in the same way.

Update the uses of SwiftSourceModuleTarget to use the more general SourceModuleTarget.

Dependencies

No dependencies on this change. DocC itself can generate documentation for C/C++ symbol graphs.

Testing

Git clone a package that has a C target, such as the SwiftPM enabled version of libarchive here and run the docc plugin to generate a preview of its documentation.

Steps:

  1. git clone -b vendor-libarchive https://github.com/cmcgee1024/swiftly.git
  2. cd swiftly/libarchive
  3. Modify the Package.swift file to point to a version of the swift-docc-plugin that permits documentation to be generated for C targets. Add the following package dependency to point to that revision:
let package = Package(
    ...
        .executable(
            name: "bsdunzip",
            targets: ["bsdunzip"]
        ),
    ],
    dependencies: [.package(path: "/Users/cmcgee/git/swift-docc-plugin")],   <---- Add this, pointing to the path to the enhanced swift-docc-plugin
  1. Generate a preview of the CArchive target swift package --disable-sandbox preview-documentation --target CArchive

Checklist

Make sure you check off the following items. If they cannot be completed, provide a reason.

  • Added tests (looking for suggestions on how to write this test)
  • Ran the ./bin/test script and it succeeded
  • Updated documentation if necessary
    • I haven't seen any documentation that assumes a Swift target, so perhaps the documentation is ok?

With this change you can now treat C/C++ targets through clang in
the same way as Swift. They can be included in generated doccarchive
and previewed in the same way.

Update the uses of SwiftSourceModuleTarget to use the more general
SourceModuleTarget.
@cmcgee1024 cmcgee1024 marked this pull request as ready for review August 28, 2024 18:11
@daniel-grumberg
Copy link

IIUC it would generate the Swift API documentation for C/ObjC/(Obj)C++ APIs and not the documentation for the underlying APIs, i.e. it does not resolve #47?

@cmcgee1024
Copy link
Member Author

@daniel-grumberg in my experience it will display the Swift view of the documentation, so yes this doesn't resolve #47. I don't understand enough about DocC internals to enable the language toggle on the top-right so that someone can switch between C/Swift/etc. Does the plugin control that somehow?

@daniel-grumberg
Copy link

This change would be a prerequisite but we would need an associated change in SwiftPM itself to generate SGFs with clang in the implementation of https://github.com/swiftlang/swift-package-manager/blob/d31cf0539de2590f54e11e5bae4771d7ee5a77e6/Sources/PackagePlugin/PackageManagerProxy.swift#L229

@daniel-grumberg
Copy link

@swift-ci please test

@cmcgee1024
Copy link
Member Author

Thanks @daniel-grumberg. I don't have the power to merge this PR, so I'll leave that to someone who does. I plan to look at making the SwiftPM side capable of generating the clang symbol graph.

@daniel-grumberg
Copy link

I would like for @d-ronnqvist to have a look but lgtm

@cmcgee1024
Copy link
Member Author

This could be the piece that is missing from the SwiftPM side. swiftlang/swift-package-manager#5639

@daniel-grumberg
Copy link

That looks mostly like it but it's pretty old and would need revisiting.

extension SwiftSourceModuleTarget: DocumentationBuildGraphTarget {
// We add the conformance here so that 'DocumentationBuildGraphTarget' doesn't need to know about 'SourceModuleTarget' or import 'PackagePlugin'.
struct SourceModuleDocumentationBuildGraphTarget: DocumentationBuildGraphTarget {
var sourceTarget: SourceModuleTarget
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: What's the reason for wrapping the SourceModuleTarget here as opposed to extending it? All the properties access same-name properties on the wrapped source target.

Copy link
Member Author

Choose a reason for hiding this comment

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

I couldn't extend it in the same was as before because SwiftSourceModuleTarget is a struct, and SourceModuleTarget is a protocol. Protocol conformance is explicit in Swift, unlike Go that has duck-type interface conformance. I couldn't figure out a way to do this in Swift through extensions. I'm quite new to Swift, and so maybe there's some kind of a trick that can be applied here. If you can think of something please let me know.

@@ -90,8 +90,8 @@ extension ArgumentExtractor {
)
}

guard let swiftSourceModuleTarget = target as? SwiftSourceModuleTarget else {
throw ArgumentParsingError.targetIsNotSwiftSourceModule(specifiedTarget)
guard let swiftSourceModuleTarget = target as? SourceModuleTarget else {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: This variable is still called Swift source module target, but it's now a general source target.

@cmcgee1024
Copy link
Member Author

@swift-ci please test

@cmcgee1024
Copy link
Member Author

@d-ronnqvist I've updated this PR and ran the CI, but I don't have permission to merge this. Anything else needed here, or can this be merged?

@d-ronnqvist
Copy link
Contributor

@d-ronnqvist I've updated this PR and ran the CI, but I don't have permission to merge this. Anything else needed here, or can this be merged?

This can be merged. Let me do that right now.

@d-ronnqvist d-ronnqvist merged commit 300ee6c into swiftlang:main Sep 5, 2024
2 checks passed
@d-ronnqvist d-ronnqvist mentioned this pull request Sep 5, 2024
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants