Support runtime downloading/installing #230
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@MattKiazyk The PR is still a WIP but I may need some help regarding I rewrote the Downloader to be shared for downloading Xcodes and runtimes, For some reason, progress is not reported when downloading runtimes using Aria, but it's working fine when downloading Xcodes. |
|
@Stevenmagdy Haven't looked much at the PR but curious about:
I'm pretty sure we are free and clear of that these days, using the new |
|
I will investigate more, but Isn't that only for XcodeReleases? |
|
The So similarly for the simulators, we can get that download path from a public endpoint. So for the final download we just need to make sure we've called the |
|
Done. Thanks for the info. |
MattKiazyk
reviewed
Oct 14, 2022
Sources/XcodesKit/RuntimeList.swift
Outdated
|
|
||
| public func downloadAndInstallRuntime(identifier: String, downloader: Downloader) async throws { | ||
| let downloadables = try await downloadableRuntimes(includeBetas: true) | ||
| guard let matchedRuntime = downloadables.first(where: { $0.visibleIdentifier == identifier }) else { |
There was a problem hiding this comment.
So one concern I have is the onus on the user to have to put quotes around the version input.
Wondering how we could make that easier? Or allow alternative options?
- Allow
runtimes install ios 16.0where it parses both. - Have no space
iOS16.0?
There was a problem hiding this comment.
I think requiring users to quote input with spaces is reasonable, especially since that's a pattern already established by many CLI tools (e.g. changing to a directory that includes a space in the name). I think the two proposed alternatives aren't desirable.
- A single input (in this case the version) should be consumed as a single argument, not n number of arguments. This would allow future extensibility of the command, such as specifying multiple versions to install in one command.
- Listing available runtimes shows a list where versions have spaces in the name (e.g. "ios 16.0"). Requiring users to input a version without spaces (e.g. "ios16.0") makes it harder to view available runtimes and then install them. It would also require documenting this need well enough for users to understand, in which case you might as well just say 'put quotes around the version'.
There was a problem hiding this comment.
@MattKiazyk I tend to agree with @beechtom on this. The first option IMO is doing more harm than good, and the second option is partially applied in beta versions 16.1-beta1 (instead of 16.1 beta1) but not the platform name itself because I don't think it looks good. I hope that doesn't block the PR from being merged.
There was a problem hiding this comment.
@Stevenmagdy no it won't block it. I'm on the minority when it comes using a CLI, so if others think that's the normal, i'm good
StevenSorial
force-pushed
the
runtime-download
branch
4 times, most recently
from
8da9e94
to
159c807
Compare
StevenSorial
force-pushed
the
runtime-download
branch
4 times, most recently
from
4e0eae7
to
b9de774
Compare
StevenSorial
force-pushed
the
runtime-download
branch
from
b9de774
to
af6351c
Compare
MattKiazyk
approved these changes
Nov 8, 2022
StevenSorial
force-pushed
the
runtime-download
branch
from
af6351c
to
ca37613
Compare
|
@MattKiazyk Also fixed #242 |
StevenSorial
force-pushed
the
runtime-download
branch
3 times, most recently
from
dc1bd09
to
39160d7
Compare
StevenSorial
force-pushed
the
runtime-download
branch
from
39160d7
to
a7633bd
Compare
|
The PR now even handles a theoretical edge case where two builds of the same OS are installed, it will add a build number to differentiate the two: |
|
First I would like to say I am REALLY looking forward to this feature. That would enable us to use xcodes for provisioning our CI/CD computers. And sorry if this is a spam message. I tested the branch to check if I can install older runtimes with it. But somehow it fails. This is my output: Maybe it is just some wrong configuration on my computer? Or is this not supposed to work as I assume? I have Xcode 16.1 currently selected. Running macOS 12.5.1. When I open this pkg like this |
|
Can you check if your terminal does have |
|
That was the solution! Thanks a lot. :-) |
|
@Stevenmagdy thanks for all this work - is it ready for another review? |
|
Yes, its ready for a review 👍🏼 |
MattKiazyk
approved these changes
Nov 18, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR builds on #223, and adds a command for installing runtimes
xcodes runtimes install 'iOS 14.5'Downloading will require an Apple account for new platforms released in 2022, it's not needed for older platforms.Installing is still a WIP