Attempt to clarify build and prebuild command documentation

[dabrahams]

It's not clear if I've exactly captured the intent of the existing docs. In particular, the existing docs seem to imply that prebuild commands can only generate .swift files and that no prebuild output files will be interpreted as resources. My proposed changes contradict that implication.

Tightened up the writing in several places as well.

[MaxDesiatov]

@tomerd would we ever want to change this behavior? As currently specified it has consequences for #7053, where we'll need to rerun these prebuild commands when target triple changes. We get the target triple when the build process is initiated and earlier, certainly not during resolution time as

a change to a declared dependendencies

makes me think.

[dabrahams]

IIUC this is just a question for @tomerd and doesn't have a bearing on the acceptability of this PR?

[tomerd]

I think that is def worth considering changing, such that there are more conditions which trigger re-running rebuild commands.

[MaxDesiatov]

IDK, specifying that "The only trigger for re-running a prebuild command is a change to declared dependencies" feels too strong to me if we have to change this behavior in the process of my work on #7053

[dabrahams]

I don't understand. If you want to change the behavior of the system you can change the documentation appropriately. Why the reluctance to precisely document the current behavior? Saying that does not make a promise about the future.

[tomerd]

agree with Dave the doc should reflect the state of things and then updated when behavior changes

[abertelrud]

I agree as well — the documentation should match the implementation, and change whenever the behaviour changes.

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

Is it true today that today's implementation doesn't run prebuild commands at the start of every build? That is different from the original intent, which is that prebuild commands run before every build, so that they can generate files whose names can't be predicted from inputs, since neither of the two main builds systems ("swift build" and "xcbuild") can handle discovery of new source files during the build.

[dabrahams]

If that's the case I should obviously change the wording to reflect it… but that said I see no reason for them to run before every build if the only goal is to “generate files whose names can't be predicted from inputs.” You can do that once and record a marker that it's been done, then use those files over and over until they are cleaned or declared dependencies of the prebuild command change. If running before every build is necessary, there must be some other, currently unstated motivation for that requirement. In fact, I extrapolated the behavior I documented from what I had been told about a prebuild command's reason for existence and the most efficient way to achieve that.

So that I document the right thing: are they run before every build of every target, or are they only run before builds of targets that are using the plugin?

[tomerd]

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:

[dabrahams]

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

[tomerd]

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

[dabrahams]

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.

[tomerd]

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"

[dabrahams]

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.

[tomerd]

#6901 (comment)

[tomerd]

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`.

[dabrahams]

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.

[tomerd]

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

[dabrahams]

Please explain how and why.

[tomerd]

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

[tomerd]

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

[tomerd]

cc @amartini51

[TimTr]

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.

[tomerd]

@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

[tomerd]

@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..."

[tomerd]

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?

[dabrahams]

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.

[tomerd]

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

[abertelrud]

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.

[tomerd]

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

[dabrahams]

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

[dabrahams]

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?

[tomerd]

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:

[tomerd]

thanks @dabrahams added some comments, one factual question regarding the output directory of prebuilt command we need input on

[abertelrud]

thanks @dabrahams added some comments, one factual question regarding the output directory of prebuilt command we need input on

Not sure I understand the conflict (referring to the question "which one is true?"). There is no assumption about the name of the output directory, so whatever the plugin passes in the outputFilesDirectory: parameter when creating the prebuildCommand is where SwiftPM / Xcode looks for plugin-created files. The example in the documentation happens to use a directory called "GeneratedFiles", but there's not conflict there; that's just an example.