Revisions for SE-0303 based on review feedback #1309
Conversation
abertelrud
commented
Mar 24, 2021
•
abertelrud
commented
- folded "prebuild" and "postbuild" into "buildTool" capability
- added more details about different types of build commands and when to choose what
- added more details about how package plugins are applied to targets
- extended the PackagePlugin API to provide more information about input files
- extended the PackagePlugin API to clearly define the Path type
- made API names more Swift-like ("outputDir" -> "outputDirectory") etc
- extended the Future Directions section to describe various things left out of initial proposal
- added some explanation of using build commands to modify artifacts at end of build
- updated examples to new API names
|
|
||
| Examples of plugins that need to use prebuild commands include SwiftGen and other tools that need to see all the input files, and whose output files are determined by the contents (not just the paths) of the input files. Such a plugin usually generates just one command and configures it with an output directory into which all generated sources will be written. | ||
|
|
||
| Because they run on every build, prebuild commands should use caching to do as little work as possible (ideally none) when there are no changes to their inputs. |
There was a problem hiding this comment.
could / should the API for Prebuild Commands include some kind of "Storage" that the plugin can use to save the state to making it easier to compute changes between invocations?
There was a problem hiding this comment.
Perhaps. The expectation was that the plugin would use the outputDirectory for any cached files between invocations, and that it would pass a command line argument to the prebuild command to a directory inside of its outputDirectory. Perhaps the TargetBuildContext directory needs a better name, since it's easy to confuse with the outputDirectory of the prebuild command.
There was a problem hiding this comment.
I should probably improve one of the examples to illustrate this as well.
There was a problem hiding this comment.
My understanding was that
/// Optional initial working directory of the command.
workingDirectory: Path? = nil,
can be used for the "skratchpad" for such plugin, say I can write a bunch of timestamps there to know which files I need to scan again etc; the same could be used for some "already generated files" etc right?
Did I get that wrong?
There was a problem hiding this comment.
As proposed, this directory is intended to be the way in which the prebuild command communicates passes output files to the build system: whatever is in that directory at the end of running the command is passed into the build system's planning step so that files get compiled, etc.
So the prebuild command needs to make sure that when it finishes running, the output files directory contains the set of files that should be compiled. It can set timestamps of those files to anything it wants to (in particular, if it changes them, then the build system will recompile those files; if it keeps the files the same, then it won't).
The scratchpad for other uses (cache files that the build system shouldn't try to compile, etc) can be put into the targetBuildContext.outputDirectory, which is the plugin's output directory (as opposed to the command). The plugin would typically use a separate subdirectory of its own output directory as the output files directory of each command it creates, but it can also create other files there.
Perhaps the names need to be made more distinct (maybe targetBuildContext.outputDirectory should be renamed?). I will also see if I can add a simple example in the comment to clarify.
There was a problem hiding this comment.
I am adding this to the command for TargetBuildContext.outputDirectory:
/// A plugin would usually create a separate subdirectory of this directory
/// for each command it creates, and the command would be configured to
/// write its outputs to that directory. The plugin may also create other
/// directories for cache files and other file system content that either
/// it or the command will need.
but will also see if I can extend one of the examples.
There was a problem hiding this comment.
I changed the name to pluginWorkDirectory to differentiate it from the command output directories, but am not overjoyed with that name. Open to suggestions. I'm sure there will be plenty of naming opinions during review and this can easily be adjusted if the semantics stay the same.
|
Great work! Looks far better now. There’re missing commas in line 701 and 707. Check it out:) |
Thank you very much for taking a look and commenting! I think there is still some clarification needed around how the output directory of prebuild commands is used, so I will look at making that a bit more clear. In particular talking about how to form a path for a cache vs the outputs that the build system should see. |
Thanks for pointing that out! I definitely need to put the latest edits here into the code again to make sure the syntax is correct. I'll make these fixes. |
- removed "postbuild" and folded "prebuild" into "buildTool" capability- added more details about build and prebuild commands
- extended the PackagePlugin API to provide more information about input files
- extended the PackagePlugin API to clearly define the Path type
- made API names more Swift-like ("outputDir" -> "outputDirectory") etc
- extended the Future Directions section to mention contextual information about the product
- updated examples to new API names
…s for plugin targets, as can be done for others
…can use availability annotations to maintain source code compatibility.
…onfusion with the one in the target build context (and `outputFilesDirectory` is more descriptive anyway).
Fix a spelling mistake. Co-authored-by: Konrad `ktoso` Malawski <konrad.malawski@project13.pl>
…ugins as a future direction
…plied to targets. Add description of support for formatters/linters as well as more advanced support for postbuild commands to the Futures section.
abertelrud
force-pushed
the
swiftpm-extensible-build-tools-revision
branch
from
eade6ff
to
f53f4a8
Compare
|
|
||
| - `succeeded` — a boolean that is true if and only if the command completed successfully (note that if the build was cancelled, the postbuild command is not run at all) | ||
| - `buildConfiguration` — the build configuration that was built (either `debug` or `release`) | ||
| - `productDirectory` — absolute path of the directory containing the built products |
There was a problem hiding this comment.
I see, yeah that's minimal enough :)
Nice to have debug/release.
No need for more in initial version I think
There was a problem hiding this comment.
A question which may come up: so... would a postBuild be a source formatter or it can't get access to the sources?
There was a problem hiding this comment.
It can read the sources but not write to them. I realize that many source formatters today rely on being able to modify the sources, but for security reasons I don't think we want to allow that in general. We should extend the API to allow them to create a structured diagnostic file that has fixits, and then make it easy for the end user to view and apply those fixits.
There was a problem hiding this comment.
but for security reasons I don't think we want to allow that in general
can you elaborate on that a bit?
There was a problem hiding this comment.
A question which may come up: so... would a postBuild be a source formatter or it can't get access to the sources?
could/should it be preBuild? since you want the build to use/validate the new formatted code?
There was a problem hiding this comment.
but for security reasons I don't think we want to allow that in general
can you elaborate on that a bit?
It seems risky to allow a plugin to have write access to the source directory during a regular build, since it would be easy for it to make changes that the package author isn't aware of. What would make sense would be to allow this as maybe a new capability that adds a subcommand to swift package, where the whole point is on-demand modification of the source. But if I just apply a plugin and do swift build, it seems prudent for the sandbox to prevent modification of the sources.
There was a problem hiding this comment.
We should extend the API to allow them to create a structured diagnostic file that has fixits, and then make it easy for the end user to view and apply those fixits.
I think this is a great idea for supporting linters, feel like a new plugin "type" or "capability"
We should have a new capability for linters and code formatters, but I do think that allowing regular build tools to pass along structured diagnostics would be good in general. For example, protoc should ideally be able to emit fixits, etc. That's something we can add over time, since we'd also need to have a way for those tools to created the structured diagnostics.
There was a problem hiding this comment.
but for security reasons I don't think we want to allow that in general
can you elaborate on that a bit?
It seems risky to allow a plugin to have write access to the source directory during a regular build, since it would be easy for it to make changes that the package author isn't aware of. What would make sense would be to allow this as maybe a new capability that adds a subcommand to
swift package, where the whole point is on-demand modification of the source. But if I just apply a plugin and doswift build, it seems prudent for the sandbox to prevent modification of the sources.
Although I suppose in practice there isn't a large distinction between having a tool that generates new code (which can contain anything) vs modifying existing code. Perhaps it's more that it seems surprising to do a build and then end up with modification to the code as a side effect.
There was a problem hiding this comment.
What would make sense would be to allow this as maybe a new capability that adds a subcommand to swift package, where the whole point is on-demand modification of the source.
I think this is a capability / plugin type we need to add regardless if it perform source modifications, but not sure this needs to be part of the initial API.
There was a problem hiding this comment.
What would make sense would be to allow this as maybe a new capability that adds a subcommand to swift package, where the whole point is on-demand modification of the source.
I think this is a capability / plugin type we need to add regardless if it perform source modifications, but not sure this needs to be part of the initial API.
Absolutely agreed. I was just suggesting the direction that should take, not that it should be added here.
| /// file may not be written if no postbuild command ends up being created. | ||
| /// The plugin may choose to pass this path to a postbuild command on the | ||
| /// command line or in the environment. | ||
| var postbuildResultFile: Path { get } |
There was a problem hiding this comment.
this is only relevant to post build plugins, right? I wonder if we need a different context object for different type of plugins or find some other way to customize the context API along these lines as we are likely to have more such attributes that are specific to the type of plugin and we would not want to "pollute" the general context with
There was a problem hiding this comment.
Because we wanted to make the capabilities very generic (combining the prebuild, build, and postbuild) capabilities, there's actually no such thing as a postbuild plugin anymore. Rather, any plugin with the build capability can produce an arbitrary assortment of prebuild commands, regular build commands, and postbuild commands.
For different capabilities I agree that the input context would be different. TargetBuildContext is very much geared toward build-oriented plugins.
There was a problem hiding this comment.
Another approach here, which is what I had originally done but then changed it to this, would be to allow the plugin to make up a path and pass it to the CommandConstructor in the creation of a postbuild command. SwiftPM would then create a result bundle file at that location. That might also allow us to extend the API in the future to be more fine-grained about what kind of results it wants there, e.g. tests or no tests, etc.
There was a problem hiding this comment.
That would be an easy change to make, and as I revisit this, I'm inclined to do so.
|
|
||
| This would be needed in order to implement more advanced tools such as code generators or linkers. | ||
|
|
||
| ### Specific Support for Code Formatters |
There was a problem hiding this comment.
IMO this applies more to linters than formatters, as the latter modify code while the former emit information
There was a problem hiding this comment.
I'd thought of them as the same but that distinction makes sense. I'll update it.
There was a problem hiding this comment.
Actually, separately I wrote:
/// Another possible capability could be specific support
/// for code linters that could emit structured diagnostics with fix-its,
/// or for code formatters that can modify the source code as a separate
/// action outside the build.
So I guess some part of my brain did make that distinction.
…cussion around first review
… proposal. Not sure whether this is the proper way to do it or whether to pair them (proposal and review discussion) together on the same line.
…eter where the creation of a postbuild command can include a location of where SwiftPM should write the build result file
…t proposal they do not address all the things that people want from commands that run after both building, testing, etc are complete (i.e. true "postbuild" commands). Instead add an accessor for the built-products directory and a description of how regular build commands can be configured to act on artifacts after they are produced by adding the path of the artifact as an input to a regular build command. This is most likely to be useful for custom build tool plugins associated with a root-level package, but can be useful in some cases.
