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

Remove CocoaPods #23958

Open
wants to merge 28 commits into
base: christmas-feature-branch
Choose a base branch
from

Conversation

mokagio
Copy link
Contributor

@mokagio mokagio commented Jan 8, 2025

This is a replay of #23951 but onto #23923, as discussed with @kean here. I copied the PR description below, but you might want to check the original PR, too, for the inline comments.


Notice the base is #23960, which addresses the build failures from #23923, so that we can verify that CI is green with only the CocoaPods changes. Refer to that PR for explanations for UI tests failures, too.


Picks up where #23750, but starting from scratch because of too many project file changes since then, and completes the migration away from CocoaPods as the mean of linking the Gutenberg XCFramework.

The sequence of changes is roughly:

  • De-integrate CocoaPods
  • Drag-n-drop the xcframeworks
  • Tweak build settings to work around compilation errors due to the frameworks see fbb0997 and 4a29aea
  • Update the script that copies the React Native App.js file from the XCFramework to the build products as a jsbundle to use the new location
  • Remove the various "if cocoapods" kind of checks from app code and automation

Theree things noteworthy:

  • I had to disable a couple of clang checks, CLANG_WARN_STRICT_PROTOTYPES and CLANG_WARN_QUOTED_INCLUDE_IN_FRAMEWORK_HEADER, because of build failures in the XCFramework imports. It doesn't seem ideal, but also we have increasingly less Objective-C so it should be acceptable. Open to suggestions.
  • @jkmassel 's implementation had a build phase that downloaded the XCFrameworks sources. I omitted it and expect devs to run make dependencies before building the app. It felt a build phase was a bit too much, but it also feels like a line in the README to call attention to this is not enough. Open to suggestions.
  • @jkmassel 's implementation also deleted the xcworkspace in favor of using only the xcproject, which makes sense given the workspace was required by the CocoaPods setup which is now gone. I decided not to remove the workspace in this PR just to not add to an already big diff. Also, when I opened the standalone project file, some things weren't working out of the box, so I decide to postpone the move to it.

SwiftLint

SwiftLint was previously fetched via CocoaPods and the automation was configured with that in mind. Removing CocoaPods also removes SwiftLint. In the interest of moving on with the removal process, I didn't provide an alternative setup for SwiftLint. The rake lint task merely prints a "no linter configured" message at the moment. As you can see in CI, this does not affect SwiftLint there, so we still have reliable checks in place.

Testing

I published this from the Simulator: https://giotestdotsite.wordpress.com/2025/01/07/via-gutenbegkit-without-cocoapods/ (please ignore the slug, it's indeed mobile-gunteber) — Not sure if the HTML source can show it, but I did check locally that the experimental editor toggle was disabled. Besides, the UI being a bit different between the two helps being aware of it.

Apart from that, I'd say run the prototype build on device and see if the Gutenber-via-XCFramework editor works as expected.

Regression Notes

  1. Potential unintended areas of impact

It's possible that some behavior in the Gutenber-XCFramework editor will be compromised. However, I don't see why, once the editor loads, all the code should be the same.

  1. What I did to test those areas of impact (or what existing automated tests I relied on)

Had a go at publishing a post from the Simulator.

  1. 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.

Testing checklist:

  • WordPress.com sites and self-hosted Jetpack sites.
  • Portrait and landscape orientations.
  • Light and dark modes.
  • Fonts: Larger, smaller and bold text.
  • High contrast.
  • VoiceOver.
  • Languages with large words or with letters/accents not frequently used in English.
  • Right-to-left languages. (Even if translation isn’t complete, formatting should still respect the right-to-left layout)
  • iPhone and iPad.
  • Multi-tasking: Split view and Slide over. (iPad)

@dangermattic
Copy link
Collaborator

dangermattic commented Jan 8, 2025

2 Warnings
⚠️ Modules/Package.swift was changed without updating its corresponding Package.resolved. Please resolve the Swift packages as appropriate to your project setup (e.g. in Xcode or by running swift package resolve).
⚠️ This PR is larger than 500 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.

Generated by 🚫 Danger

@mokagio mokagio added this to the 25.8 milestone Jan 8, 2025
@mokagio mokagio added the Tooling Build, Release, and Validation Tools label Jan 8, 2025
@mokagio mokagio self-assigned this Jan 8, 2025
@wpmobilebot
Copy link
Contributor

wpmobilebot commented Jan 8, 2025

WordPress Alpha📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.
App NameWordPress Alpha WordPress Alpha
ConfigurationRelease-Alpha
Build Numberpr23958-b2e22a1
Version25.6
Bundle IDorg.wordpress.alpha
Commitb2e22a1
App Center BuildWPiOS - One-Offs #11324
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Jan 8, 2025

Jetpack Alpha📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.
App NameJetpack Alpha Jetpack Alpha
ConfigurationRelease-Alpha
Build Numberpr23958-b2e22a1
Version25.6
Bundle IDcom.jetpack.alpha
Commitb2e22a1
App Center Buildjetpack-installable-builds #10357
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@mokagio mokagio requested review from jkmassel and kean January 8, 2025 10:09
@mokagio mokagio changed the base branch from christmas-feature-branch to mokagio/christmas-build-failures January 8, 2025 10:36
@mokagio mokagio force-pushed the mokagio/remove-cocoapods-christmast-branch branch from 96e2bd8 to 1e62678 Compare January 8, 2025 10:37
@mokagio mokagio marked this pull request as ready for review January 8, 2025 15:57
Base automatically changed from mokagio/christmas-build-failures to christmas-feature-branch January 9, 2025 17:05
That's for the value `CLANG_WARN_QUOTED_INCLUDE_IN_FRAMEWORK_HEADER`,
but it's useless to set `$(inherited)` for a Yes or No value because
removing the value from the target will automatically make Xcode read it
from the project, achieving the same result.
Was giving errors in Yoga:

> Double-quoted include "YGEnums.h" in framework header, expected angle-bracketed instead
It resulted in a build failure:

> React.framework/Headers/RCTAppearance.h:16:60:
> A function declaration without a prototype is deprecated in all versions of C
Strange that `pod deintegrate` did not remove it. Maybe it was some
custom piece of setup...
@mokagio mokagio force-pushed the mokagio/remove-cocoapods-christmast-branch branch from 1e62678 to b2e22a1 Compare January 10, 2025 10:58
@kean kean closed this Jan 10, 2025
@kean
Copy link
Contributor

kean commented Jan 10, 2025

Hey, nice work. It's a historic moment 😃

I've updated the tests in the target branch. It should be green now. I don't think I should/can review the changes in infra, but you get 👍

@kean kean reopened this Jan 10, 2025
end

abstract_target 'Tools' do
pod 'SwiftLint', swiftlint_version
Copy link
Contributor

Choose a reason for hiding this comment

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

Where does the SwiftLint version get pinned now? It doesn't appear to be in Gemfile. I'd probably suggest trying SwiftPM since this project is not itself a dependency, so it should be fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Where does the SwiftLint version get pinned now?

The version is defined in .swiftlint.yml:

swiftlint_version: 0.54.0

As for installing the tool, I'm happy to have a go at setting it up via SwiftPM. I ignored the problem for the context of this PR because there was already a lot going on.

I was also keen to ask everyone in the team what they'd prefer to do for provisioning. @jkmassel and I discussed. in the past the idea of having Apps Infra recommend and maintain a golden path for integration (which was CocoaPods but now needs to be updated) but leave the teams the option to decide how to go about it. For example, some people/team in the past expressed the feeling that running the linter on every build was too much friction when just hammering code together and they'd rather get linter feedback in the PR via CI only.

Copy link
Contributor

@kean kean Jan 10, 2025

Choose a reason for hiding this comment

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

I'd suggest removing swiftlint_version from .swiftlint.yml and pinning it using SwiftPM. It should solve both issues: installation and pinning. Wdyt?

Note: I never tried it. I usually use Gemfile for tools.

rather get linter feedback in the PR via CI only

I'm sorry, I don't believe anyone would've preferred this 😄

@mokagio mokagio requested a review from a team January 10, 2025 15:24
tar -xvf Frameworks/Gutenberg.tar.gz -C Frameworks/ -k
mv -n Frameworks/Frameworks/*.xcframework Frameworks/
rm -rf Frameworks/Frameworks Frameworks/dummy.txt
mkdir -p Frameworks/hermes.xcframework/ios-arm64/dSYMs Frameworks/hermes.xcframework/ios-arm64_x86_64-simulator/dSYMs
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given gutenberg-mobile should have an hopefully short time left in the project, I think this rudimental way to fetch it is acceptable.

Also, the version is not likely to change at all, which again makes me comfortable with simply hardcoding it there and duplicating it it in Fastlane.

#import "WPAuthenticator-Swift.h"
#import <WordPressAuthenticator/WordPressAuthenticator-Swift.h>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The header above was necessary when the lib was distributed via CocoaPods, but now we can access the Xcode-generated one directly.

This is unrelated with the CP removal for Gutenberg, but I noticed it when grepping "CocoaPods".

@@ -50,6 +50,7 @@ let package = Package(
.package(url: "https://github.com/Automattic/wordpress-rs", revision: "alpha-20241116"),
.package(url: "https://github.com/wordpress-mobile/GutenbergKit", revision: "4476d597b271778d001a26c50d74e527b54ebfef"),
.package(url: "https://github.com/Automattic/color-studio", branch: "trunk"),
.package(url: "https://github.com/allenhumphreys/AztecEditor-iOS", branch: "ah/spm"),
Copy link
Contributor

Choose a reason for hiding this comment

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

👋 @mokagio - do we want to point Aztec to a fork? If I understand correctly this will block us if we need to push out the rare Aztec fix. Or maybe there's a timeline to get back to the WP org repo shortly after this change?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Tooling Build, Release, and Validation Tools
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants