Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add XcodeCommandPlugin to fully support Xcode 14 #181

Merged
merged 5 commits into from
Jul 28, 2022

Conversation

calda
Copy link
Member

@calda calda commented Jul 27, 2022

This PR adds an XcodeCommandPlugin to fully support Xcode 14.

Xcode.command.plugin.mov

#175 added support for invoking the SPM CommandPlugin on a Swift Package when opened in Xcode 14.

This didn't support other types of Xcode workspaces, e.g. an iOS app project defined in a .xcodeproj, as @bachand pointed out in #175 (comment). To support that use case, we have to add an XcodeCommandPlugin conformance to our plugin.

I also started running in to the Cannot import "swiftformat" as "SwiftFormat" error that @bachand mentioned, which was blocking me from testing this, so I updated the plugin to use precompiled binary executables instead of compiling SwiftFormat and SwiftLint from source. This significantly reduces how long it takes to run the plugin and simplifies our dependencies, which are all nice wins.

.package(url: "https://github.com/calda/SwiftFormat", exact: "0.49.11-beta-2"),
// The `SwiftLintFramework` target uses "unsafe build flags" so Xcode doesn't
// allow us to reference a specific version number. To work around that,
// we can reference the specific commit for that version.
Copy link
Member Author

@calda calda Jul 28, 2022

Choose a reason for hiding this comment

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

This was actually also blocking consumers from specifying version numbers like from: 1.0.0 for this package, so that was another reason why we needed to move to prebuilt binary executables.

Copy link
Collaborator

@erichoracek erichoracek left a comment

Choose a reason for hiding this comment

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

Nice, LGTM!

// - Unlike SPM targets which are just directories, Xcode targets are
// an arbitrary collection of paths.
let inputTargetNames = Set(argumentExtractor.extractOption(named: "target"))
let inputPaths = context.xcodeProject.targets.lazy
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why make this lazy if we're just converting it to an array below?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd expect lazy here to improve performance a bit for large targets with lots of files by reducing the total number of loop iterations (e.g. not having to do two separate loop passes for the map and then filter). Could be mistaken though.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Got it, makes sense, so we get better data locality since each of the operations for a single element occur in sequence?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I think so

Copy link
Member Author

@calda calda Jul 28, 2022

Choose a reason for hiding this comment

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

Maybe it doesn't really matter / is a micro-optimization, I didn't instrument it of course. I just tend to lean for lazy in cases where a functional chain on a collection has a bunch of individual steps like this.

