Fix disable native toolset install logic

[chcosta]

No description provided.

[chcosta]

Coreclr validation build - https://dev.azure.com/dnceng/public/_build/results?buildId=410692&view=results

[sunandabalu]

Probably a good idea a kick off a Winforms build, since that was the original issue.

[sunandabalu]

Change itself looks fine to me.

[chcosta]

Technically, they don't exercise this particular code path, but I agree that more validation is good and it would be bad to unintentionally regress them yet again. Thanks for the note, I created dotnet/winforms#2260 to validate winforms.

[ViktorHofer]

You are right, I accidently reverted the condition. Are you sure that you would do a Test-Path on a boolean in powershell? Doesn't a -ne suffice?

[chcosta]

The code is following a quirky pattern. Essentially, "DisableNativeToolsetInstalls" is being used as a switch rather than a boolean. If it's present, then the flag is enabled. Meaning, DisableNativeToolsetInstalls=$true and DisableNativeToolsetInstalls=$false are equivalent. Another option would be to do something like

If((test-path variable:DisableNativeToolsetInstalls) - and $DisableNativeToolsetInstalls)

That would be the more expected behavior. I won't be able to do any validation or make any changes on this until tomorrow.

[ViktorHofer]

@chcosta @sunandabalu I just did a thorough root analysis of the bugs introduced in 7a82f9a:

1. The config-toolset.sh import is broken, the file isn't imported anymore at all. This impacts not just native toolset bootstrapping but also cli and nuget cache settings. The null comparison was wrong.
2. The condition which checks on the variable DisableNativeToolsetInstalls was inverted. It took me a while to understand the whole syntax of the line: if [[ -z "${DisableNativeToolsetInstalls:-}" ]]; then. The variable expansion :- sets the variable to $null if the variable is not defined. The -z option checks if the string is null, that is, has zero length. With that logic the toolset bootstrapping would be skipped if the variable IS NOT defined which we don't want. The fix is to use the -n option instead which checks if the string is not null.
3. The condition to check the if ($env:DisableNativeToolsetInstalls) variable in the tools.ps1 script was wrong which resulted in the exit branch never to be reached. The correct usage, as Chris mentioned is to check via Test-Path and with -eq operations. My first PR unfortunately failed doing this correctly, thanks Chris for letting me know.

I submitted the fixes into this PR and validated it offline on both corefx and the runtime repository.

[ViktorHofer]

Merging to unblock repos which might have been broken by my previous attempt.

[riarenas]

Is there a scenario we should add to the arcade-validation repo that would have caught this, and would catch similar problems going forward?

[ViktorHofer]

Yes that would be good if we want to rely on the native toolset bootstrapping goind forward.

[chcosta]

I think adding unit tests to arcade for our powershell scripts would be my preferred approach. I'm uncertain what the state of unit testing bash scripts is though