fix: remove python usage in macOS cli wrapper

[deepak1556]

Fixes #134635
Fixes #137954

[deepak1556]

Pending build for verification https://monacotools.visualstudio.com/Monaco/_build/results?buildId=145742&view=results

[joaomoreno]

@deepak1556 What are your thoughts on #137954 ?

[deepak1556]

Thanks for the pointer! I agree with the sentiment in that issue, if there is a simpler non-python way to get the absolute path it would also make this script more resilient. But I have to try it out, to confirm if it covers all cases. Let me try to adjust this PR in that front.

[stephanierifai]

Hi there! Is there an ETA on this? This would help us with a related issue folks have been hitting on Docker Desktop so appreciate the work on it.

[StefanScherer]

I don't know the history well enough, but I wonder if we can just drop the dependency on python at all. 🤔

What does scripts/code.sh do different? It seems it tries to solve the same problem, but with just plain bash.
See https://github.com/microsoft/vscode/blob/main/scripts/code.sh#L5-L8

if [[ "$OSTYPE" == "darwin"* ]]; then
	realpath() { [[ $1 = /* ]] && echo "$1" || echo "$PWD/${1#./}"; }
	ROOT=$(dirname "$(dirname "$(realpath "$0")")")
else

It also seems that Apple doesn't install Python 3 by default in Monterey 12.3. See eg. https://twitter.com/manomarks/status/1486845980527894538?s=21 🤔

[deepak1556]

What does scripts/code.sh do different?

It does try to achieve the same effect but that won't work for symlink cases, resources/darwin/bin/code.sh will usually get symlinked to default binary search path /usr/local/bin so that code command is available to users. We need a bash solution that will also work for this case.

[deepak1556]

Change works fine, tested with https://monacotools.visualstudio.com/Monaco/_build/results?buildId=152892&view=results

@joaomoreno PTAL, thanks!

[deepak1556]

This PR does not handle multiple installation cases for the same bundle identifier, I am sure if there is a wide use case for it.

[joaomoreno]

This PR does not handle multiple installation cases for the same bundle identifier, I am sure if there is a wide use case for it.

Yeah you make a good point. I'm guessing mdfindwill just pick a random installation, if two different versions of the same product are found in disk... I'm not too excited about that.

[deepak1556]

I think we can make a safe fallback case with e9af584 if there are multiple results from mdfind. (ie)

1. Use mdfind when there is exactly one result
2. Fallback to sudo realpath implementation otherwise

thoughts ?

[joaomoreno]

I don't really know. All I know is we went with Python because no matter how hard I tried, I couldn't find a pure shell solution that worked across shells/macOS versions, at the time. If you have confidence in the fallback logic and are up for taking the theoretical issue stream that might arise from it, then I approve 👌

[deepak1556]

You are right, I found myself in the same path when writing this PR. But given the python2 deprecation makes our cli completely useless on macOS monterey for many users, I will go ahead and try this #138582 (comment), will see if I can improve it later based on user feedback.

[n8felton]

I'm not sure that using mdfind should be an acceptable solution as there are plenty of instances where Spotlight may be completely disabled, or if VScode is installed to a location that is excluded from Spotlight.

I recommend using one of the readlink solutions[1] proposed[2] as it will use the actual symlink used to call code, which helps to simulate what the python realpath code was doing, rather than assume whatever app bundle is found with mdfind.

[Tyriar]

The pure bash approach seems like the way to go from my perspective. Since there's a bash shebang it should be guaranteed that the script will run under bash, you just need to make sure you support the lowest expected version of bash shipped in macOS.

I'm not the best with bash scripts so my review of the actual scripts probably wouldn't be too useful.

[deepak1556]

Verified locally, merging for insiders testing.

[aeschli]

related: microsoft/vscode-remote-release#6442

[deepak1556]

Thanks for addressing it @aeschli !!