Replace custom path type with URL in plugin API #7184

neonichu · 2023-12-11T21:26:28Z

Plugins introduced their own custom path type which doesn't have Windows support, causing several issues with plugins on Windows. We should rectify this situation by using Foundation's URL type instead which already works fine on Windows.

rdar://117230149

neonichu · 2023-12-11T21:27:03Z

@swift-ci please test

neonichu · 2023-12-11T21:28:35Z

Two things to point out:

this introduces a bunch of deprecation warnings for the code in PackagePlugin itself since we still need to create paths for backwards compatibility. I am not sure how to avoid these
since URL is now part of the public interface of PackagePlugin, we can no longer use implementation-only imports for Foundation

neonichu · 2023-12-11T21:39:15Z

I will look into adding a few new test cases which exercise the new APIs instead of the old ones.

neonichu · 2023-12-12T00:34:22Z

I'm actually a bit surprised that the test suite passes, I seem to be able to see problems with rather trivial manual testing...

neonichu · 2023-12-12T01:23:42Z

/Users/neonacho/Projects/public/swift-package-manager/Fixtures/Miscellaneous/Plugins/MySourceGenPlugin/.build/plugins/cache/MySourceGenPrebuildPlugin

Ah fun, turns out if you ever build using bootstrap, that plugin library will be preferred for running plugins, so what happened was that my SwiftPM was using URLs but the plugin was using an old library which just led to kinda broken paths on the plugin side.

neonichu · 2023-12-12T19:57:38Z

Added a test now which exercises both build tool and prebuild APIs using URLs. This required bumping our version to 5.11 so that the new APIs can be used in the example.

I think two tasks remain for this PR:

change the deprecation of Path such that we can avoid deprecations in our own code (or at least minimize them)
introduce some kind of versioning for the plugin API library to avoid the same confusion I had with mixing and matching incompatible versions. This should mostly impact development, but it's still worthwhile to handle

neonichu · 2023-12-12T23:05:50Z

