Set up SwiftLint as a standalone build plugin #23996

mokagio · 2025-01-21T09:08:27Z

#23958 removes CocoaPods from the project setup, but with SwiftLint also goes away.

This PR re-introduces SwiftLint, fetching it using the SwiftPM plugin repo but then calling the binary directly via the existing SwiftLint aggregate target (which calls rake lint under the hood.

I considered using the plugin setup, see #23990, but didn't go ahead with it because:

It requires integrating in every target
It does not seem to care about our configuration
It requires extract flags for permissions when running in CI and will prompt the user on first run locally
The setup does not lend itself to having a single source of truth for the SwiftLint version to use, which is a requirement for our CI linter to run—and I strongly advice keeping the CI linter setup as it is
In my few minutes of tinkering with it, Xcode crashed at least 4 times (see example in SwiftLint Build Tool Plugin example #23990 description)
This is a bit of an argument from authority, which is why I'm leaving it last and using it more as a reinforcement of my conclusion than an explanation, but even the original SwiftLint author recommend against using the plugin

For reference, this setup is inspired by SwiftFormat.

I considered fetching SwiftLint directly instead of the plugin, but doing so would have required compiling SwiftLint from source to run it, which was very time consuming.

Finally, it crossed my mind to keep the old CocoaPods setup for SwiftLint. I started going down that path in #23998 but stopped once I noticed that the performance using the setup in this PR were comparable to those on trunk, see #23999

Given that removing CocoaPods whether to build the app or for tooling, brings us closer to our goal of no longer depending on Ruby for product-development tasks, I'd rather stick with this Swift-only approach.

Note

I'm having issues archiving the app via Fastlane, even though ~~vanilla xcodebuild~~ and Xcode work fine. I'm investigating...

error: module 'WordPressSharedObjC' in AST file 
'/Users/gio/Developer/a8c/wpios/DerivedData/ModuleCache.noindex/EQUUY9BHSJ5N/WordPressSharedObjC-5G93B85NZ09I.pcm'
(imported by AST file '/Users/gio/Developer/a8c/wpios/DerivedData/WordPress/Build/Intermediates.noindex/ArchiveIntermediates/WordPress Alpha/PrecompiledHeaders/WordPress-Bridging-Header-swift_1L0UBHDEION2G-clang_EQUUY9BHSJ5N.pch')
is not defined in any loaded module map file;
maybe you need to load /Users/gio/Developer/a8c/wpios/DerivedData/WordPress/Build/Intermediates.noindex/ArchiveIntermediates/WordPress Alpha/IntermediateBuildFilesPath/GeneratedModuleMaps-iphoneos/WordPressSharedObjC.modulemap'?

Apart from being unsure about the error itself, I don't understand is how the changes in this PR might be affecting the build that way...

Updated I haven't fixed the issue but I have a workaround that I like and that I think we should consider in order to merge this PR and then move forward with #23958

The TL;DR is that the error occurs only when building builds with configuration Release or Release-Alpha, so I told the script that runs SwiftLint to only run on Debug builds. We have CI covering us when building a different configuration because it runs the standalone linter. In fact, I also disabled the SwiftLint build phase in CI, to save time because of that.

Regression Notes

Potential unintended areas of impact – N.A.
What I did to test those areas of impact (or what existing automated tests I relied on) – N.A.
What automated tests I added (or what prevented me from doing so) – N.A.

PR submission checklist:

I have completed the Regression Notes.
I have considered adding unit tests for my changes. N.A.
I have considered adding accessibility improvements for my changes. N.A.
I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary. N.A.

UI changes testing checklist: Not a UI PR.

…mage)

…red image)

…matedImageView

…thenticatedImageDownload redirect)

…IF data

This reverts commit 3e41f0c8024c0e871653a52c1240749bbd489787.

wpmobilebot · 2025-01-21T09:18:41Z

📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.

	App Name	Jetpack Alpha
	Configuration	Release-Alpha
	Build Number	pr23996-8479673
	Version	25.6
	Bundle ID	com.jetpack.alpha
	Commit	`8479673`
	App Center Build	jetpack-installable-builds #10420

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

wpmobilebot · 2025-01-21T09:20:51Z

📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.

	App Name	WordPress Alpha
	Configuration	Release-Alpha
	Build Number	pr23996-8479673
	Version	25.6
	Bundle ID	org.wordpress.alpha
	Commit	`8479673`
	App Center Build	WPiOS - One-Offs #11388

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

There's a problem with the Docker image and version 0.58.1: ``` Digest: sha256:0a86399eb15ab7d3e9535f38df6f3f443aad1efde3cba9ff5ee34a3d226386e9 [2025-01-21T09:34:29Z] Status: Downloaded newer image for ghcr.io/realm/swiftlint:0.58.1 [2025-01-21T09:34:31Z] warning: Currently running SwiftLint 0.58.0 but configuration specified version 0.58.1. ``` https://buildkite.com/automattic/wordpress-ios/builds/25472#01948835-83d4-47b2-b177-40ba3bf2d5a8

mokagio · 2025-01-21T21:23:55Z

WordPress/Classes/Extensions/NSAttributedString+Helpers.swift

@@ -33,8 +33,7 @@ public extension NSAttributedString {
            if
                displayAnimatedGifs,
                let animatedImage = image as? AnimatedImage,
-                animatedImage.gifData != nil
-            {
+                animatedImage.gifData != nil {


Moving up from version 0.54.0 brought up this and other opening_brace violations.

Might be related to the rule having been rewritten in version 0.55.0 and then updated in version 0.57.0.

mokagio · 2025-01-21T21:29:02Z

.swiftlint.yml

@@ -1,10 +1,11 @@
-swiftlint_version: 0.54.0
+swiftlint_version: 0.58.2


We cannot use 0.54.0 because it's not available as a built tool plugin, see https://github.com/SimplyDanny/SwiftLintPlugins/releases

Given that, it makes sense to use the latest available version. But even if we didn't want to:

0.58.0 and 0.58.1 have a bug where the Docker image reads the wrong version, so we cannot use them

0.55.0 does not have the --working-directory option which we need

This reverts commit 9da124f. When run via the plugin, SwiftLint consistently fails to fetch the remote configuration. See https://buildkite.com/automattic/wordpress-ios/builds/25484#01948b50-ee3b-4e18-a2a8-1555071c933c

This way, we'll be able to add additional checks without messing up the project file.

BuildTools/Package.swift

kean

This is a bit of an argument from authority, which is why I'm leaving it last and using it more as a reinforcement of my conclusion than an explanation, but realm/SwiftLint#5207 (comment)

I concur. It should be used only when the code requires a tool to compile – mainly code generators.

kean · 2025-01-23T18:33:55Z

BuildTools/.gitignore

@@ -0,0 +1 @@
+.build


I'd add it to the .gitignore file in the root directory.

kean · 2025-01-23T18:41:50Z

BuildTools/Package.swift

+    name: "BuildTools",
+    platforms: [.macOS(.v10_13)],
+    dependencies: [
+        .package(url: "https://github.com/SimplyDanny/SwiftLintPlugins", exact: loadSwiftLintVersion()),


Does the version numbers have to be in .swiftlint.yml? Are there any other roadblocks for simply defining the version here in the package:

.package(url: "https://github.com/SimplyDanny/SwiftLintPlugins", exact: "0.58.2"),

.swiftlint.yml needs to specify the SwiftLint version the configuration expects

https://github.com/Automattic/a8c-ci-toolkit-buildkite-plugin/blob/29064ab1e6e49135f9700d86b7b3279081b9ebc9/bin/run_swiftlint#L23-L27

We don't have to read the version from there here, but I find it nice to have a single source of truth.

We could duplicate the value here, but we might run into some annoying issues if the version defined here diverges from the one in the config so that we end up with CI running a different version than local and giving different feedback.

kean added 30 commits January 2, 2025 16:33

Add convenience init to LightboxViewController

79c49ae

Integrate LightboxViewController in SiteMedia

246b122

Integrate LightboxViewController in ReaderDetailsCoordinator (cover i…

6ed4454

…mage)

INtegrate in DefaultContentCoordinator

95399b0

Integrate LightboxViewController in Guteberg

b3a84ac

Integrate LightboxViewController in ExternalMediaPickerViewController

8fbeb66

Integrate LightboxViewController in PostSettingsViewController (featu…

8f7dd16

…red image)

Remove FeaturedImageViewController (ObjC)

8096de9

Rewrite PostFeaturedImageCell

88c7b13

Integrate LightboxViewController in ReaderCommentsViewController

da7b29a

Update WPRichTextImage to use AsyncImageView

3354d85

Automatically pick thumbnail when available

c3993e0

Remove WPImageViewController

36b392e

Update release notes

342e63e

Remove ImageLoader

d613c05

Remove ImageDimensionParser

2ac4c2e

Update MediaItemHeaderView to use AsyncImageView instead of CachedAni…

5141337

…matedImageView

Fix code formatting in RichTextView

a3adfee

Update AnimatedGifAttachmentViewProvider to use GIFImageView directly

61871d0

Remove SolidColorActivityIndicator

4b3f4ef

Remove CachedAnimatedImageView

79b3a0a

Remove GIFPlaybackStrategy

c96c844

Update EditorMediaUtility to use ImageDownloader directly (without Au…

3f270d2

…thenticatedImageDownload redirect)

Remove AuthenticatedImageDownload

dca2548

Update MediaExternalExporter to use ImageDownloader for downloading G…

f9a2315

…IF data

Remove AnimatedImageCache

8ad55b8

Remove remaining AlamofireImage usages from the anouncement cells

f013a31

Remove AlamofireImageCacheAdapter

f27c41d

Remove AlamofireImage

129bc3e

Add ImagePrefetcher

eece5e9

mokagio added 3 commits January 21, 2025 17:25

Fix opening_braces violation using swiftlint --fix

e647171

Revert "Fix opening_braces violation using swiftlint --fix"

b45f8b7

This reverts commit 3e41f0c8024c0e871653a52c1240749bbd489787.

Set SDK when running SwiftLint to avoid conflicts

5ce2bdc

mokagio changed the base branch from trunk to mokagio/remove-cocoapods-christmast-branch January 21, 2025 09:08

Try with version 0.58.0 which seems a bit faster than 0.58.2

088fed2

Fix SwiftLint open_brace violations via running with --fix

f006bb7

mokagio mentioned this pull request Jan 21, 2025

Remove CocoaPods #23958

Open

14 tasks

mokagio added 2 commits January 21, 2025 20:32

Try version 0.58.1

cfc62e6

mokagio force-pushed the mokagio/swiftlint-via-isolated-plugin branch from 8df602d to f692687 Compare January 21, 2025 20:46

Restore remote configuration

9da124f

mokagio commented Jan 21, 2025

View reviewed changes

mokagio added 3 commits January 22, 2025 11:47

Revert "Restore remote configuration"

05a0116

This reverts commit 9da124f. When run via the plugin, SwiftLint consistently fails to fetch the remote configuration. See https://buildkite.com/automattic/wordpress-ios/builds/25484#01948b50-ee3b-4e18-a2a8-1555071c933c

Wrap SwiftLint call from aggretate target in script

1b5d37d

This way, we'll be able to add additional checks without messing up the project file.

Do not run build-time SwiftLint when building in CI

db78e12

mokagio force-pushed the mokagio/swiftlint-via-isolated-plugin branch from af75c45 to db78e12 Compare January 23, 2025 00:15

Only run SwiftLint build phase in Debug builds

53f4eab

mokagio marked this pull request as ready for review January 23, 2025 00:54

mokagio added the Tooling Build, Release, and Validation Tools label Jan 23, 2025

mokagio added this to the Pending milestone Jan 23, 2025

mokagio requested review from crazytonyli, kean and a team January 23, 2025 00:58

mokagio commented Jan 23, 2025

View reviewed changes

BuildTools/Package.swift Outdated Show resolved Hide resolved

Remove commented out SwiftLint source alternatives

8479673

kean approved these changes Jan 23, 2025

View reviewed changes

mokagio force-pushed the mokagio/remove-cocoapods-christmast-branch branch from 5189e4b to d7d0010 Compare February 5, 2025 05:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set up SwiftLint as a standalone build plugin #23996

Set up SwiftLint as a standalone build plugin #23996

mokagio commented Jan 21, 2025 •

edited

Loading

wpmobilebot commented Jan 21, 2025 •

edited

Loading

wpmobilebot commented Jan 21, 2025 •

edited

Loading

mokagio Jan 21, 2025

mokagio Jan 21, 2025

kean left a comment

kean Jan 23, 2025

kean Jan 23, 2025

mokagio Jan 28, 2025

		@@ -1,10 +1,11 @@
		swiftlint_version: 0.54.0
		swiftlint_version: 0.58.2

		@@ -0,0 +1 @@
		.build

Set up SwiftLint as a standalone build plugin #23996

Are you sure you want to change the base?

Set up SwiftLint as a standalone build plugin #23996

Conversation

mokagio commented Jan 21, 2025 • edited Loading

Regression Notes

wpmobilebot commented Jan 21, 2025 • edited Loading

wpmobilebot commented Jan 21, 2025 • edited Loading

mokagio Jan 21, 2025

Choose a reason for hiding this comment

mokagio Jan 21, 2025

Choose a reason for hiding this comment

kean left a comment

Choose a reason for hiding this comment

kean Jan 23, 2025

Choose a reason for hiding this comment

kean Jan 23, 2025

Choose a reason for hiding this comment

mokagio Jan 28, 2025

Choose a reason for hiding this comment

mokagio commented Jan 21, 2025 •

edited

Loading

wpmobilebot commented Jan 21, 2025 •

edited

Loading

wpmobilebot commented Jan 21, 2025 •

edited

Loading