Comment on lines -11 to -17
.package(url: "https://github.com/calda/SwiftFormat", exact: "0.49.11-beta-2"),
// The `SwiftLintFramework` target uses "unsafe build flags" so Xcode doesn't
// allow us to reference a specific version number. To work around that,
// we can reference the specific commit for that version.
// - This is top-of-tree master from 7/22/22, because the most recent release version
// (0.47.1) seems to have issues in some real-world projects like airbnb/epoxy-ios.
.package(url: "https://github.com/realm/SwiftLint", revision: "c5aa806"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Will this allow us to not have the full source of these two tools and all of the deps in projects that consume this plugin? I was noticing that Epoxy had way more resolved package deps after we added this dep

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes! The only remaining source dependency is Swift Argument Parser, everything else is precompiled.

@calda calda merged commit b53ed2a into master Jul 28, 2022
@calda calda deleted the cal--xcode-command-plugin branch July 28, 2022 15:54
Copy link
Contributor

@bachand bachand left a comment

Choose a reason for hiding this comment

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

The binary dependencies are great!

#if canImport(XcodeProjectPlugin)
extension AirbnbSwiftFormatPlugin: XcodeCommandPlugin {

func performCommand(context: XcodePluginContext, arguments: [String]) throws {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice work @calda ! I see that in our current implementation we are only considering the arguments in the standard plugin implementation.

In Xcode 14 it's possible to pass arguments as well. Can we honor those arguments passed in from Xcode for completeness?

Screen Shot 2022-07-28 at 6 03 11 PM

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it sort of makes sense for the SPM package plugin and Xcode plugin to have different sets of arguments, especially considering the supported functionality is different (e.g. there isn't a feasible implementation of --paths for the Xcode plugin). Also since you have to add the arguments manually each time you invoke the tool in Xcode, I somewhat assume excluding individual files wouldn't be a common use case. It wouldn't hurt to support it though.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see what you're saying @calda . I mostly want to make sure we're intentional.

there isn't a feasible implementation of --paths for the Xcode plugin

Can you elaborate on why this isn't possible? Asking to learn.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking of the difference between SPM targets and Xcode targets -- SPM targets correspond directly to folders, so have a directory path for these input paths to be resolved against. On the other hand Xcode targets are just a collection of files so don't have a directory path. The Xcode project itself does have a directory, though, now that I look at it, so you could resolve paths against that directory.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense. I think it's reasonable to ignore --files when within Xcode.

@calda calda restored the cal--xcode-command-plugin branch August 4, 2022 13:31
Halle added a commit to slumberGroup/swift that referenced this pull request Dec 6, 2022
commit 9f2d60d
Author: 유재호 <[email protected]>
Date:   Mon Nov 28 23:53:34 2022 +0900

    Add missing space after some comment delimiters (airbnb#208)

commit e8412c7
Author: Hanton Yang <[email protected]>
Date:   Fri Nov 18 21:28:12 2022 -0800

    Fix typo in SE-0156 link (airbnb#207)

commit 07bb2e0
Author: Cal Stephens <[email protected]>
Date:   Fri Nov 11 09:21:26 2022 -0800

    Update rule for long function declarations to specify effects (`async`, `throws`) should be written on the line after the closing paren (airbnb#205)

commit 9ea3428
Author: Cal Stephens <[email protected]>
Date:   Fri Nov 4 17:24:56 2022 -0700

    Update `Package.swift` to explicitly not support Linux (airbnb#206)

commit 7884f26
Author: Michael Bachand <[email protected]>
Date:   Thu Oct 20 20:26:36 2022 -0700

    Update language around when an underscore prefix may be appropriate (airbnb#203)

commit 105d7cc
Author: Michael Bachand <[email protected]>
Date:   Thu Oct 20 20:25:27 2022 -0700

    Update README.md (airbnb#204)

commit 831d3c2
Author: 유재호 <[email protected]>
Date:   Tue Oct 18 11:17:12 2022 +0900

    Fix typos in rule 'prefer opaque generic parameter syntax' (airbnb#202)

commit fff6fdc
Author: Cal Stephens <[email protected]>
Date:   Mon Oct 17 16:42:22 2022 -0700

    Bump SwiftFormat version (airbnb#201)

commit f600ed0
Author: Cal Stephens <[email protected]>
Date:   Mon Oct 17 11:04:28 2022 -0700

    Add rule to prefer opaque generic parameter syntax (airbnb#193)

commit f7f2ff9
Author: 유재호 <[email protected]>
Date:   Tue Oct 18 02:13:17 2022 +0900

    Fix naming exception example(underscore prefix) and typos (airbnb#200)

commit f5ae3d7
Author: Cal Stephens <[email protected]>
Date:   Fri Oct 14 11:18:27 2022 -0700

    Add rule to prefer simplified generic extension syntax (airbnb#194)

commit 00216e3
Author: Cal Stephens <[email protected]>
Date:   Thu Oct 13 16:49:19 2022 -0700

    Add rule to prefer shorthand `if let` optional unwrapping syntax (airbnb#192)

commit 860f0d5
Author: Cal Stephens <[email protected]>
Date:   Tue Oct 11 14:39:32 2022 -0700

    Add rule to prefer trailing closure syntax for closure arguments with no parameter name (airbnb#199)

commit eff3f23
Author: 유재호 <[email protected]>
Date:   Tue Aug 23 01:15:12 2022 +0900

    Fix typos in README.md (airbnb#191)

commit f602b12
Author: Cal Stephens <[email protected]>
Date:   Tue Aug 16 16:52:56 2022 -0500

    Update to SwiftFormat build based on 0.49.16 (airbnb#190)

commit d1b1bda
Author: 유재호 <[email protected]>
Date:   Mon Aug 15 22:55:29 2022 +0900

    Fix typo and broken type annotations (airbnb#189)

commit cec2928
Author: Cal Stephens <[email protected]>
Date:   Fri Jul 29 18:30:21 2022 -0700

    Improve handling of `--swift-format` and `--exclude` arguments (airbnb#182)

commit 1bf9613
Author: Cal Stephens <[email protected]>
Date:   Thu Jul 28 09:21:53 2022 -0700

    Infer Swift version from Package.swift swift-tools-version (airbnb#180)

commit b53ed2a
Author: Cal Stephens <[email protected]>
Date:   Thu Jul 28 08:54:24 2022 -0700

    Add `XcodeCommandPlugin` to fully support Xcode 14  (airbnb#181)

commit 30e8f68
Author: Michael Bachand <[email protected]>
Date:   Wed Jul 27 09:35:21 2022 -0700

    Remove executable as product (airbnb#179)

commit 1d8bb91
Author: Cal Stephens <[email protected]>
Date:   Mon Jul 25 15:23:08 2022 -0700

    Add Swift Package Index badges (airbnb#178)

commit 7ecef5d
Author: Cal Stephens <[email protected]>
Date:   Mon Jul 25 13:39:16 2022 -0700

    Add info about Swift Package Manager command plugin to README.md (airbnb#176)

commit 5542090
Author: Jay Hickey <[email protected]>
Date:   Sat Jul 23 13:56:43 2022 -0700

    Add `--paths` argument to only lint specific paths (airbnb#177)

commit 503c5bc
Author: Cal Stephens <[email protected]>
Date:   Fri Jul 22 14:17:09 2022 -0700

    Support target selection dialog in Xcode 14 (airbnb#175)

commit 7079bc3
Author: Cal Stephens <[email protected]>
Date:   Fri Jul 22 14:06:02 2022 -0700

    Improve error handling, add support for `--exclude` flag (airbnb#173)

commit 852a2e3
Author: Cal Stephens <[email protected]>
Date:   Fri Jul 22 11:46:12 2022 -0700

    Bump to latest commit of SwiftLint (airbnb#174)

commit 20290d8
Author: Michael Bachand <[email protected]>
Date:   Thu Jul 21 13:46:20 2022 -0700

    Aesthetics for new Swift command plugin (airbnb#172)

    * restrucutre

    * reorganize again

    * one more change

    * update ci

    * Update Package.swift

    * Update main.yml

commit 640b1ee
Author: Cal Stephens <[email protected]>
Date:   Thu Jul 21 08:20:35 2022 -0700

    Update Swift version to 5.6 (airbnb#169)

commit fb658f4
Author: Cal Stephens <[email protected]>
Date:   Wed Jul 20 14:23:23 2022 -0700

    Update xcode_settings.sh script to enable Xcode spell check (airbnb#171)

commit b844956
Author: Cal Stephens <[email protected]>
Date:   Tue Jul 19 19:14:56 2022 -0700

    Add Swift Package Manager command plugin (airbnb#168)

commit c60d1f6
Author: Cal Stephens <[email protected]>
Date:   Tue Jul 19 11:59:19 2022 -0700

    Include a single space in a set of empty braces (`{ }`) (airbnb#167)

commit d53c95b
Author: Cal Stephens <[email protected]>
Date:   Tue Jul 19 11:58:54 2022 -0700

    Use commas instead of `&&` operators in conditional statements (airbnb#166)

commit 15309c9
Author: Cal Stephens <[email protected]>
Date:   Tue Jul 5 10:02:40 2022 -0700

    Add rule to remove blank lines from the top and bottom of scopes (excluding type bodies) (airbnb#164)

commit a310253
Author: Cal Stephens <[email protected]>
Date:   Sat Jul 2 19:55:48 2022 -0700

    Add `actor` to `--organizetypes` option (airbnb#165)

commit ea85cf2
Author: Michael Bachand <[email protected]>
Date:   Wed Jun 8 15:00:29 2022 -0700

    Fix href for #whitespace-around-comment-delimiters (airbnb#163)

commit 5034513
Author: Cal Stephens <[email protected]>
Date:   Tue Jun 7 12:11:13 2022 -0700

    Add rule that there should be no spaces inside brackets of collection literals (airbnb#162)

commit 9785e35
Author: Cal Stephens <[email protected]>
Date:   Tue Jun 7 12:11:03 2022 -0700

    Add rule that there should be no spaces before or inside the parentheses of function calls / declarations (airbnb#161)

commit 904b72d
Author: Cal Stephens <[email protected]>
Date:   Tue Jun 7 12:10:52 2022 -0700

    Add rule to include spaces before and after comment delimiters (airbnb#160)
@calda calda deleted the cal--xcode-command-plugin branch December 10, 2022 23:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants