Toolchain selection

[matthewbastien]

Fixes #756

[adam-fowler]

Haven't looked at code in any detail as I'm doing this on my phone. I assume you have included a valid state where there is no toolchain.

Instead of checking do we have a toolchain everywhere why don't you create two stages to the extension initialisation one for the toolchain and one for the rest. At initialisation if you have no toolchain you are forced through the select toolchain UI and the rest of the extension doesn't initialize until a toolchain is either downloaded or selected.

[matthewbastien]

Haven't looked at code in any detail as I'm doing this on my phone. I assume you have included a valid state where there is no toolchain.

Instead of checking do we have a toolchain everywhere why don't you create two stages to the extension initialisation one for the toolchain and one for the rest. At initialisation if you have no toolchain you are forced through the select toolchain UI and the rest of the extension doesn't initialize until a toolchain is either downloaded or selected.

Fair point. I've made it so that the WorkspaceContext doesn't even get created if there's no available toolchain. This allows us to keep relatively the same behaviour as before this patch. However, the command registration now takes an optional WorkspaceContext and will display the toolchain error message if commands are invoked without a WorkspaceContext. I've also moved some of the functionality that doesn't require a toolchain outside of the WorkspaceContext (e.g. the reload extensions notification).

[adam-fowler]

So far tested this on macOS (Although I didn't test with multiple Xcode's installed) and works quite well. One minor issue, if you have a local toolchain and then select a new toolchain but set it globally you are still using the original local toolchain. I think you should probably clear the local setting here, or add another dialog to ask do you want to clear it.

I'll do a code review later, but I did see a lot of the code is still dealing with possibility of an undefined toolchain, folder context etc, which should be unnecessary now given your description of the changes you made.

[adam-fowler]

Here is an initial review of code. There is a load of code related to having an optional WorkspaceContext that can be reverted.

We need to make sure we get the UI flow correct for toolchain installation if no toolchain is currently installed.

[adam-fowler]

A few more comments, nothing major.

[adam-fowler]

Two minor nits, apart from that I think we are good

[matthewbastien]

So far tested this on macOS (Although I didn't test with multiple Xcode's installed) and works quite well. One minor issue, if you have a local toolchain and then select a new toolchain but set it globally you are still using the original local toolchain. I think you should probably clear the local setting here, or add another dialog to ask do you want to clear it.

I'll do a code review later, but I did see a lot of the code is still dealing with possibility of an undefined toolchain, folder context etc, which should be unnecessary now given your description of the changes you made.

So, I did some more investigation into this. It appears that settings for individual workspace folders are actually ignored for swift.path. I've removed the option to configure workspace folder settings and added a warning about workspace settings taking precedence over user settings.

@adam-fowler could you take one more look at the code and see if we're good?

[adam-fowler]

This all looks good

I have thought of one issue though, perhaps we can resolve this in another PR

If I select an Xcode install that isn't the Xcode install returned by xcode-select -p (default). The calls to SwiftToolchain.getLLDB and SwiftToolchain.getSDKPath return the wrong paths ie from the default install.

Should we set DEVELOPER_DIR environment var if we are setting the path to an Xcode install to ensure we get the correct LLDB and SDK path

[matthewbastien]

This all looks good

I have thought of one issue though, perhaps we can resolve this in another PR

If I select an Xcode install that isn't the Xcode install returned by xcode-select -p (default). The calls to SwiftToolchain.getLLDB and SwiftToolchain.getSDKPath return the wrong paths ie from the default install.

Should we set DEVELOPER_DIR environment var if we are setting the path to an Xcode install to ensure we get the correct LLDB and SDK path

Good catch. I fixed the XCTest path, but did not do the same for these two. I'll merge this PR and have a look at fixing the issue in another PR.