Add select option to install #182

tahirmt · 2022-02-04T05:32:31Z

Adds options --select and --update to install command.

Details

Uses Exit immediately if trying to select currently selected xcode #181 as base because without that the --select option will always ask user for password even if the Xcode version is already selected
I'm not that fluent in PromiseKit so happy to make any improvements
Creating this as a draft until we can get Exit immediately if trying to select currently selected xcode #181 merged
Closes [Feature Request] xcodes install *** --select #161

Kingkhalifa195

Will try to check this out

Kingkhalifa195 · 2022-03-16T00:58:43Z

Sources/XcodesKit/XcodeInstaller.swift

@@ -164,6 +164,11 @@ public final class XcodeInstaller {
            Current.shell.exit(0)
        }
    }
+
+    /// Perform the install but don't exit out but return the installed xcode version as output instead
+    public func installWithoutLogging(_ installationType: InstallationType, dataSource: DataSource, downloader: Downloader, destination: Path) -> Promise<InstalledXcode> {


I keep having set back anytime I installed it

Could you please elaborate?

rogerluan · 2022-09-19T11:04:28Z

@tahirmt thank you for this PR! This makes total sense to me 💯

@MattKiazyk this would also be aligned with xcpretty/xcode-install#467, since xcode-install's default behavior automatically selects the Xcode version that was just installed, and it would be a huge improvement over xcode-install and xcodes if we could also optionally run update automatically right before installing (otherwise Xcode versions that exist might not be found). I had to work-around this behavior implementing a "pre-install update" behavior here but it'd be great if it was built-in into xcodes ☺️

I'll review this PR to help move it forward 💪

MattKiazyk · 2022-09-19T23:05:43Z

Sources/xcodes/main.swift

+                        return selectXcode(shouldPrint: print, pathOrVersion: version, directory: destination, fallbackToInteractive: false)
+                    } else {
+                        return Promise { _ in
+                            throw InstallError.notAlreadyInstalled


I'm not a huge fan of throwing just to get to a catch. Can you refactor this a bit so that it's a bit simpler around the different options. That may involved some new functions to make things a bit cleaner.

Let me know if you need some help around this. Thanks

I did some refactor. Let me know what you think.

I think unit testing is where I'm kind of lost and will require some help

MattKiazyk

Thanks for the PR!

There's some coding issues around how the select parameter gets passed through. Made some comments

MattKiazyk · 2022-09-29T02:54:25Z

Sources/xcodes/main.swift

+
+            if select, case .version(let version) = installation {
+                firstly {
+                    selectXcode(shouldPrint: print, pathOrVersion: version, directory: destination, fallbackToInteractive: false)


I think this is incorrect - we should install first, then select after.

This was done to only select if it is already installed. If the version isn't installed this will throw and then we can install it.

The new select flag on the install is "Select the installed xcode version after installation." which its not doing.

You're checking if the select flag is passed in, trying to select the new xcode (that isn't installed) and then installing it.

I was thinking of a use case where on CI you want to ensure that a certain Xcode version is installed. So you could do xcodes install x.x.x --select --update. If it's already installed you would expect it to be selected.

So with all of these options if we try to select first, we exit super early by just selecting the version. But if we don't, it will first update the list, then see that it is already installed and select it. The list update will always happen even if Xcode was already installed and it wasn't needed. If that's the behavior you prefer I can remove it.

MattKiazyk · 2022-09-29T02:55:40Z

Sources/xcodes/main.swift

+                Current.logging.log("\nXcode \(xcode.version.descriptionWithoutBuildMetadata) has been installed to \(xcode.path.string)".green)
+
+                // Install was successful, now select it
+                return selectXcode(shouldPrint: print, pathOrVersion: xcode.path.string, directory: destination, fallbackToInteractive: false)


The selectXcode here isn't looking at the select option passed in.

Actually, this line will not get executed if select is false because install would exit early from the step before. That's why I don't have a check here.

Unless I missed something in how Current.shell.exit(0) works.

Yep I tested it out

➜ xcodes git:(add-select-option-to-install) swift run xcodes install 14.0.1 --experimental-unxip Building for debugging... Build complete! (0.08s) Downloading with aria2 (1/6) Downloading Xcode 14.0.1+14A400: 99% (2/6) Unarchiving Xcode (This can take a while) Using experimental unxip. If you encounter any issues, remove the flag and try again (3/6) Moving Xcode to /Applications/Xcode-14.0.1.app (4/6) Moving Xcode archive Xcode-14.0.1+14A400.xip to the Trash (5/6) Checking security assessment and code signing (6/6) Finishing installation xcodes requires superuser privileges in order to finish installation. macOS User Password: Xcode 14.0.1 has been installed to /Applications/Xcode-14.0.1.app

ah, i see now.. can we refactor that a bit.

Can we remove the exitAfterInstall property on the installer.install. It's a bit confusing on doing it there if in the end we are using that to skip the select

Instead can we pass the select option inside this function and the last step check that, selecting if user wants.

I refactored this.

rogerluan · 2022-09-29T11:04:12Z

@tahirmt since #181 got merged, would you mind rebasing this branch or merging the main branch into this one? 🙇

MattKiazyk · 2022-10-05T02:42:55Z

Sources/xcodes/main.swift

+                }
+            }
+            .then { xcode -> Promise<Void> in
+                select ? selectXcode(shouldPrint: print, pathOrVersion: xcode.path.string, directory: destination, fallbackToInteractive: false) : .init()


Sorry one more #picky - can we if else on this select instead of ? :

tahirmt

If else

Sources/xcodes/main.swift

MattKiazyk

Thanks for the changes and willingness to wait.

jokerputihboy

Tepelekek

Kingkhalifa195 reviewed Mar 16, 2022

View reviewed changes

tahirmt force-pushed the add-select-option-to-install branch 2 times, most recently from 0d0ac3c to 8061e41 Compare March 17, 2022 18:27

MattKiazyk requested changes Sep 19, 2022

View reviewed changes

tahirmt force-pushed the add-select-option-to-install branch from 8061e41 to a22f4db Compare September 20, 2022 02:10

rogerluan mentioned this pull request Sep 20, 2022

Exit immediately if trying to select currently selected xcode #181

Merged

MattKiazyk requested changes Sep 29, 2022

View reviewed changes

tahirmt force-pushed the add-select-option-to-install branch from 60e66c8 to 873ae9d Compare September 29, 2022 11:25

tahirmt marked this pull request as ready for review September 29, 2022 11:26

tahirmt requested a review from a team as a code owner September 29, 2022 11:26

MattKiazyk reviewed Oct 5, 2022

View reviewed changes

tahirmt commented Oct 5, 2022

View reviewed changes

Sources/xcodes/main.swift Outdated Show resolved Hide resolved

tahirmt and others added 9 commits October 7, 2022 22:24

Add select and update options to install

5450065

Fix build errors after rebase

0a5eeb1

Code cleanup and refactor

7c365df

More cleanup

012d155

Remove duplicate log

c947a8b

Refactor install function and exit call

70c0c25

Small refactor

fe3b37b

Remove unused enum

f1025cc

Formatting

77d2e29

tahirmt force-pushed the add-select-option-to-install branch from 94b01c2 to 77d2e29 Compare October 8, 2022 02:25

tahirmt requested a review from MattKiazyk October 8, 2022 02:28

MattKiazyk approved these changes Oct 26, 2022

View reviewed changes

MattKiazyk added the enhancement New feature or request label Oct 26, 2022

MattKiazyk merged commit f9771ca into XcodesOrg:main Oct 26, 2022

tahirmt deleted the add-select-option-to-install branch October 26, 2022 03:25

rogerluan mentioned this pull request Nov 1, 2022

[action] add xcodes action, deprecating xcversion and xcode-install fastlane/fastlane#20672

Merged

5 tasks

jokerputihboy approved these changes Nov 27, 2024

View reviewed changes

jokerputihboy mentioned this pull request Nov 27, 2024

""jtboy jokerputihboy/ALBETH-ARDA-BELLY-#2

Open

jokerputihboy approved these changes Dec 2, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add select option to install #182

Add select option to install #182

tahirmt commented Feb 4, 2022 •

edited

Loading

Kingkhalifa195 left a comment

Kingkhalifa195 Mar 16, 2022

tahirmt Mar 16, 2022

rogerluan commented Sep 19, 2022

MattKiazyk Sep 19, 2022

tahirmt Sep 20, 2022

tahirmt Sep 20, 2022

MattKiazyk left a comment

MattKiazyk Sep 29, 2022

tahirmt Sep 29, 2022

MattKiazyk Sep 29, 2022

tahirmt Sep 29, 2022

MattKiazyk Sep 29, 2022

tahirmt Sep 29, 2022

tahirmt Sep 29, 2022 •

edited

Loading

tahirmt Sep 29, 2022

MattKiazyk Oct 1, 2022

tahirmt Oct 1, 2022

rogerluan commented Sep 29, 2022

MattKiazyk Oct 5, 2022

tahirmt left a comment

MattKiazyk left a comment

jokerputihboy left a comment

Add select option to install #182

Add select option to install #182

Conversation

tahirmt commented Feb 4, 2022 • edited Loading

Details

Kingkhalifa195 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rogerluan commented Sep 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MattKiazyk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tahirmt Sep 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rogerluan commented Sep 29, 2022

Choose a reason for hiding this comment

tahirmt left a comment

Choose a reason for hiding this comment

MattKiazyk left a comment

Choose a reason for hiding this comment

jokerputihboy left a comment

Choose a reason for hiding this comment

tahirmt commented Feb 4, 2022 •

edited

Loading

tahirmt Sep 29, 2022 •

edited

Loading