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

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

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

AliSoftware
Copy link
Contributor

@AliSoftware AliSoftware commented Apr 15, 2024

Why?

To prevent the Editor pod to fail to be pushed to trunk if the Aztec pod it depends on hasn't propagated through the CDN yet

Fixes the issue raised in this Slack thread: p1711026617605489-slack-CC7L49W13 cc @geriux

Testing

It's gonna be difficult to test the change on this repo without actually doing a new version of the Aztec and Editor pods.

The new ability of publish_pod to accept a --synchronous flag has already been tested as part of the Gravatar-iOS-SDK repo (which had a similar issue of co-dependant pods having to be released together), so I think we should be good though.

So I say we can probably trust that it will work as expected like it did for the Gravatar repo, and I'm counting on the next dev who will have to make a new version of these pods to let us know if it ends up still not working.

To prevent the Editor pod to fail to be pushed to trunk if the Aztec pod it depends on hasn't propagated through the CDN yet
@AliSoftware AliSoftware requested review from a team and geriux April 15, 2024 17:00
@AliSoftware AliSoftware self-assigned this Apr 15, 2024
@AliSoftware AliSoftware changed the title Make pod trunk push be --synchronous Make pod trunk push be --synchronous — to fix issue when releasing both pods together Apr 15, 2024
@AliSoftware AliSoftware enabled auto-merge April 15, 2024 17:45
@AliSoftware AliSoftware changed the title Make pod trunk push be --synchronous — to fix issue when releasing both pods together [Tooling] Make pod trunk push be --synchronous, to fix issue when releasing both pods together Apr 15, 2024
@mokagio
Copy link
Contributor

mokagio commented Apr 16, 2024

I noticed CI has been waiting for almost 12 hours.

Not surprising given it's configured to run on Xcode 13 😳

image

Looking at upgrading now...

This was referenced Apr 16, 2024
@AliSoftware
Copy link
Contributor Author

It seems like the test failures are due to the "Allow Paste" System Prompt—which now appears on the simulator in more recent iOS versions—during the copy/paste-related tests.

@geriux @mokagio can I let you look at those failures? There's probably a way in XCTest to catch those during a UITest (maybe addUIInterruptionMonitor ? Other?)
I gave it a quick try on my end but this quickly turned into a rabbit hole and I didn't have the bandwidth to go further on my end, so I'll let this to you 🙇


PS: While taking a quick look at the workspace I noticed that the build settings are still set to DEPLOYMENT_TARGET=11.0 (and the podspec as well), and if we update the project to e.g. iOS13, there are a couple of depreciations warnings and changes that would be needed in the code to get rid of those warnings (which would otherwise prevent the pod to be released) + make the lib ready for later updates.

I also noticed that all the methods around manipulating and converting between NSRange/UTF16NSRange/Range<String.UTF16View> seems from a time where the model for the String type in Swift was not as nice as today's. Since then, Swift.Foundation have gotten a lot of built-in methods for that (like NSRange(swiftrange, in: swiftstring) and the likes) which would be more resilient (including handling edge cases like NSNotFound and how it might be represented differently in 32 vs 64 bits platforms, ranges with negative locations and/or length, etc) than the manual code written in Aztec's extensions, and switching to them would make the code more bug-proof, especially on such a tricky topic of manipulating Unicode representations of Strings.

@mokagio
Copy link
Contributor

mokagio commented Apr 17, 2024

PS: While taking a quick look at the workspace I noticed that the build settings are still set to DEPLOYMENT_TARGET=11.0 (and the podspec as well), and if we update the project to e.g. iOS13, there are a couple of depreciations warnings and changes that would be needed in the code to get rid of those warnings (which would otherwise prevent the pod to be released) + make the lib ready for later updates.

2f50d83 bumps the target to iOS 12, which is the minimum supported by Xcode 15. The warnings remain, I added --allow-warnings to the pod commands as a workaround.

@mokagio
Copy link
Contributor

mokagio commented Apr 17, 2024

It seems like the test failures are due to the "Allow Paste" System Prompt—which now appears on the simulator in more recent iOS versions—during the copy/paste-related tests.

🤔

@jkmassel addressed this in #1377 by making the pasteboard injectable during unit testing. When I merged the latest develop into #1392, the lockup related to prompt disappeared.

As far as I can tell, the remaining tests failures at the custom TextView level are due to something misbehaving during the HTML parsing. If I had time to investigate, I would pursue the String API route you suggested:

I also noticed that all the methods around manipulating and converting between NSRange/UTF16NSRange/Range<String.UTF16View> seems from a time where the model for the String type in Swift was not as nice as today's

@AliSoftware
Copy link
Contributor Author

AliSoftware commented Apr 17, 2024

The warnings remain, I added --allow-warnings to the pod commands as a workaround.

I don't see that --allow-warnings added to the pod commands in .buildkite/ yet, so I'm guessing you either wrote that you'd do it but forgot, or forgot to push the change 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants