Add an ExtensionEvaluator for evaluating extensions in a package graph #3286
Conversation
abertelrud
requested review from
friedbunny,
neonichu and
tomerd
as code owners
|
@swift-ci please smoke test |
tomerd
added
the
ready
|
|
||
| // If this target does use any extensions, create the input context to pass to them. | ||
| // FIXME: We'll want to decide on what directories to provide to the extenion | ||
| let package = self.packages.first{ $0.targets.contains(target) }! |
There was a problem hiding this comment.
first! safe? replace with guard to make safer long term?
There was a problem hiding this comment.
Yes, this should be an internal error. A leftover from before cleaning up the code. Thanks for catching!
There was a problem hiding this comment.
Also, one would think that mapping a target back to its package should be common enough to have a method of some sort (which could then do caching etc) but I didn't spot one. I can create one but wanted to keep the code minimal here. But this should definitely throw an error (I try to avoid ! completely).
| ) | ||
|
|
||
| // Generate emittable Diagnostics from the extension output. | ||
| let diagnostics: [Diagnostic] = extensionOutput.diagnostics.map { diag in |
There was a problem hiding this comment.
maybe this can be extracted to an extension that will make this easier to read
| } | ||
|
|
||
| // Generate commands from the extension output. | ||
| let commands: [ExtensionEvaluationResult.Command] = extensionOutput.commands.map { cmd in |
There was a problem hiding this comment.
this can also be extracted to an extension imo
| /// FIXME: This should be asynchronous, taking a queue and a completion closure. | ||
| fileprivate func runExtension(sources: Sources, input: ExtensionEvaluationInput, extensionRunner: ExtensionRunner, diagnostics: DiagnosticsEngine, fileSystem: FileSystem) throws -> (output: ExtensionEvaluationOutput, stdoutText: Data) { | ||
| // Serialize the ExtensionEvaluationInput to JSON. | ||
| let encoder = JSONEncoder() |
There was a problem hiding this comment.
could the be made static at top level to avoid creating new instance every time?
| // Deserialize the JSON to an ExtensionEvaluationOutput. | ||
| let output: ExtensionEvaluationOutput | ||
| do { | ||
| let decoder = JSONDecoder() |
| fileprivate func runExtension(sources: Sources, input: ExtensionEvaluationInput, extensionRunner: ExtensionRunner, diagnostics: DiagnosticsEngine, fileSystem: FileSystem) throws -> (output: ExtensionEvaluationOutput, stdoutText: Data) { | ||
| // Serialize the ExtensionEvaluationInput to JSON. | ||
| let encoder = JSONEncoder() | ||
| encoder.outputFormatting = [.prettyPrinted, .withoutEscapingSlashes] |
There was a problem hiding this comment.
iirc we have the helpers for this in Basics
There was a problem hiding this comment.
Right, that's a good point. Also, this doesn't need to be pretty-printed, so I should probably just take this out.
| public let diagnostics: [Diagnostic] | ||
|
|
||
| // A location representing a file name or path and an optional line number. | ||
| // FIXME: This should be part of the Diagnostics APIs. |
There was a problem hiding this comment.
maybe add as an extension to Basics until we move to TSC
There was a problem hiding this comment.
That makes sense. I think maybe that could be a separate follow-on PR?
| var outputDir: String | ||
| var cacheDir: String | ||
| var execsDir: String | ||
| var options: [String: String] |
There was a problem hiding this comment.
Yes, or probably removed since we're not doing options initially.
|
The test failure is: which is a toolchain problem. |
|
@swift-ci please smoke test |
…by input parameters to the package graph in addition to the SWIFTPM_ENABLE_EXTENSION_TARGETS environment variable, so that unit tests can selectively enable it for synthetic-package tests.
…xtensions in targets to be evaluated and that returns the commands to run and other information. Some TODOs and FIXMEs in the code for things left to be filled in, such as product dependencies and separate extension usage arrays.
…ry formatting of the JSON encoding.
abertelrud
force-pushed
the
extensibility-part-three
branch
from
e9c5954 to
6aea3c9
Compare
|
@swift-ci please smoke test |
|
Not sure what's going on here: |
|
@swift-ci please self test linux |
|
@swift-ci please smoke test linux |
|
This Linux smoke test seems to be a failure while building libClang... |
|
@swift-ci please self test linux |
|
@swift-ci please smoke test linux |
??? |
|
Looking into the failure. |
|
@swift-ci please smoke test linux |
should be fixed with #3288 |
Add a first version of an ExtensionEvaluator that allows all use of extensions in targets to be evaluated and that returns the commands to run and other information. Some TODOs and FIXMEs in the code for things left to be filled in, such as product dependencies and separate extension usage arrays. This will be added in follow-on PRs.
Motivation:
This abstracts out the logic around invoking extensions and passing them inputs and receiving outputs, while leaving the details of how that is done (e.g. compiling and then running in a sandbox, or some other way) to concrete implementations. This also allows the facility to be at a lower level than the logic providing the extension runner.
There is no caching yet and but that will be added in future PRs (although most is likely to live in the concrete implementations of ExtensionRunner).
There is some similarity here with the manifest loader, and the intent is to evolve both so that there can be a shared implementation.
Modifications:
Add an ExtensionEvaluator plus an ExtensionRunner protocol.
Define a ExtensionEvaluationResult struct for capturing distilled results of invoking extensions. This includes diagnostics and plain-text output from running the extension (suitable for debugging of the extension, as with the manifest).
Define the three types of generated commands that are planned for the initial implementation: prebuild, built-tool, and postbuild.
This builds on #3285 and it is best to review that PR first and then just the diff 37b2915 of this PR.