Use Xcode 15.0.1 by jkmassel · Pull Request #1377 · wordpress-mobile/AztecEditor-iOS

jkmassel · 2023-11-04T02:26:26Z

Migrates the project to Xcode 15 and Apple Silicon in CI

Also includes:

An adjustment to the UITextView subclass that injects a UIPasteboard object. This allows us to mock access to the system pasteboard, which requires user interaction in recent iOS versions
An adjustment to string index handling – in newer versions of Xcode, there are runtime assertions about string index overflow, so we need to be a bit more careful when replacing text
A tiny little fix for a failing test, presumably related to something about how iOS generates PNG files.

This can be reviewed by anyone, but I'd appreciate if @mokagio or @crazytonyli took a look at the project code adjustments!

Testing Details

Ensure CI passes

Please check here if your pull request includes additional test coverage.
I have considered if this change warrants release notes and have added them to the appropriate section in the CHANGELOG.md if necessary.

This resolves a string overflow issue that appeared in tests in Xcode 15.0.1

Apple requires user interaction to allow using the pasteboard – this change allows us to mock the pasteboard while under test to avoid that

crazytonyli · 2023-11-05T21:04:41Z

Aztec/Classes/Extensions/String+RangeConversion.swift

+
+    func isValidIndex(_ index: String.UTF16View.Index) -> Bool {
+        (self.startIndex ... self.endIndex).contains(index)
+    }


The endIndex is not an accessible index.

A string’s “past the end” position—that is, the position one greater than the last valid subscript argument.

AliSoftware · 2023-12-18T12:01:56Z

Aztec/Classes/Extensions/String+RangeConversion.swift

+    }
+
+    func isValidIndex(_ index: String.UTF16View.Index) -> Bool {
+        (self.startIndex ... self.endIndex).contains(index)


Suggested change

(self.startIndex ... self.endIndex).contains(index)

(self.startIndex ..< self.endIndex).contains(index)

crazytonyli · 2023-12-18T19:55:43Z

.buildkite/pipeline.yml

+      # post a message in the logs
+      cat .buildkite/validate_podspec_annotation.md
+      # and also as an annotation
+      cat .buildkite/validate_podspec_annotation.md | buildkite-agent annotate --style 'warning'


There is a new workaround for this issue: You can pass a --patch-cocoapods option to validate_podspec and the publish pod utilities. See https://github.com/wordpress-mobile/WordPressKit-iOS/pull/652/files.

crazytonyli

Maybe updating the publishing pod commands in.buildkite/publish-aztec-pod.sh too, with the same --patch-cocoapods option? Otherwise, future releasing jobs would fail.

crazytonyli · 2023-12-19T21:17:46Z

.buildkite/pipeline.yml

    env: *common_env
    plugins: *common_plugins
+    agents:
+      queue: upload


This agents configuration needs to be removed.

crazytonyli · 2023-12-20T00:02:24Z

Aztec/Classes/TextKit/TextView.swift


    open override func cut(_ sender: Any?) {
-        let data = storage.attributedSubstring(from: selectedRange).archivedData()
+        let data = try! storage.attributedSubstring(from: selectedRange).archivedData()


Does that mean this line crashes if the selected attributed string contains an "attribute value" that doesn't conform to NSCoding? I don't really know anything about this library's implementation, not sure how likely that's going to happen...

That's a good question, but also before the migration to Xcode 15 and newer Swift back when archivedData was not marked as throws, I'd expect the code would already crash (but maybe with an ObjC NSException rather than a Swift error being thrown)?

That being said, it would be a nice improvement to use let data = try? … instead and then make the subsequent call to storeInPasteboard(encoded: data) conditional on if let data; but not sure if that fix belongs in this PR or separate.

Also, looking at the deprecated vs new method signature for NSKeyedArchiver.archivedData, I'm wondering if the only case where it could throw is if you passed requiresSecureCoding: true and the objects conformed to NSCoding… but not NSSecureCoding.

But since here we use requiresSecureCoding: false in the implementation of archivedData, maybe that means we'll never throw in practice? Or if we do, that would be under the same conditions as the ones the current version of the library already does, i.e. not related to Xcode 13->Xcode15 migration?

AliSoftware · 2023-12-20T16:54:29Z

Breadcrumbs: I reverted the attribution of the cluster to this repo in Buildkite (https://github.com/Automattic/buildkite-ci/pull/331) — aka reverted this repo to use Intel infra — while this PR is still not merged.

Once we land this PR, don't forget to redo the change in aztec-editor-ios.tf to switch it back to use the cluster again.

mokagio · 2024-01-12T00:16:10Z

I wanted to help here and tried to address the unit tests failures via #1382 but didn't get very far.

mokagio · 2024-04-16T04:56:02Z

Going to merge this despite unit tests failures, to unblock moving to the cluster in CI, which in turns unblocks adopting the latest Xcode, #1392, which, finally, will unblock #1391.

I'm still experiencing unit tests issue locally, but I want to see how/if the situation changes once this lands.

jkmassel added 7 commits November 3, 2023 17:54

Build with Xcode 15.0.1

746cb0b

Add additional string range bounds checking

4e32af6

This resolves a string overflow issue that appeared in tests in Xcode 15.0.1

Fix tests broken by iOS 14 pasteboard restrictions

e73e1c8

Apple requires user interaction to allow using the pasteboard – this change allows us to mock the pasteboard while under test to avoid that

Test with iOS 17 and iPhone 15 Pro

c076791

Make Gutenpack renderer tests easier to debug

39ebb60

Fix snapshot tests

6f244bb

Disable podspec validation

1539e7c

jkmassel added the [Type] Enhancement label Nov 4, 2023

jkmassel requested a review from a team November 4, 2023 02:26

jkmassel self-assigned this Nov 4, 2023

crazytonyli requested changes Nov 5, 2023

View reviewed changes

AliSoftware reviewed Dec 18, 2023

View reviewed changes

crazytonyli reviewed Dec 18, 2023

View reviewed changes

jkmassel added 2 commits December 19, 2023 13:50

Fix off-by-one issue with string indexes

5e4c974

Fix linter issues

71a459e

jkmassel requested review from AliSoftware and crazytonyli December 19, 2023 20:53

jkmassel enabled auto-merge (rebase) December 19, 2023 20:54

jkmassel force-pushed the use/xcode-15.0.1 branch from b7d60ed to 5f76bb3 Compare December 19, 2023 21:00

crazytonyli requested changes Dec 19, 2023

View reviewed changes

Apply Tony’s CocoaPods fix

4436946

jkmassel force-pushed the use/xcode-15.0.1 branch from 5f76bb3 to 4436946 Compare December 19, 2023 21:47

jkmassel added 3 commits December 19, 2023 15:16

Fix CocoaPods warnings

1a00efb

Bump iOS Min Version to 12.0

59f04a1

One more warning

1dd0099

crazytonyli reviewed Dec 20, 2023

View reviewed changes

AliSoftware approved these changes Dec 20, 2023

View reviewed changes

crazytonyli approved these changes Dec 20, 2023

View reviewed changes

mokagio disabled auto-merge April 16, 2024 04:56

mokagio merged commit 0fd30b2 into develop Apr 16, 2024

mokagio deleted the use/xcode-15.0.1 branch April 16, 2024 04:56

This was referenced Apr 16, 2024

Use Xcode 15.3 in CI #1392

Closed

[Tooling] Make pod trunk push be --synchronous, to fix issue when releasing both pods together #1391

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Xcode 15.0.1#1377

Use Xcode 15.0.1#1377
mokagio merged 13 commits intodevelopfrom
use/xcode-15.0.1

jkmassel commented Nov 4, 2023

Uh oh!

crazytonyli Nov 5, 2023 •

edited

Loading

Uh oh!

AliSoftware Dec 18, 2023

Uh oh!

crazytonyli Dec 18, 2023

Uh oh!

crazytonyli left a comment

Uh oh!

crazytonyli Dec 19, 2023

Uh oh!

crazytonyli Dec 20, 2023

Uh oh!

AliSoftware Dec 20, 2023 •

edited

Loading

Uh oh!

AliSoftware Dec 20, 2023

Uh oh!

AliSoftware commented Dec 20, 2023 •

edited

Loading

Uh oh!

mokagio commented Jan 12, 2024

Uh oh!

mokagio commented Apr 16, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Comments

	(self.startIndex ... self.endIndex).contains(index)
	(self.startIndex ..< self.endIndex).contains(index)

Conversation

jkmassel commented Nov 4, 2023

Migrates the project to Xcode 15 and Apple Silicon in CI

Testing Details

Uh oh!

crazytonyli Nov 5, 2023 • 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!

AliSoftware Dec 18, 2023

Choose a reason for hiding this comment

Uh oh!

crazytonyli Dec 18, 2023

Choose a reason for hiding this comment

Uh oh!

crazytonyli left a comment

Choose a reason for hiding this comment

Uh oh!

crazytonyli Dec 19, 2023

Choose a reason for hiding this comment

Uh oh!

crazytonyli Dec 20, 2023

Choose a reason for hiding this comment

Uh oh!

AliSoftware Dec 20, 2023 • 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!

AliSoftware Dec 20, 2023

Choose a reason for hiding this comment

Uh oh!

AliSoftware commented Dec 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mokagio commented Jan 12, 2024

Uh oh!

mokagio commented Apr 16, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Comments

crazytonyli Nov 5, 2023 •

edited

Loading

AliSoftware Dec 20, 2023 •

edited

Loading

AliSoftware commented Dec 20, 2023 •

edited

Loading