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

Fix Visual Studio Code build task give 'The terminal shell path “dotnet” is a directory' #3372

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dauphine-dev
Copy link

Fix Visual Studio Code build task give 'The terminal shell path “dotnet” is a directory'
See:
https://stackoverflow.com/questions/58072505/visual-studio-code-build-task-give-the-terminal-shell-path-dotnet-is-a-direct/5

@dnfclas
Copy link

dnfclas commented Oct 24, 2019

CLA assistant check
All CLA requirements met.

@gregg-miskelly
Copy link
Contributor

This seems reasonable to me, but I am not sure if there is any downside of running dotnet instead a shell instead of a process?

@dauphine-dev
Copy link
Author

I run dotnet as shell, and until now I haven't detected any downside.
And also I run often directly in shell console too, and it works well.

@gregg-miskelly
Copy link
Contributor

@dbaeumer this PR is proposing to change the tasks.json tasks generated by the C# extension to use shell instead of process because process does the "wrong" thing if there happens to be a directory with the same name as the process we are launching. Are you aware of anything we should be careful to test or any extra downside of using shell? I am assuming it is ever so slightly slower since now the shell has to spin up, but that perf penalty should be small?

@dbaeumer
Copy link

dbaeumer commented Oct 28, 2019

@gregg-miskelly besides spinning up the shell there is a different interpretation of the arguments. In process they go directly to the process and with shell they are subject to shell interpretation logic.

What is the underlying reasons of having process fail in this setup.

And in general you should use task providers now and not adding tasks to the tasks.json file.

@gregg-miskelly
Copy link
Contributor

@dbaeumer The underlying problem with process is this -- we are trying to execute the dotnet executable. With process if PATH contains a directory that has a sub directory named dotnet then the process code will try to execute that subdirectory instead of ignoring it and continuing the PATH search to find the executable. The path searching code inside of (most?) shells seems to be a bit smarter than this and will ignore directories.

For using task providers instead -- you are suggesting this as a way of simplifying the tasks.json content as well as making it possible to change without requiring users to regenerate their tasks.json file? In our case, if I understand things correctly, I believe users would still need a tasks.json file since users need to specify the project file to build. But certainly we could use task providers to make this a bit simpler.

@dbaeumer
Copy link

@gregg-miskelly I assume this only happens under Windows :-) since there we have our own code to find the executable. The problem lies here:

https://github.com/Microsoft/vscode/blob/master/src/vs/base/node/processes.ts#L467

This should not do an exist. It should do a stat and then check if it is a file.

I opened microsoft/vscode#83509 and we should try to fix this for October.

@dauphine-dev
Copy link
Author

@dbaeumer
For information, I am running Visual Studio Code under Linux (Mint 19.2 Tina / Cinnamon)

@dbaeumer
Copy link

@alexr00 do we execute the code under Linux as well ?

@alexr00
Copy link

alexr00 commented Oct 30, 2019

It looks like the terminal might be. @dauphine-dev could you install the latest Insiders build of VS Code and see if you're still seeing the issue?

@dauphine-dev
Copy link
Author

@alexr00
With latest Insiders build (1.40.0-insider), I have still the issue, and I get that:

> Executing task: dotnet build /home/david/test/test.csproj /property:GenerateFullPaths=true /consoleloggerparameters:NoSummary <

The terminal shell path "dotnet" is a directory

Terminal will be reused by tasks, press any key to close it.

@connor4312
Copy link

@dauphine-dev what's the date or commit hash of the VS Code version you're using? I'm able to repro on stable, but not on today's Insiders build, using this task config (where my dotnet.exe is at C:\Program Files\dotnet\dotnet.exe):

        {
            "label": "build",
            "command": "dotnet",
            "options": {
                "env": {
                    "PATH": "C:\\Program Files;C:\\Program Files\\dotnet"
                }
            },
            "type": "process",
            "args": [
                "build",
                "${workspaceFolder}/my.csproj", // replace this
                "/property:GenerateFullPaths=true",
                "/consoleloggerparameters:NoSummary"
            ],
            "problemMatcher": "$msCompile"
        },

@dauphine-dev
Copy link
Author

dauphine-dev commented Oct 31, 2019

@connor4312

From the about menu:

Version: 1.40.0-insider
Commit: 31f577ec88dc96ad2028699fb597b19022224b46
Date: 2019-10-30T05:32:25.621Z
Electron: 6.1.2
Chrome: 76.0.3809.146
Node.js: 12.4.0
V8: 7.6.303.31-electron.0
OS: Linux x64 4.15.0-66-generic

I will try again the .deb installer from today.

Edit:
I have just test with the last version, and I still have the issue.

Version:

Version: 1.40.0-insider
Commit: 93ee2fc3121b7f66ddf568c051f3bfff7db8d618
Date: 2019-10-31T06:28:32.160Z
Electron: 6.1.2
Chrome: 76.0.3809.146
Node.js: 12.4.0
V8: 7.6.303.31-electron.0
OS: Linux x64 4.15.0-66-generic

tasks.json

{
    "version": "2.0.0",
    "tasks": [
        {
            "label": "build",
            "command": "dotnet",
            "type": "process",
            "args": [
                "build",
                "${workspaceFolder}/test1.csproj",
                "/property:GenerateFullPaths=true",
                "/consoleloggerparameters:NoSummary"
            ],
            "problemMatcher": "$msCompile"
        },
        {
            "label": "publish",
            "command": "dotnet",
            "type": "process",
            "args": [
                "publish",
                "${workspaceFolder}/test1.csproj",
                "/property:GenerateFullPaths=true",
                "/consoleloggerparameters:NoSummary"
            ],
            "problemMatcher": "$msCompile"
        },
        {
            "label": "watch",
            "command": "dotnet",
            "type": "process",
            "args": [
                "watch",
                "run",
                "${workspaceFolder}/test1.csproj",
                "/property:GenerateFullPaths=true",
                "/consoleloggerparameters:NoSummary"
            ],
            "problemMatcher": "$msCompile"
        }
    ]
}

Issue:

> Executing task: dotnet build /home/david/test1/test1.csproj /property:GenerateFullPaths=true /consoleloggerparameters:NoSummary <

The terminal shell path "dotnet" is a directory

@connor4312
Copy link

I eventually managed to reproduce this, somehow, but it's quite frustrating. I got it to happen on my Insiders built, and it now happens consistently there. But then when opening the same workspace with the OSS build (where I can actually debug), even on the same commit that Insiders was built from, I don't run into the issue, and I can't seem to get it to repro again. Trace-level logs in vscode aren't very useful, and auditd doesn't provide much useful information here. It does appear to be an issue somewhere in vscode itself, rather than this extension.

[2019-10-31 09:00:12.334] [renderer1] [trace] terminalInstance#ctor (id: 1) {"name":"Task - build","executable":"dotnet","args":["build","/home/connor/Downloads/TestApp/TestApp.csproj","/property:GenerateFullPaths=true","/consoleloggerparameters:NoSummary"],"waitOnExit":"Terminal will be reused by tasks, press any key to close it.","initialText":"\u001b[1m> Executing task: dotnet build /home/connor/Downloads/TestApp/TestApp.csproj /property:GenerateFullPaths=true /consoleloggerparameters:NoSummary <\u001b[0m\n","cwd":{"$mid":1,"path":"/home/connor/Downloads/TestApp","scheme":"file"},"env":{"DOTNET_ROOT":"/home/connor/Downloads/TestApp","PATH":"/home/connor/Downloads/TestApp:/usr/bin"}}

@Tyriar
Copy link
Contributor

Tyriar commented Oct 31, 2019

@connor4312
Copy link

connor4312 commented Oct 31, 2019

I think that's it. I had created ~/dotnet at one point while fiddling around, but I didn't reference it in the task.json at all so I assumed it was irrelevant a few minutes later when I got a reproduction. It looks like I can consistently repro when ~/dotnet is present, and cannot do so if I remove the folder. Launching OSS from VS Code, apparently, sets the termcwd differently.

@alexr00
Copy link

alexr00 commented Oct 31, 2019

The task system should really be resolving that before it hands over to the terminal. I'll try that repro.
Nope, it shouldn't we only find executable for windows.
@Tyriar, is this something you want to change in the terminal, or is this expected behavior?

@Tyriar
Copy link
Contributor

Tyriar commented Oct 31, 2019

@alexr00 it's a bug in the terminal too though; if it's a dir it should still check the path not fail there. microsoft/vscode#83772

@alexr00
Copy link

alexr00 commented Nov 1, 2019

@Tyriar makes sense!

Summary: The issue occurred when the command of a process task was also the name of a folder. It is fixed in VS Code Insiders for Windows and that fix will also be in the 1.40 stable release. On non-Windows, the issue still occurs when you have a folder in your project with the same name as the task command. The fix for non-Windows will not be in 1.40, but will probably come soon.

@dauphine-dev
Copy link
Author

On non-Windows, the issue still occurs when you have a folder in your project with the same name as the task command.

I am not sure about that.
This is the tree of my folder:

.
├── bin
│   └── Debug
│       └── netcoreapp3.0
├── obj
│   ├── Debug
│   │   └── netcoreapp3.0
│   │       ├── test1.AssemblyInfo.cs
│   │       ├── test1.AssemblyInfoInputs.cache
│   │       ├── test1.assets.cache
│   │       └── test1.csprojAssemblyReference.cache
│   ├── project.assets.json
│   ├── test1.csproj.nuget.cache
│   ├── test1.csproj.nuget.dgspec.json
│   ├── test1.csproj.nuget.g.props
│   └── test1.csproj.nuget.g.targets
├── Program.cs
└── test1.csproj

And my task.json:

{
    "version": "2.0.0",
    "tasks": [
        {
            "label": "build",
            "command": "dotnet",
            "type": "process",
            "args": [
                "build",
                "${workspaceFolder}/test1.csproj",
                "/property:GenerateFullPaths=true",
                "/consoleloggerparameters:NoSummary"
            ],
            "problemMatcher": "$msCompile"
        },
        {
            "label": "publish",
            "command": "dotnet",
            "type": "process",
            "args": [
                "publish",
                "${workspaceFolder}/test1.csproj",
                "/property:GenerateFullPaths=true",
                "/consoleloggerparameters:NoSummary"
            ],
            "problemMatcher": "$msCompile"
        },
        {
            "label": "watch",
            "command": "dotnet",
            "type": "process",
            "args": [
                "watch",
                "run",
                "${workspaceFolder}/test1.csproj",
                "/property:GenerateFullPaths=true",
                "/consoleloggerparameters:NoSummary"
            ],
            "problemMatcher": "$msCompile"
        }
    ]
}

Commands are dotnet and I haven't any "dotnet" folder

@tobil4sk
Copy link

This should now be fixed for non-windows systems as well in the next insider build: microsoft/vscode#158666.

The issue only occured when the process PWD contained the folder with the same name as the command, which isn't always necessarily the project directory, and might for example be the user's home directory instead.

@georgedimac
Copy link

it's still an issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants