search

apache / cordova-paramedic

Apache Cordova - Paramedic
https://cordova.apache.org/
Apache License 2.0
36 stars 53 forks source link

feat(ios): add Xcode 13 & iOS 15 support #226

Closed airdrummingfool closed 2 years ago

airdrummingfool commented 2 years ago

Platforms affected

iOS

Motivation and Context

The instruments command has been deprecated in Xcode 12 and removed in Xcode 13, which causes the getSimulatorCollection() function to fail to list the simulators. The instruments command in Xcode 12 includes a deprecation warning that says to use xcrun xctrace instead.

fixes #225

Description

Testing

Checklist

erisu commented 2 years ago

I have not tested these changes but are you 100% positive that they are working correctly from your testing?

Looking at the code from this link shows that the returned result is parsed by regular expressions to grab the simulator IDs. https://github.com/apache/cordova-paramedic/blob/master/lib/utils/utilities.js#L134

As you pointed out in your ticket description:

it looks like xcrun xctrace list devices is a good replacement, though the output does not line up exactly.

If the regular expression is not updated, I do not see how your testing of this PR works cause the regular expression is looking for the old pattern.

Can you please confirm this?

airdrummingfool commented 2 years ago

@erisu Thanks for pointing this out. I didn't look at the regex because in my testing, the original change in this PR was enough to bypass the crash and complete the tests. However, looking into it more, you're right - the regex will not generate any matches as-is, and should be adjusted.

I've changed the code a bit to account for the difference, and the regex now correctly finds matches. The only thing I can find that this materially affects is that cordova-paramedic couldn't uninstall the app from the simulator without the new changes (which didn't stop the tests from proceeding, which is why I didn't notice it in my original testing).

One small note: I wasn't sure how best to handle Simulator being a part of the new output. It could be integrated into the regex, but it seemed like that would make it more brittle (and possibly cause issues across languages?) so I added separate code to remove Simulator from the line before the regex is run. It's still not language-independent, but at least it can be changed easily if it becomes necessary.

erisu commented 2 years ago

Previous Regular Expression w/ instruments

Use Case 1: iPhone:

Previous RegEx: /^([a-zA-Z\d ]+) \(([\d.]+)\) \[([a-zA-Z\d-]*)\].*$/ Sample Data: iPhone 12 (14.5) [0AFC41A6-E65C-484A-89C8-EC756B330821] (Simulator)

Example Match:

use-case-1

Use Case 2: iPhone + Apple Watch:

We do not handle this use case since the previous regular expression does not match with the sample data: iPhone 12 (14.5) + Apple Watch Series 5 - 44mm (7.4) [DC773D49-C704-4BA2-A3C1-A6FCAA68B047] (Simulator)

This is to remain excluded from this PR.

New Regular Expression w/ xcrun xctrace list

Use Case 1: iPhone:

New RegEx: /^([a-zA-Z\d ]+) Simulator \(([\d.]+)\) \(([a-zA-Z\d-]*)\).*$/ Sample Data: iPhone 12 Pro Simulator (15.0) (14B3DA19-0752-4484-B393-AC95EF2D46F8)

In the above example RegEx, it might be better to add Simulator to the expression.

Example Match:

use-case-1-new

Use Case 2: iPhone + Apple Watch:

This is to remain excluded from this PR.

erisu commented 2 years ago

From the above comment, what do you think about adding Simulator to the expression?

/^([a-zA-Z\d ]+) Simulator \(([\d.]+)\) \(([a-zA-Z\d-]*)\).*$/

airdrummingfool commented 2 years ago

@erisu that was the other option I considered. I have no objections to doing it that way - I will update the PR accordingly.

erisu commented 2 years ago

For some additional notes, I have been trying to figure out why the iOS tests are failing in paramedic. It seems that GitHub has finally added iOS 15 to their image and all of our tests are running against iOS 15.

So I am trying to figure out what we can do to make it run against the correct versions.

Here are the changes I have been messing around with, within my repo. These changes also includes your changes. https://github.com/erisu/cordova-paramedic/pull/14/files

erisu commented 2 years ago

So from testing, the macOS image has these versions of Xcode preinstalled.

Available versions:
- 13.0.0 (/Applications/Xcode_13.0.app)
- 13.0.0 (/Applications/Xcode_13.0_beta.app)
- 12.5.1 (/Applications/Xcode_12.5.1.app)
- 12.5.0 (/Applications/Xcode_12.5.app)
- 12.4.0 (/Applications/Xcode_12.4.app)
- 11.7.0 (/Applications/Xcode_11.7.app)

