-
Notifications
You must be signed in to change notification settings - Fork 134
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
Moved XIP expansion to a temporal directory #179
base: main
Are you sure you want to change the base?
Conversation
A few comments I didn't add to the commit:
Footnotes
|
@juanjonol Thanks for the PR! It is appreciated! I love what you did here. I have one picky comment plus another ask. Can we add a flag onto the cli so that the user could (if they wanted) to NOT expand to the temporary directory and instead to where it was downloaded? I agree temporary is better, but just wanted to give that option to the user if they wanted to revert back to the old behaviour Thanks! |
Instead of expanding XIPs on its current directory, a temporal directory is used. The main advantage of this approach is that, because the temporal directory is created on the same volume where Xcode will be installed, the installation is vastly improved if the XIP is on a different volume. - https://nshipster.com/temporary-files/ - The implementation of the `temporalDirectory` function and variable are based on the similar `trashItem`, but without the `@discardableResult` annotation (I cannot think of any case where this URL could to be ignored). This closes XcodesOrg#178.
…de `Files` As suggested by @MattKiazyk, to be able to get the expansion directory by simply calling `Current.files.xcodeExpansionDirectory()`. - The `Files.temporalDirectory()` added on the previous commits is keep even if it's not used outside `Files.xcodeExpansionDirectory()`, to keep each function with a single statement (as the other functions on `Files`). - I'm not sure there's value on adding the `xcodeExpansionDirectory` variable, but added for consistency with the other functions on `Files`.
…rectory As suggested by @MattKiazyk, to allow the user to revert to the old behaviour if needed. - For testing this flag is set to `true`, to use a well-known location when testing. - The `xcodeExpansionDirectory` function cannot be easily expressed on a single line anymore, so removed the variable added on the previous commit (which wasn’t used anyway).
276531b
to
6fe3a3e
Compare
@MattKiazyk I added a Also, I though about adding tests, but I don't know how to test this. It's a small change so maybe it isn't needed, but if you have any idea about it, we can look into it. |
I've tested this installing Xcode 13.3 betas 1 and 2 and it worked perfectly. |
Hi @MattKiazyk, this PR has been stale for a few months. Could you please review it? I see there're conflicts now, but there's no point fixing them if the PR won't be merged... |
# Conflicts: # Sources/XcodesKit/XcodeInstaller.swift # Sources/xcodes/main.swift
I've updated this PR with the latest changes on main. All test are green and it can be merged without conflicts. @MattKiazyk could you please take a look at this and tell me if there's something preventing its merge? This still says that there're changes requested, but I made those changes months ago... |
# Conflicts: # Sources/XcodesKit/XcodeInstaller.swift # Sources/xcodes/main.swift # Tests/XcodesKitTests/XcodesKitTests.swift
afc0934
to
f4b45a8
Compare
# Conflicts: # Sources/XcodesKit/XcodeInstaller.swift # Sources/xcodes/App.swift
# Conflicts: # Sources/XcodesKit/Environment.swift # Sources/XcodesKit/XcodeInstaller.swift # Sources/xcodes/App.swift # Tests/XcodesKitTests/Environment+Mock.swift # Tests/XcodesKitTests/XcodesKitTests.swift
# Conflicts: # Sources/xcodes/App.swift
Instead of expanding XIPs on its current directory, a temporal directory is used. The main advantage of this approach is that, because the temporal directory is created on the same volume where Xcode will be installed, the installation is vastly improved if the XIP is on a different volume.
temporalDirectory
function and variable are based on the similartrashItem
, but without the@discardableResult
annotation (I cannot think of any case where this URL could to be ignored).This closes #178.
I've tested this on September 21st, to get some hard data about the improvement with this PR.
Installing Xcode 14.1 beta 2 (14B5024i) on a different volume that the one where the .xip is located, the
(3/6) Moving Xcode to /Volumes/<path>/Xcode-14-1-0-Beta.2.app
phase with the old implementation (using the--xip-expand-inplace
flag) takes 4.78 minutes on a 2021 MacBook Pro 14". Without--xip-expand-inplace
, this is virtually instantaneous (0.0009 seconds).For comparison, the improvement I get using
--experimental-unxip
over regular unxip is 1.33 minutes.