SE-0318 Package creation #3514

miggs597 · 2021-05-26T23:32:18Z

swift package create <packageName> Makes new packages while also handling the ceremony of making the package's directory first.
Defaults to executable package

swift package add-template <url> Facilitates adding new templates from git

Templates are generic Swift package directories that are transformed when making a new packages based off of a template

Directory structure is simplified from:

OldDirStruct % tree
.
OldDirStruct % tree
.
├── Package.swift
├── README.md
├── Sources
│   └── OldDirStruct
│       └── OldDirStruct.swift
└── Tests
    └── OldDirStructTests
        └── OldDirStructTests.swift

To:

.
├── Package.swift
├── README.md
└── Sources
    └── NewDirStruct.swift

tomerd · 2021-05-27T04:45:27Z

Sources/Commands/SwiftPackageTool.swift

+                throw InternalError("Could not get the current working directory")
+            }
+
+            if localFileSystem.exists(cwd.appending(RelativePath(url))) {


when passing in a path it should be an absolute path, not relative to the working directory.

tomerd · 2021-05-27T04:46:19Z

Sources/Commands/SwiftPackageTool.swift

+
+                try localFileSystem.copy(from: currentTemplateLocation, to: templateDirectory)
+            } else {
+                let templatePath = configPath.appending(components: "templates", "new-package", String(url.split(separator: "/").last!))


url parsing in fragile

Foundation has URLComponents for this.

tomerd · 2021-05-27T04:47:37Z

Sources/Commands/SwiftPackageTool.swift

+    }
+}
+
+internal var useNewBehaviour: Mode {


useNewBehaviour -> createPackageMode

this var should be be in an extension to InitPackage rather than "free floating" to better encapsulate the logic

tomerd · 2021-05-27T04:47:51Z

Sources/Commands/SwiftPackageTool.swift

+    }
+}
+
+internal enum Mode {


Mode -> CreatePackageMode

this function should be be in an extension to InitPackage rather than "free floating" to better encapsulate the logic

tomerd · 2021-05-27T04:48:15Z

Sources/Commands/SwiftPackageTool.swift

@@ -966,3 +1051,207 @@ fileprivate func logPackageChanges(changes: [(PackageReference, Workspace.Packag
    }
    stream.flush()
 }
+
+internal struct PackageTemplate {


is this type still required? iirc this was for JSON parsing

tomerd · 2021-05-27T04:49:51Z

Sources/Commands/SwiftPackageTool.swift

+    case legacy
+}
+
+internal func getSwiftPMDefaultTemplate(type: InitPackage.PackageType,


this function should be be in an extension to InitPackage rather than "free floating" to better encapsulate the logic

tomerd · 2021-05-27T04:49:56Z

Sources/Commands/SwiftPackageTool.swift

+    }
+}
+
+fileprivate func makePackage(fileSystem: FileSystem,


this function should be be in an extension to InitPackage rather than "free floating" to better encapsulate the logic

tomerd · 2021-05-27T04:50:05Z

Sources/Commands/SwiftPackageTool.swift

+    try initPackage.writePackageStructure()
+}
+
+fileprivate func copyTemplate(fileSystem: FileSystem, sourcePath: AbsolutePath, destinationPath: AbsolutePath, name: String) throws {


this function should be be in an extension to InitPackage rather than "free floating" to better encapsulate the logic

tomerd · 2021-05-27T04:50:24Z

Sources/Commands/SwiftPackageTool.swift

+    }
+}
+
+fileprivate enum MakePackageMode {


this enum should be be in an extension to InitPackage rather than "free floating" to better encapsulate the logic

tomerd · 2021-05-27T04:52:26Z

Sources/Commands/SwiftPackageTool.swift

+internal func getSwiftPMDefaultTemplate(type: InitPackage.PackageType,
+                                        sources: RelativePath = .init("./Sources"),
+                                        tests: RelativePath? = nil,
+                                        createSubDirectoryForModule: Bool = false) -> PackageTemplate {


this can now return InitPackage.PackageTemplate since we dont need the other type now that we are not parsing JSON

tomerd · 2021-05-27T04:53:42Z

Sources/Commands/SwiftPackageTool.swift

+    let templateHomeDirectory = configPath.appending(components: "templates", "new-package")
+
+    var foundTemplate = false
+    var templateToUse = ""


make templateToUse optional and then you can get rid of foundTemplate

tomerd · 2021-05-27T04:54:23Z

Sources/Commands/SwiftPackageTool.swift

+        name = packageName ?? cwd.basename
+        destinationPath = cwd
+    case .create:
+        name = packageName!


not safe! you should guard to make sure its set

tomerd · 2021-05-27T04:55:40Z

Sources/Commands/SwiftPackageTool.swift

+            print("Copying \($0)")
+            try copyTemplate(fileSystem: fileSystem, sourcePath: templateHomeDirectory.appending(components: templateToUse, $0), destinationPath: destinationPath, name: name)
+        }
+        return


if returning here, then the else branch is not required and you can move the let packageTemplate down

tomerd · 2021-05-27T05:07:05Z

Sources/Commands/SwiftPackageTool.swift

+        print(message)
+    }
+    try initPackage.writePackageStructure()
+}


if I read this correctly, I dont think this follow the logic defined in the evolution proposal.

I suggest restructuring the code in the following way:

separate makePackage into 2 functions, one for the "legacy behavior" (e.g. makePackageLegacy) and one for the "new" one (e.g. makePackageNew).

from the Init command, call makePackageLegacy when mode == .legacy and makePackageNew when mode == .new

from the Create command, we can always call makePackageNew since its only available when mode == .new

initPackage.writePackageStructure should wither take a mode argument, or also be split into legacy/new since per the design its supposed to work a bit differently in legacy vs. new mode (e.g. not create certain files)

tomerd · 2021-05-27T05:11:01Z

Sources/Commands/SwiftPackageTool.swift

+    // Recursively copy the template package
+    // Currently only replaces the string literal "$NAME"
+    if fileSystem.isDirectory(sourcePath) {
+        if let fileName = sourcePath.pathString.split(separator: "/").last {


is this fileName or directory name?

tomerd · 2021-05-27T05:13:22Z

Sources/Commands/SwiftPackageTool.swift

+            renamed = renamed.replacingOccurrences(of: "___NAME_AS_C99___", with: name.spm_mangledToC99ExtendedIdentifier())
+
+            if let fileName = sourcePath.pathString.split(separator: "/").last {
+                if !fileSystem.exists(destinationPath.appending(component: String(fileName))) {


seems like you can move this condition up so not to do anything if the file already exists

tomerd · 2021-05-27T05:14:11Z

Sources/Workspace/InitPackage.swift

@@ -241,7 +274,7 @@ public final class InitPackage {
                # \(pkgname)

                A description of this package.
-
+                


remove redundant space

tomerd · 2021-05-27T05:16:07Z

Tests/WorkspaceTests/InitTests.swift

@@ -13,6 +13,7 @@ import SPMTestSupport
 import TSCBasic


ideally none of these test have to change

tomerd · 2021-05-27T05:16:49Z

Tests/CommandsTests/PackageToolTests.swift

+
+    func testCreateNoNameArgument() throws {
+        try XCTSkipIf(useNewBehaviour == .legacy)
+        fixture(name: "Miscellaneous") { prefix in


is the fixture needed here?

tomerd · 2021-05-27T05:18:54Z

Package.swift

@@ -273,7 +273,7 @@ let package = Package(
            dependencies: ["swift-build", "swift-package", "swift-test", "swift-run", "Commands", "Workspace", "SPMTestSupport"]),
        .testTarget(
            name: "WorkspaceTests",
-            dependencies: ["Workspace", "SPMTestSupport"]),
+            dependencies: ["Workspace", "SPMTestSupport", "Commands"]),


this likely points to some leaky abstraction - we should not need to bring Commands into workspace tests

tomerd · 2021-05-27T05:19:12Z

Sources/Commands/SwiftPackageTool.swift

+        if useNewBehaviour == .new {
+            subCommands.insert(Create.self, at: 6)
+            subCommands.insert(AddTemplate.self, at: 7)
+        }


tomerd · 2021-05-27T05:20:52Z

Tests/WorkspaceTests/InitTests.swift

@@ -13,6 +13,7 @@ import SPMTestSupport
 import TSCBasic
 import PackageModel
 import Workspace
+@testable import Commands


see above, this likely point to some leaky abstraction and should not be needed

abertelrud · 2021-05-28T17:29:02Z

Sources/Commands/SwiftPackageTool.swift

+
+    public init() {}
+
+    public static var _errorLabel: String { "error" }


Is this a requirement of swift-argument-parser?

We do need the constructor, but _errorLabel isn't needed.
From the swift-argument-parser documentation _errorLabel is used to insert the label at the beginning of an error message.

/// The label to use for "Error: ..." messages from this type. (experimental) public static var _errorLabel: String { get }

abertelrud · 2021-05-28T17:31:06Z

Sources/Commands/SwiftPackageTool.swift

+        ]
+
+        if InitPackage.createPackageMode == .new {
+            subCommands.insert(Create.self, at: 6)


Would it make sense to derive these indices from looking up Init.self in the array? If not, it would be good to have a comment up in that array, since it's very easy otherwise to add a subcommand without realizing that this hardcoded index has to be updated.

compnerd · 2021-05-30T20:28:42Z

Tests/WorkspaceTests/InitTests.swift

@@ -266,10 +266,15 @@ class InitTests: XCTestCase {
            try localFileSystem.removeFileTree(packageRoot)
            try localFileSystem.createDirectory(packageRoot)

+            let packageTemplate = InitPackage.PackageTemplate(sourcesDirectory: RelativePath("./Sources"),
+                                                              testsDirectory: RelativePath("./Tests"),


Please do not assume that the path separator is /. You should use the join method to generate the relative paths.

Might have hit limitation of json, would be nice to have support for multiline strings

Needed because the default directory structure has been simplified

- Moved to transforming dir - Tests are now skipped depending on flag

- Restored InitTests.swift - add-template is less fragile - extension instead of free floating

testPlatforms() was removed because the refactoring also removes InitPackage.InitPackageOptions() It wasn't possible from a user to access, it's dead code and only used by this test.

- Fixed some typos - add-template is now more robust when determining if we have a path or web url - rough implementation of update-template

-C needed to come first

The funny name is just a folder that is space separated

abertelrud · 2021-07-12T19:03:23Z

@swift-ci please smoke test

abertelrud · 2021-07-12T19:06:45Z

Sources/Commands/SwiftPackageTool.swift

+
+    struct AddTemplate: SwiftCommand {
+        static let configuration = CommandConfiguration(
+            abstract: "Donwloads/moves a template into the default template directory")


Suggested change

abstract: "Donwloads/moves a template into the default template directory")

abstract: "Downloads/moves a template into the default template directory")

abertelrud · 2021-07-12T19:07:15Z

Sources/Commands/SwiftPackageTool.swift

+        @OptionGroup(_hiddenFromHelp: true)
+        var swiftOptions: SwiftToolOptions
+
+        @Argument(help: "URL to template (absolute path to template works too)")


Suggested change

@Argument(help: "URL to template (absolute path to template works too)")

@Argument(help: "URL or absolute path of template")

abertelrud · 2021-08-02T20:15:11Z

Closing because we are exploring a different approach (evolution proposal was sent back).

miggs597 requested review from abertelrud, friedbunny, neonichu and tomerd as code owners May 26, 2021 23:32

miggs597 marked this pull request as draft May 26, 2021 23:32

tomerd reviewed May 27, 2021

View reviewed changes

abertelrud reviewed May 28, 2021

View reviewed changes

compnerd reviewed May 30, 2021

View reviewed changes

Miguel Perez added 22 commits June 30, 2021 22:31

Adding tests

de6860a

Using a template for package creation test

7451487

Returned .gitignore and readme

cdc830e

Ability to add description to README.md

26eca0f

Might have hit limitation of json, would be nice to have support for multiline strings

Can now add dependencies from template

2f730f3

WIP new command + template

623cbab

Updated old tests so they still pass

a408757

Needed because the default directory structure has been simplified

Block against using --type and --template

3f2aeb2

default dependencies

4f867b1

Feature Flag

f2c5b54

- Moved to transforming dir - Tests are now skipped depending on flag

Removed leaky abstraction

90b3ee0

- Restored InitTests.swift - add-template is less fragile - extension instead of free floating

Refactoring

9ce6e16

testPlatforms() was removed because the refactoring also removes InitPackage.InitPackageOptions() It wasn't possible from a user to access, it's dead code and only used by this test.

Updates from feedback

667022a

Removed getSwiftPMDefaultTemplate

01967cc

- Fixed some typos - add-template is now more robust when determining if we have a path or web url - rough implementation of update-template

Validate we have a git repo before updating

ad06f60

Added tests for swift package create

169e4fa

Fixed pull

49c6f95

-C needed to come first

Added tests

97a0566

updates from feedback

f668f6a

More fixes and tests for funny names

ee259bc

The funny name is just a folder that is space separated

Removed whitespace

e1b6cea

Rebased off main and is merge ready

8ee943d

miggs597 force-pushed the createSub branch from 8f1d716 to 8ee943d Compare July 1, 2021 03:11

miggs597 marked this pull request as ready for review July 1, 2021 03:11

abertelrud reviewed Jul 12, 2021

View reviewed changes

elsh marked this pull request as draft July 12, 2021 21:24

abertelrud closed this Aug 2, 2021

		@@ -241,7 +274,7 @@ public final class InitPackage {
		# \(pkgname)

		A description of this package.


		public init() {}

		public static var _errorLabel: String { "error" }

	abstract: "Donwloads/moves a template into the default template directory")
	abstract: "Downloads/moves a template into the default template directory")

	@Argument(help: "URL to template (absolute path to template works too)")
	@Argument(help: "URL or absolute path of template")

SE-0318 Package creation #3514

SE-0318 Package creation #3514

Uh oh!

Conversation

miggs597 commented May 26, 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!

Choose a reason for hiding this comment

Uh oh!

tomerd May 27, 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!

tomerd May 27, 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!

abertelrud commented Jul 12, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abertelrud commented Aug 2, 2021

Uh oh!

Uh oh!

tomerd May 27, 2021 •

edited

Loading

tomerd May 27, 2021 •

edited

Loading