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

chore: Add buildtool plugin for SPM swiftlint support #2104

Closed
wants to merge 3 commits into from

Conversation

atierian
Copy link
Member

@atierian atierian commented Aug 8, 2022

Issue #, if available:
N/A

Description of changes:
Adds a buildtool() swiftlint plugin, adds that plugin as a dependency for Amplify and each plugin.
Adds // swiftlint:disable: ... in a few places in AWSCognitoAuthPlugin that were errors.

Implementation based on various points of information and examples in realm/SwiftLint#3840
This targets a specific version of swiftlint w/ checksum.

Check points: (check or cross out if not relevant)

  • Added new tests to cover change, if needed
  • Build succeeds with all target using Swift Package Manager
  • All unit tests pass
  • All integration tests pass
  • Security oriented best practices and standards are followed (e.g. using input sanitization, principle of least privilege, etc)
  • Documentation update for the change if required
  • PR title conforms to conventional commit style
  • If breaking change, documentation/changelog update with migration instructions

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@atierian
Copy link
Member Author

atierian commented Aug 8, 2022

TODO

  • Confirm that this change doesn't degrade customer experience pulling Amplify in as a dependency.

  • Does the buildtool plugin run when a client app builds? If so, does it degrade their build time performance?

  • Profile clean pull (no package cache) time. Does this change increase initial pull time?

  • Do swiftlint warnings appear when a client app builds?

  • Pass SWIFTPM_ENABLE_PLUGINS=1 as env variable to get tests running.

@atierian
Copy link
Member Author

Clean build comparison errors and warnings.

Branch Errors Warnings
dev-preview (97a2bac) 0 217
chore/lint after adding swiftlint plugin 26 1246
Δ +26 +1029

@atierian
Copy link
Member Author

Clean build time comparison

Using xcodebuild -showBuildTimingSummary
building Amplify-Package, which includes Amplify + all plugins

time in seconds

Step dev-preview chore/lint Δ seconds Δ %
SwiftCompile 396.597 497.638 +101.041 +25.5
CompileC 41.800 50.452 +8.652 +20.7
SwiftEmitModule 39.986 54.112 +14.126 +35.3
SwiftDriver 24.292 22.933 -1.359 -5.6
Ld 13.807 14.642 +0.835 +6.0
GenerateDSYMFile 12.497 12.501 +0.004 +0.0
CopySwiftLibs 1.865 1.666 -0.199 -10.7
ProcessInfoPlistFile 0.947 2.540 +1.593 +168.2
Copy 0.334 0.322 -0.012 -3.6
SwiftDriver Compilation Requirements 0.277 0.368 +0.091 +32.9
WriteAuxiliaryFile 0.231 0.216 -0.015 -6.5
SwiftDriver Compilation 0.199 0.169 -0.030 -15.1
RegisterExecutionPolicyException 0.188 0.212 +0.024 +12.8
Touch 0.125 0.053 -0.072 -57.6
CpResource 0.048 0.022 -0.026 -54.2
SwiftMergeGeneratedHeaders 0.030 0.057 +0.027 +90.0
ExtractAppIntentsMetadata 0.021 0.031 +0.010 +47.6
PhaseScriptExecution - 4.964 4.964 -

@atierian
Copy link
Member Author

Client app experience

UX

After pulling in Amplify as a dependency (targeting chore/lint), the first client app build generates this popup dialog.
Xcode popup dialog displaying text Some build plug-ins are disabled because they have changed, or have never been enabled. Enable them now? Build plug-ins with malicious code can harm your Mac or compromise your privacy. Be sure you trust the source of build plug-ins before you enable them. Options to select are Show in Issues Navigator and Trust & Enable All
Not exactly the most reassuring message 😕. Selecting "Show in Issues Navigator" removes the popup and continues the build. However it appears again the next time building the client app. Selecting "Trust & Enable All" prevents the popup from appearing again, even after a clean build of the client app.

Package Resolution Time

Time in seconds from Reset Package Caches until packages are resolved.

dev-preview chore/lint
71.3 77.4

Build Time

Current state has client apps that link Amplify running the swiftlint buildtool plugin. As if that weren't bad enough, it appears though the time to run swiftlint takes exponentially longer when building from a client app in comparison to build the Amplify package directly. This is a show stopper, so we'll have figure out a way to prevent the plugin from building when client apps build.

Action Items

  • Look into ability to prevent build plugin from running when added as a dependency.

@atierian
Copy link
Member Author

The linting warnings don't show up in a client app's issue navigator, but they are available in the build logs. It looks like the addition of the --cache-path speeds up subsequent builds dramatically as the linter's targeted directories aren't altered.

@atierian
Copy link
Member Author

The goal of this is to add SwiftLint as a SPM buildtool plugin that runs when Amplify is built standalone (by developers working on Amplify), but never when Amplify is being built as a dependency of another target (app or library). Despite best efforts, I couldn't get that to happen. I'm throwing in the towel on this for the time being as I'm not confident that SPM plugins can accommodate this goal in their current state. Leaving the PR in draft form with all of the documented findings.

If anyone believes this is doable, I encourage you to continue work on this branch. If you have any questions about the findings listed in the comments above, feel free to ping me here.

@atierian
Copy link
Member Author

The only alternative I can think of at this point is removing the plugin as part of our CI/CD release process. While that would certainly work, it feels brittle and a little hacky.

@atierian
Copy link
Member Author

atierian commented Nov 5, 2024

closing as this is handled in CI

@atierian atierian closed this Nov 5, 2024
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.

1 participant