Skip to content

Add initial experimental support for combined documentation for multiple targets #84

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

Merged
merged 17 commits into from
Aug 9, 2024

Conversation

d-ronnqvist
Copy link
Contributor

@d-ronnqvist d-ronnqvist commented Jun 14, 2024

Bug/issue #, if applicable: rdar://116698361

Summary

This adds an experimental --enable-experimental-combined-documentation flag to the DocC plugin which does two things

  • Enabled targets to link to their dependencies in the generate-documentation build
  • Creates a combined documentation archive that contains all the generated documentation from the build.

For more information on how this behaves, see the Testing section.

It also adds updates the plugins with some new utilities to build documentation tasks in an ordered based on their target dependencies and to query the DocC executable for its supported features.

Dependencies

This requires a DocC version that supports combined documentation. For example; the version of DocC that come with the Swift 6.0 toolchain, the Swift trunk development toolchain, or the default toolchain in Xcode 16.

Testing

To test this, another package needs to depend on this branch of my fork of the Swift-DocC Plugin:

.package(url: "https://github.com/d-ronnqvist/swift-docc-plugin", branch: "multi-target-documentation"),

This is an additive feature, builds that don't pass the --enable-experimental-combined-documentation flag should continue to behave as before (with some minor improvements to the information that's printed about the generated documentation archives).

Help text

When running swift package generate-documentation --help the plugin should only list the new --enable-experimental-combined-documentation flag if the DocC executable in current the Swift toolchain supports combined documentation.

Unsupported use

When running swift package generate-documentation --enable-experimental-combined-documentation with DocC from a Swift toolchain that doesn't support combined documentation, the plugin should fail with a descriptive error message

error: Unsupported use of '--enable-experimental-combined-documentation'. DocC version at '/path/to/current/docc' doesn't support combined documentation.

Building all targets

When running swift package generate-documentation --enable-experimental-combined-documentation without specifying any targets should build documentation for all targets that support documentation in the package—same as it does today.

A target that depends on another target in the same package can write links to that other target using links that start with a "/" and the module name of the other target. For example: ``/OtherTarget/SomeSymbol/someFunction()``.

After the plugin has generated documentation for all targets, it will create one more documentation archives that combines the generated documentation for all targets from the build.

Building some targets

When running swift package generate-documentation --enable-experimental-combined-documentation and specifying a number of targets using --target TargetName pairs, the plugin should build documentation for only targets—same as it does today.

A target that depends on another target in the same package can only write links to that target if it is also listed as one of the --target TargetName pairs passed to the generate-documentation command.

After the plugin has generated documentation for the specified targets, it will create one more documentation archives that combines the generated documentation for only the specified targets.

Checklist

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

  • Added tests
  • Ran the ./bin/test script and it succeeded
  • Updated plugin CLI help text. User facing documentation will be added when this feature nears completion.

@d-ronnqvist d-ronnqvist force-pushed the multi-target-documentation branch from 6a1592c to fd4f39b Compare June 14, 2024 10:22
@d-ronnqvist
Copy link
Contributor Author

@swift-ci please test

@d-ronnqvist
Copy link
Contributor Author

@swift-ci please test

@d-ronnqvist
Copy link
Contributor Author

@swift-ci please test

@d-ronnqvist
Copy link
Contributor Author

@swift-ci please test macOS

Comment on lines +78 to +87
extension DocCFeatures.Feature {
/// DocC supports writing diagnostic information to a JSON file, specified by `--diagnostics-output-path`.
static let diagnosticsFileOutput = DocCFeatures.Feature(name: "diagnostics-file")

/// DocC supports linking between documentation builds and merging archives into a combined archive.
static let linkDependencies = DocCFeatures.Feature(name: "dependency")

/// DocC supports grouping overloaded symbols.
static let overloads = DocCFeatures.Feature(name: "overloads")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is specific to the features inside the compiler why not implement this in https://github.com/swiftlang/swift-docc directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The plugin can't import SwiftDocC so this can't be defined there.

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems off to declare this data here. If this are DocC features supported by the plugin we may want to rename this to DocCSupportedFeatures (minor)

@sofiaromorales
Copy link
Contributor

Thanks for this work, David!

Some comments:

