xcodes icon indicating copy to clipboard operation
xcodes copied to clipboard

Moved XIP expansion to a temporal directory

Open juanjonol opened this issue 4 years ago • 6 comments

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 avatar Jan 23 '22 17:01 juanjonol

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.

[^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.

juanjonol avatar Jan 23 '22 17:01 juanjonol

@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!

MattKiazyk avatar Jan 28 '22 17:01 MattKiazyk

@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 avatar Feb 06 '22 12:02 juanjonol

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

juanjonol avatar Feb 13 '22 13:02 juanjonol

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...

juanjonol avatar May 15 '22 09:05 juanjonol

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...

juanjonol avatar Sep 18 '22 09:09 juanjonol