debugger: pass along SDKROOT for the launch of the debugger

[compnerd]

This is used by the debugger to locate the Swift module information for
the debugger. Without this, we would fail to setup the Swift scratch
space in LLDB, which then results in the failure of the debugger and
subsequently the DAP.

Fixes: #250

[adam-fowler]

This shouldn't be necessary as tests/applications inherit their environment from their parent process ie vscode.

Is this really all that is needed to resolve launching with the swift LLDB? It doesn't make sense

[stevapple]

Is SDKROOT the environment variable something necessary for Swift LLDB to work, or it is just used to pass along the value? We can have SDKROOT set up with custom host SDK in launch profile if it’s the former one. The latter case could be handled by hardcoding the init command directly.

[compnerd]

Is this really all that is needed to resolve launching with the swift LLDB? It doesn't make sense

Yes, with this and the other change, I’m able to get lldb to run with the Swift liblldb.

Is SDKROOT the environment variable something necessary for Swift LLDB to work, or it is just used to pass along the value? We can have SDKROOT set up with custom host SDK in launch profiles if it’s the former one. The latter case could be handled by hardcoding the init command directly.

SDKROOT is never necessary in any context - it is merely a fallback to provide the value for -sdk (just to hide the need to write the argument). I don’t know the code lldb behavior well enough so I took the conservative approach. There is a comment in the change that would set it through commands. That is trivial to switch to if the ordering is not something we need to worry about. I would prefer the non-environment approach if possible (in general).

What do you mean by custom host SDK?

[stevapple]

What do you mean by custom host SDK?

Custom SDKROOT override, mainly used to switch to alternative toolchain & SDK without having to change the global environment. See #266.

SDKROOT is never necessary in any context - it is merely a fallback to provide the value for -sdk (just to hide the need to write the argument).

SDKROOT, initially designed to override the default host SDK, is not meant to provide SDK for the target (use -sdk explicitly in such cases). It points to the SDK for the host. For example, when %SDKROOT% and --sdk <sdk> are both set, the manifest should be built against the host SDK, and the package itself should be built against the target SDK.

Since we’re debugging the target executable, I don’t think we should pass SDKROOT here. What debugger really need to know should be the target SDK instead?

[adam-fowler]

Yes, with this and the other change, I’m able to get lldb to run with the Swift liblldb.

Which other change is that? The setting of the xctest.dll path? #276

[compnerd]

Which other change is that? The setting of the xctest.dll path? #276

The change to remove the "do not launch lldb on Windows" check.

[compnerd]

What do you mean by custom host SDK?

Custom SDKROOT override, mainly used to switch to alternative toolchain & SDK without having to change the global environment. See #266.

Ah, having the ability to specify the SDKROOT instead of relying on the environment is definitely a good thing to have.

SDKROOT is never necessary in any context - it is merely a fallback to provide the value for -sdk (just to hide the need to write the argument).

SDKROOT, initially designed to override the default host SDK, is not meant to provide SDK for the target (use -sdk explicitly in such cases). It points to the SDK for the host. For example, when %SDKROOT% and --sdk <sdk> are both set, the manifest should be built against the host SDK, and the package itself should be built against the target SDK.

I'm not sure that there is value here. While I would certainly like to have different SDKs, the Android SDK is currently not in a state where packaging is reasonable. The Android build is now "broken" - it has conflated Android and termux and this will need to be cleaned up. Even when the Android SDK is available, we will need additional environment variables (e.g. NDKROOT). Furthermore, the way to specify that is via the destination.json rather than the environment variable, which entirely avoids the issue that you are thinking of.

Furthermore, if SDKROOT and -sdk are both specified, SDKROOT is ignored. SDKROOT is a last chance, we don't know where the system "headers" are, let's just try this. Unlike the constrained environments of Unix, Windows gives you far more freedom to layout things as you wish, and so you cannot assume that you can just build against /usr/lib/swift or /usr/include. The -sdk argument tells you where the content is.

Since we’re debugging the target executable, I don’t think we should pass SDKROOT here. What debugger really need to know should be the target SDK instead?

Yes, the value that should be set is the host SDK path.

[adam-fowler]

Which other change is that? The setting of the xctest.dll path? #276

The change to remove the "do not launch lldb on Windows" check.

oh ok. You didn't need to add any lldb commands into the launch.json?

[compnerd]

Well, that's the thing that I think that @stevapple and I were discussing above. I think that @stevapple would prefer that we don't use the environment ... and in principle I agree with that. I don't like the environment approach either. I would prefer that we uncomment https://github.com/swift-server/vscode-swift/pull/275/files#diff-48e8c0b41f2583e0ffbaf19954e30331945a5731890f0a3af3f975b18e0f154eR174 and remove the environment variable. The problem is that I don't understand how the LLDB plugin functions and I don't know if we need to worry about ordering of commands if the user wants to add custom ones. If someone knows if it is safe to inject this and it will auto-merge the user specified settings, then I think that is a better solution.

Then, once we have the ability to specify the SDKROOT as a setting in VSCode, we should change this to pipe that value through rather than the environment variable.

[stevapple]

We're only updating the launch config in very limited situations (mainly on configuration changes), so I guess we can assume this is relatively safe to cover (with prompt on users).

We cannot use Swift plugin configuration in launch.json, so the value should be hardcoded in the command instead.

[adam-fowler]

We're only updating the launch config in very limited situations (mainly on configuration changes), so I guess we can assume this is relatively safe to cover (with prompt on users).

We cannot use Swift plugin configuration in launch.json, so the value should be hardcoded in the command instead.

There is a difference between the launch configuration generation and usage for debugging/running tests and debugging applications.

• The application launch configs are saved into launch.json, it can use power shell environment variables and only gets updated infrequently (as @stevapple points out above).
• The launch config for tests are not saved into launch.json, they are built on the fly whenever you run tests and cannot use power shell environment variables.

This PR only updates the test launch config. I guess it will also need to update the application launch configs, where it can use a power shell env var. I think I would prefer the lldb command route as well. @compnerd Out of interest is there any difference to using initCommands over preRunCommands.

[compnerd]

I think that we are talking about different things. I am talking about the merging of the commands for the LLDB plugin. In the CodeLLDB plugin settings, you can define a set of items for them. I don't know how they interact with the overridden values here.

@compnerd Out of interest is there any difference to using initCommands over preRunCommands.

Yes, there is a big difference and why I use the preRunCommands. They are run at different times, and the initCommands contains the process creation by means of target create ... that will need to be preserved. That does not get merged implicitly. This is why I assume that if we are overriding the settings, we need to handle that ourselves, and I don't know how to do that. This is why I did the SDKROOT environment variable, even though I think that this is less desirable.

[adam-fowler]

I think that we are talking about different things. I am talking about the merging of the commands for the LLDB plugin. In the CodeLLDB plugin settings, you can define a set of items for them. I don't know how they interact with the overridden values here.

I did think about using the settings (it is cleaner, if those settings are static) but if we have an editable SDKROOT it might be awkward to keep the lldb settings in-sync

[compnerd]

I think that we are talking about different things. I am talking about the merging of the commands for the LLDB plugin. In the CodeLLDB plugin settings, you can define a set of items for them. I don't know how they interact with the overridden values here.

I did think about using the settings (it is cleaner, if those settings are static) but if we have an editable SDKROOT it might be awkward to keep the lldb settings in-sync

The preRunCommands are those settings, and that awkwardness is the problem that I have with this too - which is why I begrudgingly used the environment to pass along the information.

[adam-fowler]

@compnerd We need to add this to createExecutableConfigurations as well. Once that is in place I'll merge both this and #262