Clean up the code and some of the API of compiling plugins, and improve persistence of cached compiler outputs #4290

abertelrud · 2022-04-12T02:26:09Z

Motivation

Provide more information to clients of libSwiftPM even when a compiled plugin doesn't have to be recompiled because nothing has changed since the last time it was. Also make the API a bit cleaner.

Details

Clients now get the command line and compiler output even in the case of failure, and also when the plugin doesn't have to be recompiled because nothing changed (previously this information was lost). This is done by serializing a proper struct to JSON rather than just storing the hexadecimal representation of the configuration hash.

Callers also have control over the name of the temporary directory to represent the plugin, rather than always having it be the last path component of the sources directory.

The changes to the construction of the command line to compile plugins isn't nearly as extensive as it looks — it's mostly blankspace changes due to nesting.

Modifications:

persist the compilation state as a JSON-coded struct rather than just a hexadecimal input hash
let clients of PluginScriptRunner control the name of the intermediates directory (and not rely on a Sources type)
clean up the struct members in the compilation result to have clearer names
adjust and extend unit tests to cover cached information

Result:

[After your change, what will change.]

…ve cached compiler outputs Clients now get the command line and compiler output even in the case of failure, and also when the plugin doesn't have to be recompiled because nothing changed (previously this information was lost). This is done by serializing a proper struct to JSON rather than just storing the hexadecimal representation of the configuration hash. Callers also have control over the name of the temporary directory to represent the plugin, rather than always having it be the last path component of the sources directory. The changes to the construction of the command line to compile plugins isn't nearly as extensive as it looks — it's mostly blankspace changes due to nesting.

abertelrud · 2022-04-12T02:26:17Z

@swift-ci please smoke test

tomerd · 2022-04-12T03:41:02Z

Sources/Workspace/DefaultPluginScriptRunner.swift

@@ -37,43 +37,46 @@ public struct DefaultPluginScriptRunner: PluginScriptRunner, Cancellable {
        self.cancellator = Cancellator(observabilityScope: .none)
    }

-    /// Public protocol function that starts compiling the plugin script to an executable. The tools version controls the availability of APIs in PackagePlugin, and should be set to the tools version of the package that defines the plugin (not of the target to which it is being applied). This function returns immediately and then calls the completion handler on the callbackq queue when compilation ends.
+    /// Public protocol function that starts compiling the plugin script to an executable. The name is used as the basename for the executable and auxiliary files.  The tools version controls the availability of APIs in PackagePlugin, and should be set to the tools version of the package that defines the plugin (not of the target to which it is being applied). This function returns immediately and then calls the completion handler on the callbackq queue after compilation ends.


changes in this file are a bit hard to follow. are there specific changes you want the reviewers to focus on?

Ah, yes, sorry about that — it's actually a bit easier when ignoring spaces, because the nesting changed.

Maybe one useful thing would be to see what is stored in the PersistedCompilationState struct. It doesn't yet have things like a fingerprint of the compiled plugin, but that's where it would be added. I think that is the most interesting thing to look at that isn't just cleaning up names and structure.

This is pretty well tested by unit tests that check for whether the compilation actually happened etc so I am not too concerned about the logic itself.

abertelrud · 2022-04-12T03:55:53Z

Sources/Workspace/DefaultPluginScriptRunner.swift

+                    commandLine: commandLine,
+                    environment: toolchain.swiftCompilerEnvironment,
+                    inputHash: compilerInputHash,
+                    duration: Double(startTime.distance(to: .now()).milliseconds() ?? 0) / 1000.0,


Millisecond precision seems enough here.

perhaps use DispatchTimeInterval so units as obvious?

Good point, though because the idea is to encode this into JSON, that would lead to additional code to encode or decode it.

I looked at adding a fileprivate extension to DispatchTimeInterval to make it adopt Codable, but that's not permissible — we then have to make it codable for the whole module.

Since we're currently not using the duration maybe I will just take it out for now.

one way to do this is use a property wrapper which implements the serialization for the duration property. this is how we often deal with date formatting

I see. As this is unused, I'll go ahead and remove it, but I'll keep that in mind for next time.

tomerd · 2022-04-12T16:21:39Z

Sources/Workspace/DefaultPluginScriptRunner.swift

+        }
+
+        // Check if we already have a compiled executable and a persisted state (we only recompile if things have changed).
+        let stateFilePath = self.cacheDir.appending(component: execName + ".state")


nit: use "json" as extension, and names the file "state"

Just the literal name "state" and not based on the name of the executable? In that case we should maybe call the executable just "plugin". But that makes it harder to see which plugin it is when looking for example using top.

I was trying to keep all three files (executable, state, and diagnostics) with the same base name derived from the plugin name. So maybe just changing it to "json" here makes sense.

sry I meant execName + "-state.json"

That makes sense.

tomerd · 2022-04-12T16:23:27Z

Sources/Workspace/DefaultPluginScriptRunner.swift

-                    // We failed to read the `.inputhash` file. We warn about it but proceed with the compilation (a cache miss).
-                    observabilityScope.emit(warning: "Couldn't read previous hash of plugin compilation inputs (\(error)")
+                    // We couldn't write out the `.state` file. We warn about it but proceed.
+                    observabilityScope.emit(debug: "... couldn't save plugin compilation state (\(error))")


curious about the "..." prefix in these debug messages

Probably just left over from print-style debugging (my indication for the end of an activity, whereas the beginning has a "..." suffix instead). I'll change it.

tomerd · 2022-04-12T16:23:38Z

