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

Moved XIP expansion to a temporal directory #179

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

juanjonol
Copy link
Contributor

@juanjonol juanjonol commented Jan 23, 2022

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.

  • It also makes more conceptual sense to keep a temporal file on the OS temporal folders.
  • 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 #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.

@juanjonol juanjonol requested a review from a team as a code owner January 23, 2022 17:30
@juanjonol
Copy link
Contributor Author

A few comments I didn't add to the commit:

  • This always changes the expansion directory, not just when the destination volume is different that the one where the XIP is. This means the XIP will never be expanded on ~/Library/Application Support/com.robotsandpencils.xcodes/ again.1 I did this for consistency and because I don't think there is anything useful that could be done with a partially-expanded Xcode version. This also allows macOS to automatically delete the expanded data if for some reason xcodes fails to move it.
  • @MattKiazyk suggested a change similar to other one on Xcodes.app, but I'm not sure if it makes sense here, for the same reasons as the previous point.

Footnotes

  1. Theoretically, url(for:in:appropriateFor:create:) can fail, and in that case the original code will be used, but I haven't found any case where it actually fails.

@MattKiazyk
Copy link
Contributor

@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).
@juanjonol juanjonol force-pushed the xip-multivolume-expasion branch from 276531b to 6fe3a3e Compare February 6, 2022 12:03
@juanjonol
Copy link
Contributor Author

@MattKiazyk I added a expand-xip-inplace flag to keep the old behaviour (simply using the directory where the .xip is). If you need anything else or you don't like how it's named/implemented, don't hesitate to tell me (I'm picky too!).

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.

@juanjonol
Copy link
Contributor Author

I've tested this installing Xcode 13.3 betas 1 and 2 and it worked perfectly.

@juanjonol juanjonol requested a review from MattKiazyk February 13, 2022 13:21
@juanjonol
Copy link
Contributor Author

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
@juanjonol
Copy link
Contributor Author

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
# 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expand .xip on the destination volume
2 participants