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

Add a command for listing available simulators #223

Merged
merged 9 commits into from
Oct 6, 2022

Conversation

StevenSorial
Copy link
Contributor

This PR adds a new command xcodes runtimes, which is intended to be a replacement for xcversion simulators from xcode-install. I chose the word "runtimes" to avoid confusion with the actual simulators (ie. iPhone Xs), which is also how apple refer to it in xcrun simctl runtime.

xcversion simulators used a list specific to each installed Xcode, this is now deprecated and now Apple returns all possible runtimes, which makes filtering our responsibilty.

@StevenSorial
Copy link
Contributor Author

StevenSorial commented Sep 25, 2022

@MattKiazyk This PR is only for listing and not for downloading/installing. Its mostly finished.
Do you think it's OK to review this PR on its own or should we include installing in the PR?

@StevenSorial StevenSorial force-pushed the simulator-runtimes branch 2 times, most recently from 477743c to c1c007d Compare September 27, 2022 04:19
Copy link
Contributor

@MattKiazyk MattKiazyk left a comment

Choose a reason for hiding this comment

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

👏 Well done! Just some #picky comments and a question around beta simulators.

@Stevenmagdy i'm ok with merging this first and adding in download/install after. Keeps the PR's smaller.

Sources/XcodesKit/RuntimeList.swift Outdated Show resolved Hide resolved
Sources/XcodesKit/Models.swift Outdated Show resolved Hide resolved
@StevenSorial
Copy link
Contributor Author

@MattKiazyk Thanks for the review. I resolved the comments.
Is there any reason async/await can't be used in the project? Concurrency should be supported starting from macOS 10.15
I used it in the tests but would like to use it in the executable itself instead of PromiseKit

@MattKiazyk
Copy link
Contributor

Is there any reason async/await can't be used in the project? Concurrency should be supported starting from macOS 10.15
I used it in the tests but would like to use it in the executable itself instead of PromiseKit

No reason why. This project was started long before Swift Concurrency and Combine. PromiseKit was used in most projects that the contributors worked on so it was the simplest for us.

I think as long as files are well organized, it can be added. My goal was to switch all of it at one point, but the time just hasn't come.

@StevenSorial
Copy link
Contributor Author

StevenSorial commented Sep 29, 2022

I think as long as files are well organized, it can be added. My goal was to switch all of it at one point, but the time just hasn't come.

I made the new command async as a start. I think it's better to migrate the project gradually using bridging method between PromiseKit and async/await

@MattKiazyk
Copy link
Contributor

@Stevenmagdy Can you mark as ready for review when you're ready for another look?

@StevenSorial
Copy link
Contributor Author

There are a few open questions left that need to be answered. I will demonstrate them with a use case for each one

Installed runtime = A runtime that exists in /Library/Developer/CoreSimulator/Profiles/Runtimes or /Library/Developer/CoreSimulator/Volumes/

Bundled runtime = A runtime that exists in Xcode-X.X.X.app/

Use case 1

  • Installed Xcodes: 13.3 (bundled runtimes: iOS 15.4)

Should xcodes runtimes list iOS 15.5? While Xcode 13.3 can’t create a simulator with 15.5 runtime, a 15.5 runtime is still prefectly installable, discoverable by xcrun simctl runtime list, and can be used if the user later installs Xcode >=14.0.

If we choose to not list it, what should happen if it was already installed? In that case the user would not see iOS 15.5 (installed) as part of the list and might think that the runtime was deleted.

This use case applies to runtimes that is newer than the installed Xcode and can’t be used by it. The PR currently list them.

Use case 2

  • Installed Xcodes: 13.5 (Bundled: iOS 15.5)
  • Installed runtimes: iOS 15.5

In that case there are two exact copies of the iOS 15.5 runtime, one is bundled and the other is installed.
How should we present that info in the list? This PR chooses two duplicate the 15.5 line, one is iOS 15.5 (Bundled) and the other is iOS 15.5 (installed). Another possiblilty is iOS 15.5 (Bundled, Installed) But I think the user might think its the exact runtime.

Use case 3

  • Installed Xcodes: 14.0 (Bundled: iOS 16.0), 13.5 (Bundled: iOS 15.5)
  • Selected Xcode: 14.0

In that case xcrun simctl runtime list will only report the bundled runtimes of Xcode 14.0 because its the selected Xcode, so, xcodes runtimes will have iOS 16.0 (Bundled) and iOS 15.5 with no (Bundled) . We can avoid this by scanning every Xcode-X.X.X.app/ for its bundled runtimes but I’m not sure its worth it just for the (Bundled) indicator.

@MattKiazyk @rogerluan what do you think?

@StevenSorial StevenSorial marked this pull request as ready for review October 1, 2022 12:40
@StevenSorial StevenSorial requested a review from a team as a code owner October 1, 2022 12:40
@rogerluan
Copy link
Contributor

rogerluan commented Oct 3, 2022

Hi @Stevenmagdy 👋

Use Case 1

I think we should list them. Just like users are capable of downloading and installing an Xcode version that their current macOS version doesn't allow running it, the user can first download & install and only upgrade their macOS version later on 👍 So current behavior is correct IMO.

Use Case 2

