-
Notifications
You must be signed in to change notification settings - Fork 139
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
Conversation
@MattKiazyk This PR is only for listing and not for downloading/installing. Its mostly finished. |
477743c
to
c1c007d
Compare
There was a problem hiding this 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.
@MattKiazyk Thanks for the review. I resolved the comments. |
4eb8075
to
42e1131
Compare
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. |
b6f1f0a
to
a90f72a
Compare
I made the new command |
@Stevenmagdy Can you mark as ready for review when you're ready for another look? |
There are a few open questions left that need to be answered. I will demonstrate them with a use case for each one
Use case 1
Should If we choose to not list it, what should happen if it was already installed? In that case the user would not see 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
In that case there are two exact copies of the iOS 15.5 runtime, one is bundled and the other is installed. Use case 3
In that case @MattKiazyk @rogerluan what do you think? |
a90f72a
to
abf5082
Compare
Hi @Stevenmagdy 👋 Use Case 1I 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 2I 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 3This is a trickier one. In your example, Looking forward to hearing @MattKiazyk's thoughts as well 🤗 Great write up @Stevenmagdy! 💪 |
Hi @rogerluan, Thanks for your answer. Agreed on use case 1 and 2. Use Case 3
No, sorry if it wasn't clear. In this example, no runtime is installed (i.e. nothing exists in Currently, if Xcode 14.0 is selected, the list would have
My concern would be that we would be deviating from Apple's official installed/bundled runtimes list reported by Edit: I forgot to mention that this PR chooses to mitigate this UX issue by appending |
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:
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 I added a note at the end as you suggested. 👍🏼
@rogerluan That would be fine too and it solves the confusion issue. I'm ok with either option. |
I still have a slight preference for the implementation of this:
It's more clear to the user than just the footnote that was added. But that may be just me 🤷♂️ @MattKiazyk WDYT? |
Sources/XcodesKit/RuntimeList.swift
Outdated
} | ||
} | ||
} | ||
Current.logging.log("\nNote: Bundled runtimes are only for the selected Xcode, more bundled runtimes may exist in other Xcode(s)") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)") |
There was a problem hiding this 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.
@MattKiazyk CI was outdated. Could you please run it again? |
This PR adds a new command
xcodes runtimes
, which is intended to be a replacement forxcversion 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 inxcrun 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.