If Xcode is set to 11.7.0. the command xcrun xctrace list devices does not exist and needs instruments -s devices. The iOS Simulator version for Xcode 11.7.0, that is on the image, is 13.7.

If Xcode is set to 12.5.1, the command xcrun xctrace list devices works. The iOS Simulator version for Xcode 12.5.1, that is on the image, is 14.5.

If Xcode is set to 13.0.0, the command xcrun xctrace list devices works. The iOS Simulator version for Xcode 13.0.0, that is on the image, is 15.0.

erisu commented 2 years ago

Do you want me to push my changes to this PR?

airdrummingfool commented 2 years ago

@erisu I'm good with whatever is easiest. You're welcome to push your changes here, or if you want to open a new PR based on your test that is fine with me as well.

erisu commented 2 years ago

OK I pushed my changes..

I had tried before you suggestion

(xcrun xctrace list devices || instruments -s devices) 2>&1 | grep ^iPhone

But I had to change it some reason, but I dont remember why.

airdrummingfool commented 2 years ago

@erisu looks like we need the rest of your changes from .github/workflows/ios.yml - right now the iOS 15 tests are failing because they're running on macOS 10.15.7 without Xcode 13, which must be what GH considers "latest".

erisu commented 2 years ago

Sorry I didnt see you revert my changed and I added it back.. I will stop updating it now and let you fix it how you think it should be..

My changes only fixes iOS 12-14.. but 15 is broken because it can not communicate with the appium. All my workflow changes were correct for also fixing iOS 12 to 14 that was broken.

iOS 15 which needs Xcode 13 is on mac-11, which is what iOS 15.x test uses.

Anyways.. I will stop touching the PR now so you can fix it how you like.

Sorry for the repush because I thought it was a Cherry-Pick conflict.

airdrummingfool commented 2 years ago

No worries, I didn't mean to revert any of your changes. I see one more small change to bring over to fix the iOS 12 tests, and then I'll take a look at the changes overall.

erisu commented 2 years ago

@airdrummingfool Since you were reverting my CI changes, I extracted my CI changes and pushed them into a seperate PR so they are still available and not lost:

Since they are passing we can merge it in and then your PR can focuse strictly on the Xcode 13.x w/ iOS 15.x (xcrun xctrace) changes... This would reduce the size of your PR scope.

If preferred.

airdrummingfool commented 2 years ago

@erisu My apologies, I did not intend to revert your changes. This branch has all but one of those changes in it already, so I can just add the last one and I think it should be good (you'll have to trigger another test run on this PR). But I also have no problem with your PR getting merged first if that's what you prefer.

airdrummingfool commented 2 years ago

@erisu if you'll run the tests now, I think this PR might be complete. At least, I tested locally with Xcode 11, 12, and 13 and the simulator ID was successfully captured...

airdrummingfool commented 2 years ago

@erisu sorry, had to fix a few lint problems. Can you run the tests again?

erisu commented 2 years ago

For information, you can run npm run lint locally to test if there is any linting error.

If there are any issues, depending on the difficulty of the issue, you might be able to resolve them with npm run lint -- --fix. But if its a complicated linting issue it would still warn and require manual fixing.

airdrummingfool commented 2 years ago

@erisu I ran it after the initial lint test failure to fix the other problems, but I missed one. It's now passing on my machine. Thanks for your help!

erisu commented 2 years ago

@airdrummingfool I went ahead and merged in my PR: https://github.com/apache/cordova-paramedic/pull/227 and rebased this PR with master.

I think it was a good decision to split out the changes to reduce the noise in this PR.

This PR's original goal is to add support for xcrun xctrace for Xcode 13. This is needed for iOS 15, so adding iOS 15.x test would be great since it comes with Xcode 13.x

My CI related changes were mostly improvement changes with the GH Action's environment setup. They were needed because I noticed all tests were running off iOS 14.4 and were incorrect. Since it was a fix to another problem, it was also another reason to split out.

Now that I have removed the extra noise from this PR, I think it will be eaiser for others to review as well. And now the PR can focus strictly on the Xcode 13.x w/ iOS 15.x...

airdrummingfool commented 2 years ago

@erisu sounds good to me, thanks!