I think they should display in separate lines, I think this makes it clear here that they are located in different places, although it may or may not be clear that they are the exact same copies just installed from different sources. No strong opinion here, though, if @MattKiazyk or yourself thinks it should be displayed in one line, that's fine too.

Use Case 3

This is a trickier one. In your example, iOS 15.5 is listed as installed even though it's bundled in Xcode 13.5, and not just installed as a runtime? Ideally I'd prefer to display to which version of Xcode each iOS version is bundled to if the user has more than 1 Xcode version installed. Not sure how much longer the user would have to wait to see this info (e.g. how much delay it will increase in the command execution), but I think it's the optimal UX and should be worth the time implementing. If it would add too much time to the command execution, I'd probably add an option to disable this so it runs faster (for advanced users).
With that being said, however, I don't think it's too much of a deal breaker to keep the current behavior. What I'm suggesting would be my expectation as a user, but if it'd be too much of a challenge to introduce this functionality, it can be ticketed as a nice-to-have


Looking forward to hearing @MattKiazyk's thoughts as well 🤗 Great write up @Stevenmagdy! 💪

@StevenSorial
Copy link
Contributor Author

StevenSorial commented Oct 3, 2022

Hi @rogerluan, Thanks for your answer.

Agreed on use case 1 and 2.

Use Case 3

In your example, iOS 15.5 is listed as installed even though it's bundled in Xcode 13.5, and not just installed as a runtime?

No, sorry if it wasn't clear. In this example, no runtime is installed (i.e. nothing exists in /Library/Developer/CoreSimulator/Profiles/Runtimes or /Library/Developer/CoreSimulator/Volumes/). The only iOS runtimes are 16.0 and 15.5 bundled in Xcode 14.0 and 13.5, respectively.

Currently, if Xcode 14.0 is selected, the list would have iOS 16.0 (Bundled) and iOS 15.5, and vice versa if Xcode 13.5 is selected.

if it'd be too much of a challenge to introduce this functionality, it can be ticketed as a nice-to-have

My concern would be that we would be deviating from Apple's official installed/bundled runtimes list reported by xcrun simctl runtime list. Also, it might be more confusing, if Xcode 14.0 is selected then the runtime bundled in Xcode 13.5 is unusable because Xcode 14.0 can't create a simulator using that runtime, i.e. the user can't run simctl create mysim iPhone8 iphoneos15.5 until they set Xcode 13.5 as selected.

Edit: I forgot to mention that this PR chooses to mitigate this UX issue by appending (Bundled with selected Xcode) instead of (Bundled) to clarify that there might be other bundled runtimes in other Xcodes.

@rogerluan
Copy link
Contributor

I wouldn't be too much concerned with the UX issue you're pointing out if xcodes properly listed the xcode version it's bundled in 🤷 to me, that'd make sense. For instance:

iOS 15.5 (Bundled with Xcode 13.5)
iOS 16 (Bundled with Xcode 14, currently selected)

It'd then implicitly indicate "hey, if you wanna use 15.5, you must first select Xcode 14" (sorta). If someone attempts to use it without selecting Xcode 13.5 first, then it'd throw an error but I think users could figure out what the issue is on their own and that they have to select that Xcode version first...

Another alternative (less preferred IMO because it tries to outsmart users) would be to automatically select the Xcode version needed to run that runtime. I'm not a fan of this, but it could be an option maybe?

@MattKiazyk
Copy link
Contributor

  1. We should still allow to install it. Like @rogerluan indicated, we allow to install Xcode if they don't have the min OS. Xcodes.app does a better job of this to check and ask before installing.

  2. Yeah separate lines. If you delete the bundled one, the previous one is there (and taking up precious bits on the harddrive)

  3. I like (Bundled with selected Xcode) option. We can add another log item (at the bottom) to show more information.

@StevenSorial
Copy link
Contributor Author

@MattKiazyk I added a note at the end as you suggested. 👍🏼

iOS 15.5 (Bundled with Xcode 13.5)
iOS 16 (Bundled with Xcode 14, currently selected)

@rogerluan That would be fine too and it solves the confusion issue. I'm ok with either option.

@rogerluan
Copy link
Contributor

rogerluan commented Oct 4, 2022

I still have a slight preference for the implementation of this:

iOS 15.5 (Bundled with Xcode 13.5)
iOS 16 (Bundled with Xcode 14, currently selected)

It's more clear to the user than just the footnote that was added. But that may be just me 🤷‍♂️ @MattKiazyk WDYT?

}
}
}
Current.logging.log("\nNote: Bundled runtimes are only for the selected Xcode, more bundled runtimes may exist in other Xcode(s)")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Current.logging.log("\nNote: Bundled runtimes are only for the selected Xcode, more bundled runtimes may exist in other Xcode(s)")
Current.logging.log("\nNote: Bundled runtimes are indicated for the currently selected Xcode, more bundled runtimes may exist in other Xcode(s)")

Copy link
Contributor

@MattKiazyk MattKiazyk left a comment

Choose a reason for hiding this comment

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

@Stevenmagdy this is really good! Appreciate the tests and the solid coding.

If you're ok with it - just the one suggested wording change.

@StevenSorial
Copy link
Contributor Author

@MattKiazyk CI was outdated. Could you please run it again?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants