Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prioritize PATH executables over local directories in terminals #158666

Merged
merged 4 commits into from Sep 15, 2022
Merged

Prioritize PATH executables over local directories in terminals #158666

merged 4 commits into from Sep 15, 2022

Conversation

tobil4sk
Copy link
Contributor

@tobil4sk tobil4sk commented Aug 20, 2022

Right now, when validating an executable, the check to ensure the path is a file and not a directory comes before the check for executables in PATH. This means that a sub directory of the current process's PWD will take priority over an executable in PATH, and so an error will be given saying that the specified path "is not a file or a symlink", even if there is a valid executable in PATH which was intended to be executed.

This fix reorders the checks to ensure that the executable in PATH takes priority over a sub directory of the local PWD.

Fixes #106661 (or rather the root issue behind it, which was misdiagnosed in the thread. The error was not happening because the OS couldn't find dotnet in PATH, but because the dotnet folder in the home directory (which is usually set as the PWD when starting VSCode from an application menu) took priority over the dotnet executable that would be found in PATH.

To test that this issue is fixed:

  1. Have an executable available from $PATH, such as python
  2. Install an extension that has a task which runs an executable (Here is a minimal sample extension which runs python)
  3. Start VSCode by running code . in a directory which contains a sub directory with the same name as the executable (in this case, python)
  4. Run the task which runs the executable (called test-task in the sample extension I have uploaded)
  5. Validate that the python interactive cli starts, and there is no The terminal process failed to launch: Path to shell executable "python" is not a file or a symlink. error.

Also properly fixes #79346, which is the same issue.

Also fixes #83772

The steps to test this issue are the same as specified in the original issue (which can be replaced with any executable):

  1. Have dotnet in $PATH
  2. Open a folder
  3. Create a dotnet folder inside the root
  4. Set the terminal shell as dotnet

And then of course now verify that the terminal opens and there is no: The terminal process failed to launch: Path to shell executable "dotnet" is not a file or a symlink. error popup.

@ghost
Copy link

ghost commented Aug 20, 2022

CLA assistant check
All CLA requirements met.

@tobil4sk
Copy link
Contributor Author

tobil4sk commented Aug 20, 2022

I think there is still one potential issue with this solution, which is that findExecutable itself can sometimes return the path to a directory. In this case, the error won't be caught before the terminal launches. I'll push another commit to fix this.

EDIT: the error doesn't get caught on the main branch, but this PR actually prevents the error from not being caught.

@tobil4sk
Copy link
Contributor Author

Actually, that is not an issue with this solution, and is an unrelated problem which is already present on the main branch. I'll try to handle that in a separate issue/PR.

Right now, when validating an executable, the check to ensure the path
is a file and not a directory comes before the check for executables in
PATH. This means that a sub directory of the current process's PWD will
take priority over an executable in PATH, and so an error will be given
saying that the specified path "is not a file or a symlink", even if
there is a valid executable in PATH which was intended to be executed.

This fix reorders the checks to ensure that the executable in PATH takes
priority over a sub directory of the local PWD.
@Tyriar
Copy link
Member

Tyriar commented Sep 15, 2022

I see this when I try open a terminal:

  ERR EACCES: permission denied, stat 'C:\Users\Daniel\AppData\Local\Microsoft\WindowsApps\Microsoft.PowerShell_8wekyb3d8bbwe\pwsh.exe': Error: EACCES: permission denied, stat 'C:\Users\Daniel\AppData\Local\Microsoft\WindowsApps\Microsoft.PowerShell_8wekyb3d8bbwe\pwsh.exe'

Pushed a fix to cover that exception only and throw for others (which we can fix if they're hit)

@Tyriar
Copy link
Member

Tyriar commented Sep 15, 2022

@meganrogge second review please

@Tyriar Tyriar merged commit 6a5e3aa into microsoft:main Sep 15, 2022
@tobil4sk tobil4sk deleted the fix-terminal-executable-checks branch September 15, 2022 22:57
@tobil4sk
Copy link
Contributor Author

Great stuff, thanks for your time!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
4 participants