Bring back the implementation of SE-0301 #5802

neonichu · 2022-10-08T00:47:14Z

This is basically bringing back @owenv's work from https://github.com/owenv/swift-package-editor into SwiftPM itself. Previously, we didn't have a way to use swift-syntax from the toolchain, but with the new SwiftParser library, it has now become independent of the toolchain and that problem has been solved.

I talked to @owenv, updated his code/tests for the newer APIs in swift-syntax and SwiftPM. Right now, I have not brought over the integration tests, yet, only the unit tests. I will also have to do some updates to support newer manifests versions, e.g. by adding a way to use registry dependencies.

In contrast to the original proposal, I'm keeping this in a separate top-level command called swift package-editor. This is up for discussion, but I think it would be good to start moduralizing the CLI a bit more to avoid having to build every dependency and module in the first CMake stage. That would require taking aprt the Commands module eventually, for now I am just opting to not add more functionality into that since it just makes everything very monolithic.

neonichu · 2022-10-08T00:47:25Z

@swift-ci please smoke test

neonichu · 2022-10-08T01:04:08Z

Looks like there's an issue with compiling with 5.6 again here.

owenv

Thanks for bringing this up-to-date! I left some comments below regarding a few parts which could be modernized further. Also, do you mind adding a Co-authored-by: Owen Voorhees <[email protected]> entry to the commit?

owenv · 2022-10-10T01:13:08Z

Sources/PackageSyntax/Formatting.swift

@@ -0,0 +1,210 @@
+/*


Some of this file could be folded into SwiftSyntax's new SwiftBasicFormat module, although there are some assumptions here which are more likely to hold in a manifest compared to arbitrary swift sources

owenv · 2022-10-10T01:14:07Z

Sources/PackageSyntax/ManifestRewriter.swift

+    public init(_ manifest: String, diagnosticsEngine: DiagnosticsEngine) throws {
+        self.originalManifest = manifest
+        self.diagnosticsEngine = diagnosticsEngine
+        self.editedSource = try Parser.parse(source: manifest)


I believe this method no longer throws

owenv · 2022-10-10T01:19:00Z

Sources/PackageSyntax/ManifestRewriter.swift

+                                                                name: TokenSyntax.identifier("product"),
+                                                                declNameArguments: nil)
+
+        let argumentList = TupleExprElementListSyntax([


This and a few other constructions could be simplified a lot by taking advantage of swiftlang/swift-syntax#848

owenv · 2022-10-10T01:32:00Z

Tests/PackageSyntaxTests/InMemoryGitRepository.swift

@@ -0,0 +1,482 @@
+/*


This is a pared-down version of what's in SPMTestSupport, it should be possible to remove it entirely

owenv · 2022-10-10T01:33:50Z

Tests/PackageSyntaxTests/Resources.swift

@@ -0,0 +1,67 @@
+/*


Most of this can likely be replaced by existing utilities in SPMTestSupport too

owenv · 2022-10-10T01:40:35Z

Package.swift

@@ -385,6 +396,12 @@ let package = Package(
            dependencies: ["Commands"],
            exclude: ["CMakeLists.txt"]
        ),
+        .executableTarget(


PackageSyntax links enough of SwiftPM that I expect this will add 20-30mb to the toolchain size. It would be great if we could start dynamically linking some of the core libraries

@owenv

This is basically bringing back @owenv's work from https://github.com/owenv/swift-package-editor into SwiftPM itself. Previously, we didn't have a way to use swift-syntax from the toolchain, but with the new `SwiftParser` library, it has now become independent of the toolchain and that problem has been solved. I talked to @owenv, updated his code/tests for the newer APIs in swift-syntax and SwiftPM. Right now, I have not brought over the integration tests, yet, only the unit tests. I will also have to do some updates to support newer manifests versions, e.g. by adding a way to use registry dependencies. In contrast to the original proposal, I'm keeping this in a separate top-level command called `swift package-editor`. This is up for discussion, but I think it would be good to start moduralizing the CI a bit more to avoid having to build every dependency and module in the first CMake stage. That would require taking aprt the `Commands` module eventually, for now I am just opting to not add more functionality into that since it just makes everything very monolithic. Co-authored-by: Owen Voorhees <[email protected]>

neonichu · 2022-10-10T17:07:29Z

Ah nice, I didn't know about Co-authored-by, added now!

tomerd · 2022-10-12T01:54:37Z

Package.swift

@@ -586,6 +607,7 @@ if ProcessInfo.processInfo.environment["SWIFTCI_USE_LOCAL_DEPS"] == nil {
        .package(url: "https://github.com/apple/swift-crypto.git", .upToNextMinor(from: minimumCryptoVersion)),
        .package(url: "https://github.com/apple/swift-system.git", .upToNextMinor(from: "1.1.1")),
        .package(url: "https://github.com/apple/swift-collections.git", .upToNextMinor(from: "1.0.1")),
+        .package(url: "https://github.com/apple/swift-syntax.git", .branch(relatedDependenciesBranch)),


can we sort these alphabetically?

tomerd · 2022-10-12T01:55:41Z

Sources/PackageSyntax/Formatting.swift

+import SwiftSyntax
+
+extension ArrayExprSyntax {
+    public func withAdditionalElementExpr(_ expr: ExprSyntax) -> ArrayExprSyntax {


lets audit what must be public and what can remain internal / private in order to control the public API surface area

tomerd · 2022-10-12T01:56:19Z

Sources/PackageSyntax/Formatting.swift

+    public var lineUnitIndent: Trivia {
+        if lineIndent.allSatisfy(\.isSpaces) {
+            let addedSpaces = lineIndent.reduce(0, {
+                guard case .spaces(let count) = $1 else { fatalError() }


remove use of fatalError, force unwraps, etc

tomerd · 2022-10-12T01:57:31Z

Sources/PackageSyntax/ManifestRewriter.swift

+            branchAndRevisionConvenienceMethodsSupported: branchAndRevisionConvenienceMethodsSupported
+        ).visit(packageDependencies).root
+
+        self.editedSource = newManifest.as(SourceFileSyntax.self)!


remove use of fatalError, force unwraps, etc

tomerd · 2022-10-12T01:57:51Z

Sources/PackageSyntax/ManifestRewriter.swift

+            let packageDependenciesFinder = ArrayExprArgumentFinder(expectedLabel: "dependencies")
+            packageDependenciesFinder.walk(argListWithDependencies)
+            guard case .found(let newPackageDependencies) = packageDependenciesFinder.result else {
+                fatalError("Could not find just inserted dependencies array")


remove use of fatalError, force unwraps, etc

tomerd · 2022-10-12T01:58:28Z

Sources/PackageSyntax/ManifestRewriter.swift

+            packageDependencies = newPackageDependencies
+        case .incompatibleExpr:
+            diagnosticsEngine.emit(.incompatibleArgument(name: "targets"))
+            throw Diagnostics.fatalError


should this throw instead of editing diagnostics then throwing generic diagnostics error?

tomerd · 2022-10-12T01:58:56Z

Sources/PackageSyntax/ManifestRewriter.swift

+
+        // Add the target dependency entry.
+        let newManifest = targetDependencies.withAdditionalElementExpr(ExprSyntax(callExpr)).root
+        self.editedSource = newManifest.as(SourceFileSyntax.self)!


remove use of fatalError, force unwraps, etc

tomerd · 2022-10-12T01:59:09Z

Sources/PackageSyntax/ManifestRewriter.swift

+
+        guard case .found(let targetDependencies) = targetDependencyFinder.result else {
+            diagnosticsEngine.emit(.missingArgument(name: "dependencies", parent: "target '\(target)'"))
+            throw Diagnostics.fatalError


tomerd · 2022-10-12T01:59:14Z

Sources/PackageSyntax/ManifestRewriter.swift

+        targetFinder.walk(targetsArrayExpr)
+        guard let targetNode = targetFinder.foundEntity else {
+            diagnosticsEngine.emit(.missingTarget(name: target))
+            throw Diagnostics.fatalError


tomerd · 2022-10-12T01:59:31Z

Sources/PackageSyntax/ManifestRewriter.swift

+        targetsArrayFinder.walk(initFnExpr.argumentList)
+        guard case .found(let targetsArrayExpr) = targetsArrayFinder.result else {
+            diagnosticsEngine.emit(.missingPackageInitArgument(name: "targets"))
+            throw Diagnostics.fatalError


should this throw instead of editing diagnostics then throwing generic diagnostics error?

tomerd · 2022-10-12T02:00:23Z

Sources/PackageSyntax/ManifestRewriter.swift

+        if Foundation.URL(string: urlOrPath)?.scheme == nil {
+            guard checksum == nil else {
+                diagnosticsEngine.emit(.unexpectedChecksumForBinaryTarget(path: urlOrPath))
+                throw Diagnostics.fatalError


tomerd · 2022-10-12T02:00:36Z

Sources/PackageSyntax/ManifestRewriter.swift

+            let productsFinder = ArrayExprArgumentFinder(expectedLabel: "products")
+            productsFinder.walk(argListWithProducts)
+            guard case .found(let newProducts) = productsFinder.result else {
+                fatalError("Could not find just inserted products array")


tomerd · 2022-10-12T02:00:38Z

Sources/PackageSyntax/ManifestRewriter.swift

+            productsNode = newProducts
+        case .incompatibleExpr:
+            diagnosticsEngine.emit(.incompatibleArgument(name: "products"))
+            throw Diagnostics.fatalError


tomerd · 2022-10-12T02:00:54Z

Sources/PackageSyntax/ManifestRewriter.swift

+        productsArrayFinder.walk(initFnExpr.argumentList)
+        guard case .found(let productsArrayExpr) = productsArrayFinder.result else {
+            diagnosticsEngine.emit(.missingPackageInitArgument(name: "products"))
+            throw Diagnostics.fatalError


tomerd · 2022-10-12T02:00:57Z

Sources/PackageSyntax/ManifestRewriter.swift

+        productFinder.walk(productsArrayExpr)
+        guard let productNode = productFinder.foundEntity else {
+            diagnosticsEngine.emit(.missingProduct(name: product))
+            throw Diagnostics.fatalError


tomerd · 2022-10-12T02:01:00Z

Sources/PackageSyntax/ManifestRewriter.swift

+
+        guard case .found(let productTargets) = productTargetsFinder.result else {
+            diagnosticsEngine.emit(.missingArgument(name: "targets", parent: "product '\(product)'"))
+            throw Diagnostics.fatalError


tomerd · 2022-10-12T02:01:21Z

Sources/PackageSyntax/ManifestRewriter.swift

+            throw Diagnostics.fatalError
+        case .missing:
+            diagnosticsEngine.emit(.missingPackageInit)
+            throw Diagnostics.fatalError


tomerd · 2022-10-12T02:01:22Z

Sources/PackageSyntax/ManifestRewriter.swift

+            return initFnExpr
+        case .foundMultiple:
+            diagnosticsEngine.emit(.multiplePackageInits)
+            throw Diagnostics.fatalError


tomerd · 2022-10-12T02:01:24Z

Sources/PackageSyntax/ManifestRewriter.swift

+            targetsNode = newTargets
+        case .incompatibleExpr:
+            diagnosticsEngine.emit(.incompatibleArgument(name: "targets"))
+            throw Diagnostics.fatalError


tomerd · 2022-10-12T02:01:32Z

Sources/PackageSyntax/ManifestRewriter.swift

+            let targetsFinder = ArrayExprArgumentFinder(expectedLabel: "targets")
+            targetsFinder.walk(argListWithTargets)
+            guard case .found(let newTargets) = targetsFinder.result else {
+                fatalError("Could not find just-inserted targets array")


tomerd · 2022-10-12T02:04:06Z

Sources/swift-package-editor/SwiftPackageEditorTool.swift

+
+        do {
+            try editorTool.packageEditor.addPackageDependency(url: dependencyURL, requirement: requirement)
+        } catch Diagnostics.fatalError {


I think we want to get away from Diagnostics.fatalError, its still in place for partial manifest loading but in this case it seems like we can just throw at the call site instead of producing diagnostics then throwing Diagnostics.fatalError

tomerd · 2022-10-12T02:04:31Z

Sources/swift-package-editor/SwiftPackageEditorTool.swift

+        do {
+            let mapping = try createProductPackageNameMapping(packageGraph: editorTool.loadPackageGraph())
+            try editorTool.packageEditor.addTarget(newTarget, productPackageNameMapping: mapping)
+        } catch Diagnostics.fatalError {


tomerd · 2022-10-12T02:20:25Z

Sources/PackageSyntax/PackageEditor.swift

+    let repositoryManager: RepositoryManager
+
+    /// The file system in use.
+    let fs: FileSystem


rename fs -> fileSystem (everywhere)

tomerd · 2022-10-12T02:21:06Z

Sources/PackageSyntax/PackageEditor.swift

+}
+
+/// The global context for package editor.
+public final class PackageEditorContext {


audit use of class, we should prefer structs unless we need a reference type

tomerd · 2022-10-12T02:21:51Z

Sources/PackageSyntax/PackageEditor.swift

+    public init(manifestPath: AbsolutePath,
+                repositoryManager: RepositoryManager,
+                toolchain: UserToolchain,
+                diagnosticsEngine: DiagnosticsEngine,


DiagnosticsEngine 😱

feels like this initializer should be simplified after the changes we done to the workspace initializers

tomerd · 2022-10-12T02:23:37Z

Sources/PackageSyntax/PackageEditor.swift

+                repositoryManager: RepositoryManager,
+                toolchain: UserToolchain,
+                diagnosticsEngine: DiagnosticsEngine,
+                fs: FileSystem = localFileSystem) throws {


do not default to localFileSystem. you can create an extension with a simper signatures (ie defaults) for testing if that is why we are defaulting

tomerd · 2022-10-12T02:24:27Z

Sources/PackageSyntax/PackageEditor.swift

+        self.diagnosticsEngine = diagnosticsEngine
+        self.fs = fs
+
+        self.manifestLoader = ManifestLoader(toolchain: toolchain)


probably use the same one we use for the workspace. see comment above about improving this initializer

tomerd · 2022-10-12T02:24:39Z

Sources/PackageSyntax/PackageEditor.swift

+            try withTemporaryDirectory {
+                let path = $0
+                try localFileSystem.writeFileContents(path.appending(component: "Package.swift"),
+                                                      bytes: ByteString(encodingAsUTF8: contents))


no need to use ByteString

tomerd · 2022-10-12T02:24:49Z

Sources/PackageSyntax/PackageEditor.swift

+                let path = $0
+                try localFileSystem.writeFileContents(path.appending(component: "Package.swift"),
+                                                      bytes: ByteString(encodingAsUTF8: contents))
+                _ = try loadManifest(at: path, fs: localFileSystem)


localFileSystem

tomerd · 2022-10-12T02:25:14Z

Sources/PackageSyntax/PackageEditor.swift

+    }
+
+    /// Load the manifest at the given path.
+    func loadManifest(


use workspace APIs?

tomerd · 2022-10-12T02:25:32Z

Sources/swift-package-editor/SwiftPackageEditorTool.swift

+    private var cachedPackageGraph: PackageGraph?
+
+    init() throws {
+        diagnostics = DiagnosticsEngine(handlers: [print(diagnostic:)])


tomerd · 2022-10-12T02:25:59Z

Sources/swift-package-editor/SwiftPackageEditorTool.swift

+        if let cachedPackageGraph = cachedPackageGraph {
+            return cachedPackageGraph
+        }
+        let workspace = try Workspace.init(forRootPackage: packageRoot, customManifestLoader: nil)


adjust to new APIs

tomerd · 2022-10-12T02:26:21Z

Sources/swift-package-editor/SwiftPackageEditorTool.swift

+            return cachedPackageGraph
+        }
+        let workspace = try Workspace.init(forRootPackage: packageRoot, customManifestLoader: nil)
+        let observability = ObservabilitySystem { _, _ in }


use the one we use in other tools

tomerd · 2022-10-12T02:27:14Z

Sources/swift-package-editor/SwiftPackageEditorTool.swift

+
+    func loadPackageGraph() throws -> PackageGraph {
+        if let cachedPackageGraph = cachedPackageGraph {
+            return cachedPackageGraph


is this safe to cache?

tomerd · 2022-10-12T02:28:03Z

Sources/swift-package-editor/SwiftPackageEditorTool.swift

+        let repositoryManager = RepositoryManager(fileSystem: localFileSystem,
+                                                  path: packageRoot.appending(component: ".build"),
+                                                  provider: GitRepositoryProvider(),
+                                                  initializationWarningHandler: { _ in })


feels like this setup sequence could / should be refactored to lean more on code from other tools / SwiftTool. it glosses over some details that may cause issues

tomerd · 2022-10-12T02:30:07Z

Sources/swift-package-editor/SwiftPackageEditorTool.swift

+    public init?(argument: String) {
+        self.init(argument)
+    }
+}


do these need to be public? they will leak if so

tomerd · 2022-10-12T02:30:44Z

Sources/swift-package-editor/SwiftPackageEditorTool.swift

+    }
+}
+
+func print(diagnostic: TSCBasic.Diagnostic) {


seems redundant, switch to Observability system instead of roll your own

tomerd · 2022-10-12T02:31:30Z

Tests/PackageSyntaxTests/TestSupport.swift

+    }
+}
+
+func XCTAssertThrows<T: Swift.Error>(


XCTest already has something similar iirc?

tomerd · 2022-10-12T02:32:01Z

Tests/PackageSyntaxTests/TestSupport.swift

+}
+
+extension FileSystem {
+    func createEmptyFiles(at root: AbsolutePath, files: String...) {


FileSystem already has similar API iirc?

tomerd · 2022-10-12T02:32:33Z

Tests/PackageSyntaxTests/Resources.swift

+}
+#endif
+
+public class Resources {


does this need to be public

tomerd · 2022-10-12T02:35:43Z

this is pretty awesome @neonichu @owenv, very happy to see this make a come back ❤️ 🙇

did a first cut and added comment mostly around moving to the new APIs and making code safe / less fragile.

the code that handles dependencies is a bit lacking in correctness afaict, we need to find a way to consolidate this with the already existing logic in the manifest parser itself. maybe there is a way to abstract some things out so its reusable in both places

did not review any of the parsing logic, I think this a job better suited to @WowbaggersLiquidLunch :D

neonichu · 2022-12-08T18:01:06Z

I think we have all the pieces in place now to be able to build a command without building it during bootstrap.

Are we OK with this being swift package-editor, contrary to the current proposal? If not, I guess we could have swift-package shell out to the other command for now? Eventually, once we can bootstrap on Windows, we could move the implementation over to swift-package, but right now we have to avoid it because we need to retain the ability of building swift-package using CMake.

cc @tomerd

aaronjedwards · 2023-09-10T05:42:19Z

Curious, is there a proposed plan for getting this merged? @tomerd @neonichu

I was reviewing the updated swift.org website and really enjoyed the new layout and walkthroughs to get people up and running with swift. When scrolling through the CLI tutorial, I immediately thought of this proposal when I read the "Adding dependencies" section and didn't see mention of the convenient commands included in this proposal. I know I was excited when this proposal was accepted, as Im sure many others were and would love to be able to make the process of package manifest editing easier and more approachable.

I know there hasn't been much in this PR lately, but if available, any update would be great!

neonichu · 2023-11-11T00:24:55Z

can be closed

Diggory · 2023-11-30T12:16:46Z

Did this feature never make it?

MaxDesiatov · 2023-11-30T12:48:46Z

IIUC there are no people currently working on the implementation.

neonichu self-assigned this Oct 8, 2022

neonichu requested a review from owenv October 8, 2022 01:06

owenv reviewed Oct 10, 2022

View reviewed changes

neonichu force-pushed the se-0301 branch from adb2507 to 71a811c Compare October 10, 2022 17:06

tomerd reviewed Oct 12, 2022

View reviewed changes

Sources/PackageSyntax/PackageEditor.swift

}

/// Load the manifest at the given path.

func loadManifest(

Copy link

Contributor

tomerd Oct 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use workspace APIs?

tomerd reviewed Oct 12, 2022

View reviewed changes

Tests/PackageSyntaxTests/Resources.swift

}

#endif

public class Resources {

Copy link

Contributor

tomerd Oct 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this need to be public

tomerd added the WIP Work in progress label Oct 17, 2022

neonichu mentioned this pull request Apr 13, 2023

[SE-0301] Package Editor Commands Implementation #3034

Closed

stevapple mentioned this pull request Jun 15, 2023

[WIP] Add Support for Swift Package Manager dependabot/dependabot-core#7344

Closed

neonichu closed this Nov 11, 2023

MaxDesiatov reopened this Jan 11, 2024

MaxDesiatov closed this Jan 11, 2024

Bring back the implementation of SE-0301 #5802

Bring back the implementation of SE-0301 #5802

Uh oh!

Conversation

neonichu commented Oct 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

neonichu commented Oct 8, 2022

Uh oh!

neonichu commented Oct 8, 2022

Uh oh!

owenv left a comment

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!

neonichu commented Oct 10, 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!

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!

tomerd Oct 12, 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!

neonichu commented Oct 8, 2022 •

edited

Loading

tomerd Oct 12, 2022 •

edited

Loading

tomerd Oct 12, 2022 •

edited

Loading