Skip to content

Attempt to clarify build and prebuild command documentation #6901

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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Attempt to clarify build and prebuild command documentation #6901

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9bef20b
Attempt to clarify build and prebuild commands
dabrahams Sep 14, 2023
006b53f
Typo
dabrahams Nov 8, 2023
370b26c
Be explicit that non-swift source languages isn't supported
dabrahams Nov 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 9 additions & 9 deletions Documentation/Plugins.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ To get access to a plugin defined in another package, add a package dependency o

### Making use of a build tool plugin

To make use of a build tool plugin, list its name in each target to which it should apply:
Add the plugin to the `plugins:` parameter of each target to which it applies:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Add the plugin to the `plugins:` parameter of each target to which it applies:
To make use of a build tool plugin, list its name in each target to which it should apply, using the `plugins:` parameter of the desired target:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why is that an improvement? It is wordier at the least and to me the extra words only seem to make room for misunderstanding.

Copy link
Contributor

@tomerd tomerd Nov 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

this is of course subjective, but I find it clearer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please explain how and why.

I can explain how it hurts:

  • “list a name in a target” is not a thing, and "list a name in a target…using the plugins parameter” is a very roundabout way of saying “pass it as an element of the plugins parameter.” It requires more thinking from a reader to comprehend.
  • “should apply” begs the question of what “should” means.

Copy link
Contributor

Choose a reason for hiding this comment

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

not a strong opinion about this one. I personally found the proposed sentence "Add the plugin to the plugins: parameter of each target to which it applies:" too terse, and was attempting to make it less so by reusing some of the original language. Maybe a good path forward could be:

"Build tool plugin operate on targets. Add the plugin to the plugins: parameter of each target to which it applies"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That doesn't make the sentence less terse. It does make the paragraph less terse, but again this information is misplaced. The relationship between targets and build tool plugins doesn't belong in the instructions about how to use them; it belongs in a discussion of what build tool plugins are.

Copy link
Contributor

Choose a reason for hiding this comment

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

#6901 (comment)


```swift
// swift-tools-version: 5.6
Expand Down Expand Up @@ -170,11 +170,15 @@ struct MyPlugin: BuildToolPlugin {

The plugin script can import *Foundation* and other standard libraries, but in the current version of SwiftPM, it cannot import other libraries.

##### Build Commands

In this example, the returned command is of the type `buildCommand`, so it will be incorporated into the build system's command graph and will run if any of the output files are missing or if the contents of any of the input files have changed since the last time the command ran.

Note that build tool plugins are always applied to a target, which is passed in the parameter to the entry point. Only source module targets have source files, so a plugin that iterates over source files will commonly test that the target it was given conforms to `SourceModuleTarget`.
The target to which the plugin applies is passed as the `target` parameter. Only source module targets have source files, so a plugin that iterates over source files will commonly test that the target it was given conforms to `SourceModuleTarget`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The target to which the plugin applies is passed as the `target` parameter. Only source module targets have source files, so a plugin that iterates over source files will commonly test that the target it was given conforms to `SourceModuleTarget`.
Build tool plugins are always applied to a specific target. The target to which the plugin applies is passed as the `target` parameter. Only source module targets have source files, so a plugin that iterates over source files will commonly test that the target it was given conforms to `SourceModuleTarget`.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How is that an improvement? It is clearly implied by the beginning of the first sentence as I wrote it. Adding needless words that add no information hurts comprehensibility.

Copy link
Contributor

@tomerd tomerd Nov 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

this is of course subjective, but I find it clearer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please explain how and why.

Copy link
Contributor

@tomerd tomerd Nov 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

IMO this additional sentence sets the context clearly for the rest of the paragraph.

Copy link
Contributor

Choose a reason for hiding this comment

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

generally, I would like to avoid rejecting PRs and instead work with the authors to fine tune the proposed changes such that everyone involved is satisfied.

perhaps a path forward could be to ask feedback from a few other folks, maybe they can help us find the right words that will make it less terse but still not over-wordy.

cc @abertelrud @TimTr @bitjammer

Copy link
Contributor

Choose a reason for hiding this comment

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

cc @amartini51

Copy link

Choose a reason for hiding this comment

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

I tend to agree that the additional wording "Build tool plugins are always applied to a specific target" felt like it was trying to document things beyond this parameter, which is out of scope. Documenting "bigger" stuff than the current scope can get out of hand quickly, and could be done for almost any API or parameter. In this case, the use feels pretty self-explanatory to me.

Perhaps it is useful to add an adjective like "mandatory" in this case, as perhaps that solves the suggester's concern? Seems like there is a desire for emphasis more than additional, external documentation.

Copy link
Contributor

@tomerd tomerd Nov 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@TimTr please note this is not API docs - this is a user guide in SwiftPM's documentation area. the API is also documented using standard API docs, eg: https://github.com/apple/swift-package-manager/blob/main/Sources/PackagePlugin/Protocols.swift

Copy link
Contributor

@tomerd tomerd Nov 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@dabrahams I read the entire section again starting from "Implementing the build tool plugin script" and then the two "Build Commands" and "Prebuild Commands" sub-sections. The latter seem to be focused on explaining the examples for build and pre-build while the paragraphs above give a more broad overview of how build tool plugins work. In the light, I would argue the whole paragraph "The target to which the plugin applies is passed as the target parameter. Only source module targets have source files, so a plugin that iterates over source files will commonly test that the target it was given conforms to SourceModuleTarget" should be hoisted to the top part (above "Build Commands" sub-section) as it is not specific to the example. At that point we can decide if we want to give additional context such as "SwiftPM will provide contextual information when invoking the plugin, through the context and target parameters..."


##### Prebuild Commands

A build tool plugin can also return commands of the type `prebuildCommand`, which run before the build starts and can populate a directory with output files whose names are not known until the command runs:
A build tool plugin can return a combination of build commands and prebuild commands. A `prebuildCommand` runs after the build tool plugin but before the build starts. This one populates a `GeneratedFiles/` directory:
Copy link
Contributor

@tomerd tomerd Nov 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

the original text said

Any prebuild commands are run after the plugin runs but before the build starts, and any files that are in the prebuild command's declared outputFilesDirectory will be evaluated as if they had been source files in the target. The prebuild command should add or remove files in this directory to reflect the results of having run the command

@neonichu @abertelrud which one is true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is GitHub lying to me? I do remember the words you're quoting, but the diff shows that this was the original text:

A build tool plugin can also return commands of the type prebuildCommand, which run before the build starts and can populate a directory with output files whose names are not known until the command runs.

Regardless, any files whose extensions don't match known source file extensions are in fact evaluated as though they were listed as resources, which makes the text you cited wrong.

Copy link
Contributor

Choose a reason for hiding this comment

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

mostly curious about outputFilesDirectory vs This one populates a GeneratedFiles/

the quote is from a paragraph down below that was deleted as part of the suggested changes

Copy link
Contributor

Choose a reason for hiding this comment

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

Whatever directory the plugin passes in the outputFilesDirectory parameter is the one that will be checked. There is nothing special about the name "GeneratedFiles" — that's just what the example happens to use as the name of its output directory.

Copy link
Contributor

Choose a reason for hiding this comment

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

okay, then I think the suggested documentation should point out usage of outputFilesDirectory (as done originally) and highlight that GeneratedFiles is just an example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me see if I can understand what that implies for this PR… back soon with a proposed update.

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 original pointing out of outputFilesDirectory was in an inaccurate context, and disconnected from the language on line 212 that covered the same topic. The new language that covers that information is in this paragraph. It doesn't mention outputFilesDirectory but instead mentions the output files themselves. I would expect that if the build tool left files in outputFilesDirectory directory that were not in the output files list it would be an error (otherwise, what's the point of having an output files list at all?) Are you sure this needs to change? If so, what change do you want?

Copy link
Contributor

Choose a reason for hiding this comment

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

my original point was that the proposed text talks about the concrete example below (GeneratedFiles) and not about the general case (whatever is specified in outputFilesDirectory). IMO the general text should describe the functionality more generally, and not just the example. does that make sense?

maybe something like the following (combination of the new + old text):

A build tool plugin can return a combination of build commands and prebuild commands. Any prebuild commands are run after the build tool plugin but before the build. Any files that are generate in the prebuild command's declared outputFilesDirectory will be evaluated as if they had been source files in the target.

In the following example, the prebuildCommand populates a GeneratedFiles/ directory:


```swift
import PackagePlugin
Expand Down Expand Up @@ -203,13 +207,9 @@ struct MyBuildToolPlugin: BuildToolPlugin {
}
```

In the case of prebuild commands, any dependencies must be binary targets, since these commands run before the build starts.

Note that a build tool plugin can return a combination of build tool commands and prebuild commands. After the plugin runs, any build commands are incorporated into the build graph, which may result in changes that require commands to run during the subsequent build.

Any prebuild commands are run after the plugin runs but before the build starts, and any files that are in the prebuild command's declared `outputFilesDirectory` will be evaluated as if they had been source files in the target. The prebuild command should add or remove files in this directory to reflect the results of having run the command.
A prebuild command has no inputs, so it will never be re-run due to changes in source files. The only trigger for re-running a prebuild command is a change to declared dependencies, which can only be (prebuilt) binary targets, since these commands run before any other targets have been built.

The current version of the Swift Package Manager supports generated Swift source files and resources as outputs, but it does not yet support non-Swift source files. Any generated resources are processed as if they had been declared in the manifest with the `.process()` rule. The intent is to eventually support any type of file that could have been included as a source file in the target, and to let the plugin provide greater controls over the downstream processing of generated files.
Any `.swift` files that are outputs of build commands or prebuild commands will be treated as Swift source files and compiled into the target being built by the plugin. Currently, compilation of output files in other source langauges isn't supported, so any other output files are treated as resources and processed as if they had been declared in the manifest with the `.process()` rule. The intent is to eventually support any type of file that could have been included as a source file in the target, and to let the plugin provide greater controls over the downstream processing of generated files.

### Command plugins

Expand Down