Add PackageExtension library and incorporate build tool commands into native build system #3287

abertelrud · 2021-02-17T23:23:30Z

Add the new PackageExtension module and library and the other necessary pieces to invoke build tool commands from the native build system. This is only accessed when the feature flag to enable the not-yet-approved extensibility feature is set.

Motivation:

This is continued work toward the draft extensibility proposal. All of this functionality is still gated by feature flags that allows or prevents extension targets from appearing in the package graph, and it's likely that further changes will be made before the proposal is accepted. But this way there will be a complete implementation to try at the start of the proposal review period.

Modifications:

add a new PackageExtension library target and product to Package.swift
update bootstrap to install it in the runtime location used by PackageDescription (though in a different subdirectory, since it does not need to be versioned and also so that imports don't bleed over)
implement a DefaultExtensionRunner that compiles and runs the main executable
- this code reuses code from ManifestLoader but should be consolidated into a unified run-scripts-in-a-sandbox API
- this needs further enhancements, such as caching of compiled executables and sandboxing
pass through the extension evaluation results to the build planning and build operation stage
generate llbuild commands for the build tool commands emitted by extensions
add a unit test with a test fixture that builds a tool that uses a library and then has an extension that uses that tool
- the sample tool just takes file contents and generates code that contains the hex form of it as a string
- this tests that the build tool inputs and outputs are incorporated correctly into the build graph

There are several TODOs and FIXMEs in the code. They will be addressed in future PRs.

This builds on #3286 and it is best to review that PR first and then just the separate diffs that make up this PR.

abertelrud · 2021-02-17T23:24:25Z

Fixtures/Miscellaneous/Extensions/MySourceGenExtension/Package.swift

@@ -10,11 +10,23 @@ let package = Package(
        //     target: "MySourceGenExt"
        // )
    ],
+    dependencies: [
+        .package(url: "https://github.com/apple/swift-argument-parser.git", .upToNextMinor(from: "0.3.1")),


I forgot that I still have an external package reference here. I wanted an example that uses SwiftArgumentParser, which a real tool would, but will change this test fixture so that no network access is needed while running unit tests.

maybe use a local dependency like "../Foo"

That's what we usually do but I didn't want to vendor all of SwiftArgParser. In this case it isn't actually to test that we can use the dependency (that will come in a later fixture, with an extension in a depenency package), but this was just to have it be "real" since pretty much every command line tool source generator written in Swift will probably use ArgParser.

So I'll remove this one though. but ../Foo is a good idea for the upcoming extension-in-a-dependency fixture.

another option is to have this Fixture run only when testing locally so CI does not do network access

That's a good point. How'd you normally do that — environment variables?

yea that's one way

Sounds good. In this case it's not essential for this test, so there's no point spending the cycles even on local tests. I will modify the test case.

abertelrud · 2021-02-17T23:25:38Z

Fixtures/Miscellaneous/Extensions/MySourceGenExtension/Sources/MySourceGenToolLib/library.swift

+
+extension String {
+
+    public var quotedForSourceCode: String {


This is just to show the ability for the tool that's built for use by the extension to depend on its own library in turn.

abertelrud · 2021-02-17T23:26:24Z

Sources/Build/BuildOperation.swift

@@ -30,6 +30,9 @@ public final class BuildOperation: PackageStructureDelegate, SPMBuildCore.BuildS

    /// The closure for loading the package graph.
    let packageGraphLoader: () throws -> PackageGraph
+
+    /// The closure for evaluating extensions in the package graph.
+    let extensionEvaluator: (PackageGraph) throws -> [ResolvedTarget: [ExtensionEvaluationResult]]


This isn't an ideal way to call this, but is very much along the lines of the existing packageGraphLoader. The entire BuildOperation layer could use some refactoring.

abertelrud · 2021-02-17T23:28:09Z

Sources/Build/BuildPlan.swift

+        for command in extensionEvaluationResults.reduce([], { $0 + $1.commands }) {
+            // Prebuild and postbuild commands are handled outside the build system.
+            if case .buildToolCommand(_, _, _, _, _, _, _, let derivedSourcePaths) = command {
+                // TODO: What should we do if we find non-Swift sources here?


There are still several diagnostics to add of this nature — it should be an error, earlier on, if an extension emits source code that isn't compatible with the target into which it's being compiled. This is because SwiftPM doesn't yet allow mixed sources, and it's beyond the scope of the extensibility proposal to change that. In this case, this error should be caught and reported earlier on.

abertelrud · 2021-02-17T23:29:22Z

Sources/Build/ManifestBuilder.swift

@@ -510,6 +510,8 @@ extension LLBuildManifestBuilder {
            if target.underlyingTarget is SystemLibraryTarget { return }
            // Ignore Binary Modules.
            if target.underlyingTarget is BinaryTarget { return }
+            // Ignore Extension Targets.
+            if target.underlyingTarget is ExtensionTarget { return }


The reason we don't reflect the extension target's dependencies here is because we will later set up build rules that establish that establishes that dependency (just below).

abertelrud · 2021-02-17T23:30:21Z

Sources/Commands/SwiftTool.swift

+        do {
+            // Configure the inputs to the extension evaluation.
+            // FIXME: These paths are still fairly preliminary.
+            let buildEnvironment = try buildParameters().buildEnvironment


This part of the code will evolve in future PRs. There are likely to be edge cases that need to be fixed before the feature can be enabled after approval.

abertelrud · 2021-02-17T23:32:36Z

Sources/Commands/SwiftTool.swift

@@ -594,6 +594,33 @@ public class SwiftTool {
            throw error
        }
    }
+
+    /// Evaluate extensions for any reachable targets in the graph, and return a mapping from targets to corresponding evaluation results.
+    func evaluateExtensions(graph: PackageGraph) throws -> [ResolvedTarget: [ExtensionEvaluationResult]] {


Every build system (of any IDE) that wants to incorporate libSwiftPM support would need to do something like this, where after loading the package graph, it evaluates the package extensions in the graph and gets back a mapping from targets to the ordered list of results of applying the target's specified extensions to that target. Those results are build-system neutral descriptions of command that then need to be incorporated into the build system as appropriate.

abertelrud · 2021-02-17T23:32:54Z

Sources/Commands/SwiftTool.swift

                diagnostics: diagnostics,
                stdoutStream: stdoutStream
            )
        case .xcode:
            let graphLoader = { try self.loadPackageGraph(explicitProduct: explicitProduct, createMultipleTestProducts: true) }
+            // FIXME: Implement the custom build command provider also.


Support for the XCBuild build system will follow in a future PR.

abertelrud · 2021-02-17T23:33:31Z

Sources/PackageExtension/CMakeLists.txt

+# See http://swift.org/LICENSE.txt for license information
+# See http://swift.org/CONTRIBUTORS.txt for Swift project authors
+
+add_library(PackageExtension


This glue is all fairly similar to the corresponding logic for PackageDescription, except that it doesn't have to build two different versions.

abertelrud · 2021-02-17T23:33:52Z

Sources/PackageExtension/PackageExtension.swift

+    public func lookupTool(named name: String) throws -> Path {
+        // TODO: Rather than just appending the name, this should instead use
+        // a mapping of tool names to paths (passed in from the context).
+        return execsDir.appending(name)


This is preliminary. See the comments.

right. makes sense for Context to take in a function that will do the lookup, or a name:path map

Absolutely. In particular, it should be an error to depend on anything you didn't declare a dependency on.

My only concern is that this will preclude quick workarounds, such as dropping a tool into /usr/local/bin etc. Perhaps that should be a warning if you use a tool you didn't depend on.

One could also argue that a standard set of tools in the toolchain should be fair game, e.g. strip etc. Certainly awk and others seem reasonable. But it's something to be discussed, I think.

With all the things in this PR, I'm trying to stub out things that can be decided later if we think that it will "slot into" the current approach in a reasonable way. This is one of those where I think a name-to-path map would be reasonable.

abertelrud · 2021-02-17T23:34:22Z

Sources/PackageExtension/PackageExtension.swift

+        public let moduleName: String
+
+        /// The path of the target directory.
+        public let targetDir: Path


We will likely want to expose public-headers directories etc, but that will be easy to thread through.

abertelrud · 2021-02-17T23:35:05Z

Sources/PackageExtension/PackageExtension.swift

+    /// is in topologically sorted order, with immediate dependencies appearing
+    /// earlier and more distant dependencies later in the list. This is mainly
+    /// intended for generating lists of search path arguments, etc.
+    public let dependencies: [DependencyTargetInfo]


Only some tools, such as protoc, need to see information about the dependency closure.

abertelrud · 2021-02-17T23:36:47Z

Sources/PackageExtension/PackageExtension.swift

+
+    /// Absolute paths of the source files in the target. This might include
+    /// derived source files generated by other extensions).
+    public let sourceFiles: [Path]


We are likely to want to refine the notion of source vs resource vs other files before enabling this. Currently SwiftPM makes this determination before extensions have a chance to run — it would clearly be more flexible to have the extensions be a part of this determination. This will be refined in future PRs.

abertelrud · 2021-02-17T23:40:26Z

Sources/PackageExtension/PackageExtension.swift

+    atexit {
+        let encoder = JSONEncoder()
+        let data = try! encoder.encode(output)
+        fputc(0, stdout)


This is explained more thoroughly at the top of this source file. The way in which libPackageExtension communicates its information back to SwiftPM is a private implementation detail, but the idea is to avoid reliance on a Unix pipe (which is not as platform-neutral as one would like) or a separate file (which will require write access in the file system) and similar side-channel solutions by reusing stdout and simply considering the structured information to be the bytes after the last null byte.

abertelrud · 2021-02-17T23:41:10Z

Sources/PackageExtension/PackageExtension.swift

+        fflush(stdout)
+    }
+    // Look for the input JSON as the last argument of the invocation.
+    guard let data = ProcessInfo().arguments.last?.data(using: .utf8) else {


This is something we want to fix. It would probably be better to pass it in on stdin (to avoid any limits on command line lengths), but we don't have TSC support for that yet.

abertelrud · 2021-02-17T23:41:51Z

Sources/Workspace/DefaultExtensionRunner.swift

+        // the JSON-serialized ExtensionEvaluationResult. Since this appears after any free-form output from the
+        // script, it can be safely split out while maintaining the ability to see debug output without resorting
+        // to side-channel communication that might be not be very cross-platform (e.g. pipes, file handles, etc).
+        var stdoutPieces = try result.output.get().split(separator: 0, omittingEmptySubsequences: false)


See earlier comments.

abertelrud · 2021-02-17T23:42:15Z

Sources/Workspace/DefaultExtensionRunner.swift

+    fileprivate func invoke(compiledExec: AbsolutePath, input: Data) throws -> (outputJSON: Data, stdoutText: Data) {
+        // FIXME: It would be more robust to pass it as `stdin` data, but we need TSC support for that.  When this is
+        // changed, PackageExtension will need to change as well (but no extensions need to change).
+        var command = [compiledExec.pathString]


Support for running this in a sandbox is coming in a future PR.

abertelrud · 2021-02-17T23:43:00Z

@swift-ci please smoke test

abertelrud · 2021-02-17T23:51:30Z

Investigating the Linux failure. I expect that this will take a couple of tries to get right, what with the new PackageExtension library, etc.

tomerd · 2021-02-18T00:02:33Z

Sources/Commands/SwiftTool.swift

+            let diagnostics = DiagnosticsEngine()
+
+            // Create the cache directory, if needed.
+            try localFileSystem.createDirectory(cacheDir, recursive: true)


do we also need to create the outputs dir?

That's a good point. Technically we don't have to create either of them, since that happens lower down in the stack. We can probably just make it part of the contract that evaluateExtensions() creates the directories as needed, so that not every client has to. I'll fix it. Thanks!

Removed this call as redundant.

abertelrud · 2021-02-18T00:09:22Z

@swift-ci please smoke test

tomerd · 2021-02-18T00:13:04Z

Sources/PackageExtension/PackageExtension.swift

+}
+
+/// A constructed command.
+fileprivate struct Command: Encodable {


side question: why do all these need to be decodable / coddle?

These are passed back to SwiftPM via JSON. That's also how the context comes in.

This part is just like PackageDescription. But it's not visible in the public-facing API that that's what happens.

tomerd · 2021-02-18T00:13:24Z

Sources/PackageExtension/PackageExtension.swift

+
+    public func fatalError(_ message: String, file: Path? = nil, line: Int? = nil) {
+        emit(error: message, file: file, line: line)
+        exit(1)


intentional?

It needs a better implementation, but I expect that unlike the package description, there will need to be a way to do error checking on things like not being able to find a tool and then good error messages. It could also be a thrown error though. This is something I'd like to discuss — we didn't talk that much about what a good error API for this use case should look like.

Removed this for now. We can discuss the best API here as a refinement.

abertelrud · 2021-02-18T00:19:57Z

Hmm... the macOS error is due to a source error that I'm not seeing locally on macOS. I'd be curious to know what's different about the tools, but can certainly change the code to make local and bot both be happy.

abertelrud · 2021-02-18T00:22:18Z

Hmm... the macOS error is due to a source error that I'm not seeing locally on macOS. I'd be curious to know what's different about the tools, but can certainly change the code to make local and bot both be happy.

Interesting, I'm using 5.4 locally, and the macOS tools are

16:14:14 swift --version
16:14:14 + swift --version
16:14:29 Apple Swift version 5.3.2 (swiftlang-1200.0.45 clang-1200.0.32.28)
16:14:29 Target: x86_64-apple-darwin19.6.0

I'll change the code so 5.3 is happy with it.

Linux toolchain is:

Swift version 5.4-dev (LLVM 404bcd566cdfe27, Swift 37d52fbb16a1de2)

which explains why it succeeded.

tomerd · 2021-02-18T04:13:40Z

@swift-ci please smoke test

abertelrud · 2021-02-18T04:43:55Z

I'll fix the macOS source code failure (seems specific to 5.3 toolchain — Linux is using 5.4 dev and works fine) in about an hour.

abertelrud · 2021-02-18T06:29:28Z

@swift-ci please smoke test

abertelrud · 2021-02-18T08:38:15Z

@swift-ci please smoke test

tomerd · 2021-02-18T20:39:10Z

Sources/Build/BuildPlan.swift

@@ -544,11 +544,15 @@ public final class SwiftTargetBuildDescription {

    /// The modulemap file for this target, if any.
    private(set) var moduleMap: AbsolutePath?
+
+    /// The results of having applied any extensions to this target.
+    public private(set) var extensionEvaluationResults: [ExtensionEvaluationResult]


does this needs to be a "var" if it is set on the constructor?

Good point — that can be a let. I modelled it after the others but there's no need.

I have made this fix.

tomerd · 2021-02-18T20:39:53Z

Sources/Build/BuildPlan.swift

@@ -1259,6 +1278,9 @@ public class BuildPlan {
        return AnySequence(productMap.values)
    }

+    /// The results of evaluating any extensions used by targets in this build.
+    private(set) var extensionEvaluationResults: [ResolvedTarget: [ExtensionEvaluationResult]]


I've made this fix as well.

tomerd · 2021-02-18T20:44:46Z

Sources/PackageExtension/PackageExtension.swift

+}
+
+// FIXME: This is preliminary.
+public struct Path: ExpressibleByStringLiteral, Encodable, Decodable {


it is unfortunate we need to define such in SwiftPM. would adding a dependency on https://github.com/apple/swift-system help?

Yes, this is really unfortunate. The issue is that the PackageExtension API has to be self-contained, like PackageDescription. If SwiftSystem becomes like Foundation or StdLib so that in be just imported (I don't know if that's the plan) then we could rely on it. But we should at least make sure the API is the same. Would it be awful to embed the same implementation?

tomerd · 2021-02-18T20:51:44Z

Sources/Workspace/DefaultExtensionRunner.swift

+
+    /// Helper function that compiles an extension as an executable and returns the path to it.
+    fileprivate func compile(sources: Sources, toolsVersion: ToolsVersion, cacheDir: AbsolutePath) throws -> AbsolutePath {
+        // FIXME: Much of this is copied from the ManifestLoader and should be consolidated.


coud we move this to some utility helper that can be reused form both places? seem unfortunate to have this twice

Yes, absolutely. That's the consolidation work I mentioned, where this would be combined with that in ManifestLoader to have a single API for running scripts that are similar to PackageDescription or PackageExtension. I'll make sure this is covered by a Radar and reference it here.

tomerd · 2021-02-18T20:52:33Z

Sources/Workspace/DefaultExtensionRunner.swift

+
+
+/// An extension runner that compiles the extension as an executable binary for the host platform, and invokes it as a subprocess.
+public struct DefaultExtensionRunner: ExtensionRunner {


I may have missed it, but I did not see any sandboxing here. is that still pending?

saw the comment about this coming in following PR

Yes, this will need to be added to both the running of this script and the command that gets created.

tomerd · 2021-02-18T20:53:32Z

Sources/Workspace/DefaultExtensionRunner.swift

+import PackageModel
+import SPMBuildCore
+import Foundation
+


would be good to run formatter on all the new files (like this one) so at least new additions are formatted in a standard way

Will do. That's swift-lint?

brew install swift-format
swift-format <file or directory>

Great, will do that for the new files.

I've reformatted the new ones (though using brew install swiftformat, not swift-format).

tomerd

this is a large PR so hard to review effectively. overall seems reasonable, I have some questions about sandboxing - which may have been left for future PR intentionally

abertelrud · 2021-02-18T21:15:13Z

this is a large PR so hard to review effectively. overall seems reasonable, I have some questions about sandboxing - which may have been left for future PR intentionally

Thanks for reviewing! I will apply the comments here, and yes, there are currently several things left for future PRs. I can iterate on this one or open separate ones.

I did split this into multiple commits to make things easier to look at, but can also split them into separate PRs.

…sources, and extend the ExtensionEvaluator to pass them on to extensions.

…sion, which will control availability annotations in the PackageExtension module.

…ption). This contains the APIs that extensions can use.

…native build system (for build tools only so far; prebuild and postbuild are coming). The default extension runner compiles and runs the extension as an executable, passing it input and receiving output in coordination with the implementation in PackageExtension (this works much the same as PackageDescription). There are still some stubbed-out areas to be filled in, and there needs to be caching and sandboxing added, but it is complete and works for what is implemented so far.

…PackageDescription, which needs either `4` or `4_2` appended to the path, there is no need for that for PackageExtension.

…though it's a small special-purpose library, there is no reason to keep it all in one source files. Internal declarations will work instead of fileprivates.

…t-parser) while running.

abertelrud · 2021-02-19T03:57:37Z

@swift-ci please smoke test

…files

abertelrud · 2021-02-19T06:23:52Z

@swift-ci please smoke test

abertelrud requested review from friedbunny, neonichu and tomerd as code owners February 17, 2021 23:23

abertelrud commented Feb 17, 2021

View reviewed changes

tomerd assigned abertelrud Feb 17, 2021

tomerd reviewed Feb 18, 2021

View reviewed changes

abertelrud force-pushed the extensibility-part-four branch from 0d1b747 to 9a52682 Compare February 18, 2021 06:16

tomerd added the ready Author believes the PR is ready to be merged & any feedback has been addressed label Feb 18, 2021

tomerd reviewed Feb 18, 2021

View reviewed changes

tomerd approved these changes Feb 18, 2021

View reviewed changes

abertelrud added 10 commits February 18, 2021 15:09

Extend TargetSourcesBuilder to return other files than sources and re…

b9061dc

…sources, and extend the ExtensionEvaluator to pass them on to extensions.

Pass through the tools version of the package that declares the exten…

162dc7f

…sion, which will control availability annotations in the PackageExtension module.

Add a PackageExtension module and library (analogous to PackageDescri…

eb67796

…ption). This contains the APIs that extensions can use.

Fix misuse of ProcessInfo.

fa2309d

Make a code change so that it compiles with Swift 5.3 toolchain.

b4cc6d6

Fix a problem compiling the extension when not in debug mode; unlike …

75eb7e1

…PackageDescription, which needs either `4` or `4_2` appended to the path, there is no need for that for PackageExtension.

Split apart PackageExtension.swift into individual source files. Al…

adb104e

…though it's a small special-purpose library, there is no reason to keep it all in one source files. Internal declarations will work instead of fileprivates.

Review feedback: change two properties to lets

4b19539

Change unit test fixture to not fetch packages (such as swift-argumen…

6574d35

…t-parser) while running.

abertelrud force-pushed the extensibility-part-four branch from 5fa12e0 to 6574d35 Compare February 19, 2021 01:54

Apply the results of running swiftformat on the newly added source …

3d306fc

…files

abertelrud merged commit a7815f5 into swiftlang:main Feb 19, 2021

abertelrud deleted the extensibility-part-four branch February 19, 2021 08:52



		/// An extension runner that compiles the extension as an executable binary for the host platform, and invokes it as a subprocess.
		public struct DefaultExtensionRunner: ExtensionRunner {

Add PackageExtension library and incorporate build tool commands into native build system #3287

Add PackageExtension library and incorporate build tool commands into native build system #3287

Uh oh!

Conversation

abertelrud commented Feb 17, 2021

Motivation:

Modifications:

Uh oh!

abertelrud Feb 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abertelrud commented Feb 17, 2021

Uh oh!

abertelrud commented Feb 17, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abertelrud commented Feb 18, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abertelrud Feb 17, 2021 •

edited

Loading

abertelrud commented Feb 18, 2021 •

edited

Loading