  1. If we emit a single combined documentation, also emitting the docs separately seems unnecessary. Something to consider is to not output the individual documentation archives if the --enable-experimental-combined-documentation was passed.

  2. Would make sense to emit the combined documentations grouped by dependencies? Since this produces a single doccarchive my first reaction was to think that I could link between them even if they don't depend on each other.

  3. As a future work would be nice to synthesize a top level page for all the targets.

@d-ronnqvist
Copy link
Contributor Author

  1. If we emit a single combined documentation, also emitting the docs separately seems unnecessary. Something to consider is to not output the individual documentation archives if the --enable-experimental-combined-documentation was passed.

When we previously talked about this PR I said that I had other follow-up changes to the current state of the PR and asked if you preferred that I commit them to this PR or make a follow-up PR after this first one was merged. This is one of those changes. Those changes also relate to some confusing behavior with the --output-dir command line option that more people will encounter now that there's a reason to build multiple target's documentation in one invocation. Adding this makes the PR a bit bigger but I you prefer, I can push those commits to this PR instead of holding them for a follow-up PR.

  1. Would make sense to emit the combined documentations grouped by dependencies? Since this produces a single doccarchive my first reaction was to think that I could link between them even if they don't depend on each other.

I don't understand what change this question is asking for.

  1. As a future work would be nice to synthesize a top level page for all the targets.

This is another follow-up change that I'm already working on and have previously demoed to the team and separately to the documentation workgroup. This change however, requires some changes to DocC, so it would be a follow-up to the other follow-up changes.

@sofiaromorales sofiaromorales self-requested a review August 7, 2024 10:52
Copy link
Contributor

@sofiaromorales sofiaromorales left a comment

Choose a reason for hiding this comment

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

🚀

@d-ronnqvist
Copy link
Contributor Author

@swift-ci please test

Copy link

@daniel-grumberg daniel-grumberg left a comment

Choose a reason for hiding this comment

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

I am concerned about the correctness of the error catching and cancellation logic. Additionally I would prefer if we used swift concurrency as this is new code.

- Check for errors after queue has run the build operations
- Avoid repeat-visiting targets when constructing the build graph
- Update internal-only documentation comment
…wift-docc-plugin into multi-target-documentation
@d-ronnqvist d-ronnqvist dismissed daniel-grumberg’s stale review August 8, 2024 09:00

This feedback has been addressed.

@d-ronnqvist
Copy link
Contributor Author

I am concerned about the correctness of the error catching and cancellation logic.

I've fixed the but with catching errors from the individual build tasks.

Additionally I would prefer if we used swift concurrency as this is new code.

I strongly disagree. Swift concurrency is not a suitable tool for this because it doesn't support defining explicit dependencies between tasks and executing them (potentially in parallel) in that dependency order.

If we were to use Swift concurrency for this we would need to implement our own task scheduler and executor to ensure that dependencies tasks are run before dependants tasks and that each task is only run once. This is not only a lot of extra code to write but it's a risk of bugs. Using Operation and OperationQueue from Foundation gives us a long-lived and well tested implementation of this for free.

@d-ronnqvist
Copy link
Contributor Author

@swift-ci please test

}
}

// Run all the documentation build tasks in reverse dependency order (dependencies before dependents).

Choose a reason for hiding this comment

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

Suggested change
// Run all the documentation build tasks in reverse dependency order (dependencies before dependents).
// Run all the documentation build tasks in dependency order (dependencies before dependents).

@d-ronnqvist
Copy link
Contributor Author

@swift-ci please test

@d-ronnqvist
Copy link
Contributor Author

@daniel-grumberg Like we talked about offline: I added this type to encapsulate running the build tasks and these tests to verify the behavior when one of the targets encounters an error during the build.

@d-ronnqvist
Copy link
Contributor Author

@swift-ci please test

@d-ronnqvist d-ronnqvist merged commit 63f47d3 into swiftlang:main Aug 9, 2024
2 checks passed
@d-ronnqvist d-ronnqvist deleted the multi-target-documentation branch August 9, 2024 12:34
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