Sources/PackagePlugin/PluginMessages.swift


        struct CommandConfiguration: Codable {
+            var version = 2


Used this as a simple way to "version" the library. This allows us to detect cases where an old plugin library is being used with a newer SwiftPM which would otherwise silently exhibit broken behavior.

neonichu · 2023-12-12T23:06:28Z

I think this should be ready now. There are still a handful of deprecation warnings that I am not certain how to resolve.

neonichu · 2023-12-12T23:06:37Z

@swift-ci please test

neonichu · 2023-12-12T23:06:52Z

@swift-ci please test package compatibility

neonichu · 2023-12-12T23:07:04Z

@swift-ci please test windows

neonichu · 2023-12-12T23:41:12Z

Hm, looks like on Linux, we don't see the "Applying" messages for plugins?

neonichu · 2023-12-13T01:33:09Z

duplicate symbol '_async_MainTu' in:
    /Users/ec2-user/jenkins/workspace/swift-package-manager-with-xcode-self-hosted-PR-osx/branch-main/swiftpm/.build/x86_64-apple-macosx/debug/swift_package.build/Entrypoint.swift.o
    /Users/ec2-user/jenkins/workspace/swift-package-manager-with-xcode-self-hosted-PR-osx/branch-main/swiftpm/.build/x86_64-apple-macosx/debug/swift_test.build/Entrypoint.swift.o
duplicate symbol '_async_Main' in:
    /Users/ec2-user/jenkins/workspace/swift-package-manager-with-xcode-self-hosted-PR-osx/branch-main/swiftpm/.build/x86_64-apple-macosx/debug/swift_package.build/Entrypoint.swift.o
    /Users/ec2-user/jenkins/workspace/swift-package-manager-with-xcode-self-hosted-PR-osx/branch-main/swiftpm/.build/x86_64-apple-macosx/debug/swift_test.build/Entrypoint.swift.o
ld: 2 duplicate symbols for architecture x86_64
[174/175] Linking SwiftPMPackageTests
error: fatalError

Yah, I'm seeing this locally, too. It seems like we have accidentally created a dependency on 5.10 compilers here which hide the async main symbol.

MaxDesiatov · 2023-12-13T09:28:12Z

I'm puzzled why this error didn't surface previously on CI when testing PRs that introduced this async entrypoint in the first place. Are reverts to those entrypoints the only available fix?

MaxDesiatov · 2023-12-13T10:57:46Z

AFAIU the error wasn't independently reproducible with any of the async entrypoint PRs tested separately, and that's how they were merged, almost simultaneously. The combination of them leads to this error though. I'm applying the least disruptive revert of two possible in #7196.

MaxDesiatov · 2023-12-13T16:22:26Z

@swift-ci test

grynspan · 2023-12-13T16:41:59Z

Will this break plugins? How can a plugin detect when it should use URL instead of the old API?

MaxDesiatov · 2023-12-13T16:43:44Z

The API is additive, thus not expected to break anything. IIUC, plugins using new swift-tools-version should use the new API, while old ones should still continue to work.

grynspan · 2023-12-13T16:49:32Z

Thanks!

neonichu · 2023-12-13T17:25:27Z

Yep, plugins declared in packages with tools-version 5.10 and earlier will work exactly as before. When upgrading a package to 5.11, there'll be deprecation warnings for Path based APIs to encourage using the URL ones, but they'll still work. We may declare the old APIs as unavailable at some point in the future to force migration to the new APIs, but even then, existing plugins will continue to work.

neonichu · 2023-12-13T18:31:58Z

Ah, the "Applying" message doesn't show on Linux, because it is from "Applying debug entitlements to \(buildProduct.binaryPath.prettyPath())", it isn't about applying the plugin. Code-signing only happens on macOS, of course. I thought otherwise because Xcode does have a "Apply build tool plug-in" message.

neonichu · 2023-12-13T20:37:35Z

Was able to resolve the last few deprecation warnings with @tomerd's help -- there was even a small bug hiding in there with an optional property not being set in the URL case.

Plugins introduced their own custom path type which doesn't have Windows support, causing several issues with plugins on Windows. We should rectify this situation by using Foundation's URL type instead which already works fine on Windows. rdar://117230149

neonichu · 2023-12-13T20:39:15Z

@swift-ci please test

neonichu · 2023-12-13T20:39:24Z

@swift-ci please test package compatibility

neonichu · 2023-12-13T20:39:36Z

@swift-ci please test windows

neonichu · 2023-12-13T20:42:37Z

@swift-ci please test windows

neonichu · 2023-12-13T21:30:59Z

@swift-ci please test windows

neonichu · 2023-12-13T23:36:47Z

@swift-ci please test windows

MaxDesiatov · 2023-12-14T10:46:10Z

@swift-ci test windows

neonichu · 2023-12-14T17:45:07Z

@swift-ci test windows

neonichu · 2023-12-14T20:57:37Z

Seeing the same test failure on Windows multiple time in a row now, so probably real:

Failed Tests (1):
  Swift(windows-x86_64) :: diagnostics/educational_notes_serialization.swift

cc @compnerd

compnerd · 2023-12-14T21:03:17Z

It seems to be passing on the swift repository 🤔 CC: @xedin who added this test

swiftlang/swift#70391 is the original introduction

compnerd · 2023-12-14T21:22:21Z

Seems to be failing on macOS: https://ci.swift.org/job/apple-llvm-project-pr-macos/4371/console

neonichu · 2023-12-14T21:53:01Z

Sounds like swiftlang/swift#70470 is supposed to fix it.

neonichu · 2023-12-14T21:53:12Z

@swift-ci test windows

neonichu self-assigned this Dec 11, 2023

neonichu requested review from tomerd and MaxDesiatov as code owners December 11, 2023 21:26

neonichu force-pushed the use-urls-for-plugin-api branch from 0f0d56c to ffdb487 Compare December 12, 2023 19:55

MaxDesiatov mentioned this pull request Dec 12, 2023

Bump version to 5.11 #6995

Closed

neonichu force-pushed the use-urls-for-plugin-api branch from ffdb487 to 0783c4c Compare December 12, 2023 23:03

neonichu commented Dec 12, 2023

View reviewed changes

MaxDesiatov approved these changes Dec 12, 2023

View reviewed changes

neonichu force-pushed the use-urls-for-plugin-api branch from 0783c4c to 7185d6d Compare December 13, 2023 20:38

neonichu merged commit b86d1d4 into main Dec 15, 2023

neonichu deleted the use-urls-for-plugin-api branch December 15, 2023 00:34

Replace custom path type with URL in plugin API #7184

Replace custom path type with URL in plugin API #7184

Conversation

neonichu commented Dec 11, 2023

neonichu commented Dec 11, 2023

neonichu commented Dec 11, 2023

neonichu commented Dec 11, 2023

neonichu commented Dec 12, 2023

neonichu commented Dec 12, 2023

neonichu commented Dec 12, 2023

neonichu Dec 12, 2023

Choose a reason for hiding this comment

neonichu commented Dec 12, 2023

neonichu commented Dec 12, 2023

neonichu commented Dec 12, 2023

neonichu commented Dec 12, 2023

neonichu commented Dec 12, 2023

neonichu commented Dec 13, 2023

MaxDesiatov commented Dec 13, 2023

MaxDesiatov commented Dec 13, 2023

MaxDesiatov commented Dec 13, 2023

grynspan commented Dec 13, 2023

MaxDesiatov commented Dec 13, 2023 • edited Loading

grynspan commented Dec 13, 2023

neonichu commented Dec 13, 2023 • edited Loading

neonichu commented Dec 13, 2023 • edited Loading

neonichu commented Dec 13, 2023

neonichu commented Dec 13, 2023

neonichu commented Dec 13, 2023

neonichu commented Dec 13, 2023

neonichu commented Dec 13, 2023

neonichu commented Dec 13, 2023

neonichu commented Dec 13, 2023

MaxDesiatov commented Dec 14, 2023

neonichu commented Dec 14, 2023

neonichu commented Dec 14, 2023

compnerd commented Dec 14, 2023 • edited Loading

compnerd commented Dec 14, 2023

neonichu commented Dec 14, 2023

neonichu commented Dec 14, 2023

MaxDesiatov commented Dec 13, 2023 •

edited

Loading

neonichu commented Dec 13, 2023 •

edited

Loading

neonichu commented Dec 13, 2023 •

edited

Loading

compnerd commented Dec 14, 2023 •

edited

Loading