Sources/Workspace/DefaultPluginScriptRunner.swift

-            }
-            else {
-                // There is no need to recompile the executable, so we just call the completion handler with the results from last time.
+                // Return a PluginCompilationResult for both the successful and unsuccessful cases (to convey diagnostics, etc).
                let result = PluginCompilationResult(


nit: return here?

I'll move up the return.

tomerd · 2022-04-12T16:24:30Z

Tests/SPMBuildCoreTests/PluginInvocationTests.swift

            var hostTriple: Triple {
                return UserToolchain.default.triple
            }

-            func compilePluginScript(sources: Sources, toolsVersion: ToolsVersion, observabilityScope: ObservabilityScope) throws -> PluginCompilationResult {
+            func compilePluginScript(sourceFiles: [AbsolutePath], pluginName: String, toolsVersion: ToolsVersion, observabilityScope: ObservabilityScope) throws -> PluginCompilationResult {


nit: these kind of functions that involve I/O seems like they should be async

I think the client calls that used this have been removed, so I can probably remove these.

Removed it.

I pushed the tsc_await to all the call sites.

abertelrud · 2022-04-13T05:42:54Z

@swift-ci please smoke test

abertelrud · 2022-04-13T05:53:44Z

@swift-ci please smoke test

abertelrud · 2022-04-13T05:54:34Z

@tomerd Thanks for your feedback, I think I've addressed all of it.

tomerd · 2022-04-13T17:23:06Z

👍

compnerd · 2022-04-15T22:42:37Z

This regressed the Windows build: https://ci-external.swift.org/job/oss-swift-windows-toolchain-x86_64-vs2019/509/console

…nd improve persistence of cached compiler outputs (#4290)" This reverts commit 09d77a7.

…ve persistence of cached compiler outputs (swiftlang#4290) Clients now get the command line and compiler output even in the case of failure, and also when the plugin doesn't have to be recompiled because nothing changed (previously this information was lost). This is done by serializing a proper struct to JSON rather than just storing the hexadecimal representation of the configuration hash. Callers also have control over the name of the temporary directory to represent the plugin, rather than always having it be the last path component of the sources directory. (cherry picked from commit 09d77a7)

… improve persistence of cached compiler outputs (#4290 & #4299) (#4309) * Clean up the code and some of the API of compiling plugins, and improve persistence of cached compiler outputs (#4290) Clients now get the command line and compiler output even in the case of failure, and also when the plugin doesn't have to be recompiled because nothing changed (previously this information was lost). This is done by serializing a proper struct to JSON rather than just storing the hexadecimal representation of the configuration hash. Callers also have control over the name of the temporary directory to represent the plugin, rather than always having it be the last path component of the sources directory. (cherry picked from commit 09d77a7) * Fix breakage on Windows, which doesn't have `ProcessResult.ExitStatus.signalled`. (#4299) (cherry picked from commit f9f74f1)

abertelrud requested review from neonichu, tomerd and elsh as code owners April 12, 2022 02:26

abertelrud self-assigned this Apr 12, 2022

tomerd reviewed Apr 12, 2022

View reviewed changes

abertelrud commented Apr 12, 2022

View reviewed changes

tomerd approved these changes Apr 12, 2022

View reviewed changes

tomerd reviewed Apr 12, 2022

View reviewed changes

abertelrud added 4 commits April 12, 2022 22:21

Remove duration which isn't used

3ffa730

Change debug output

ff85d84

Move up return statement

e68d6ca

Name the file "-state.json" instead of "-state"

a8b4e10

Remove the synchronous version of compilePluginScript

470eed8

abertelrud force-pushed the eng/plugin-compilation-api-improvements branch from e64084f to 470eed8 Compare April 13, 2022 05:52

abertelrud merged commit 09d77a7 into swiftlang:main Apr 15, 2022

abertelrud deleted the eng/plugin-compilation-api-improvements branch April 15, 2022 19:07

compnerd added a commit that referenced this pull request Apr 15, 2022

Revert "Clean up the code and some of the API of compiling plugins, a…

6c89819

…nd improve persistence of cached compiler outputs (#4290)" This reverts commit 09d77a7.

compnerd mentioned this pull request Apr 15, 2022

Revert "Clean up the code and some of the API of compiling plugins, and improve persistence of cached compiler outputs" #4298

Closed

abertelrud mentioned this pull request Apr 15, 2022

Fix breakage on Windows, which doesn't have ProcessResult.ExitStatus.signalled. #4299

Merged

abertelrud mentioned this pull request Apr 19, 2022

[5.7] Clean up the code and some of the API of compiling plugins, and improve persistence of cached compiler outputs (#4290 & #4299) #4309

Merged

Clean up the code and some of the API of compiling plugins, and improve persistence of cached compiler outputs #4290

Clean up the code and some of the API of compiling plugins, and improve persistence of cached compiler outputs #4290

Uh oh!

Conversation

abertelrud commented Apr 12, 2022

Motivation

Details

Modifications:

Result:

Uh oh!

abertelrud commented Apr 12, 2022

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 Apr 13, 2022 • 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!

abertelrud Apr 13, 2022 • 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!

abertelrud commented Apr 13, 2022

Uh oh!

abertelrud commented Apr 13, 2022

Uh oh!

abertelrud commented Apr 13, 2022

Uh oh!

tomerd commented Apr 13, 2022

Uh oh!

compnerd commented Apr 15, 2022

Uh oh!

Uh oh!

abertelrud Apr 13, 2022 •

edited

Loading

abertelrud Apr 13, 2022 •

edited

Loading