XcodesOrg / xcodes

The best command-line tool to install and switch between multiple versions of Xcode.
MIT License
3.66k stars 122 forks source link

Moved XIP expansion to a temporal directory #179

Open juanjonol opened 2 years ago

juanjonol commented 2 years ago

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.

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 commented 2 years ago

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

[^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 commented 2 years ago

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

juanjonol commented 2 years ago

@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 commented 2 years ago

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

juanjonol commented 2 years ago

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 commented 1 year ago

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