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

[WIP] Use WSL PHP for validation #25576

Closed
wants to merge 2,935 commits into
base: master
from

Conversation

Projects
None yet
@codeguy

codeguy commented Apr 27, 2017

Related to #22391

WORK IN PROGRESS. DO NOT MERGE YET.

I am having an issue fetching the standard output from the spawned validator process. What I am doing is spawning a cmd.exe process, and that spawns a bash.exe process who in turn runs "php -l -n -f /path/to/file/to/validate.php". However, this callback is never called:

https://github.com/codeguy/vscode/blob/feature-wsl-php-validator/extensions/php/src/features/validationProvider.ts#L284-L287

I have not hooked up custom configuration settings yet. I am simply forcing VSCode to use WSL php for validation with these temporary lines:

https://github.com/codeguy/vscode/blob/feature-wsl-php-validator/extensions/php/src/features/validationProvider.ts#L252-L265

Do you have any thoughts why I am not getting any stdout?

I have seen this issue with WSL:

cpdt/slinky#11

Correct issue to reference is Microsoft/WSL#2

But that was apparently resolved with latest code pushed out with Creators Update.

Thoughts?

Wasn't sure which branch to send PR against, so using master for now. I can always re-submit.

@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Apr 27, 2017

@codeguy, thanks for your PR! By analyzing the history of the files in this pull request, we identified @dbaeumer and @egamma to be potential reviewers.

mention-bot commented Apr 27, 2017

@codeguy, thanks for your PR! By analyzing the history of the files in this pull request, we identified @dbaeumer and @egamma to be potential reviewers.

@codeguy

This comment has been minimized.

Show comment
Hide comment
@codeguy

codeguy Apr 27, 2017

Same issue happens if I directly invoke bash with childProcess.spawn('bash', ['-c "php -l ..."'], options); The standard output callback is never called.

codeguy commented Apr 27, 2017

Same issue happens if I directly invoke bash with childProcess.spawn('bash', ['-c "php -l ..."'], options); The standard output callback is never called.

@codeguy

This comment has been minimized.

Show comment
Hide comment
@codeguy

codeguy Apr 27, 2017

Also, if I directly run bash -c "php -l -n -f /mnt/c/Users/me/file.php" in console, it works. Does not work when invoked in a Node.js script.

codeguy commented Apr 27, 2017

Also, if I directly run bash -c "php -l -n -f /mnt/c/Users/me/file.php" in console, it works. Does not work when invoked in a Node.js script.

@khouzam

This comment has been minimized.

Show comment
Hide comment
@khouzam

khouzam Apr 27, 2017

Member

Hey @codeguy

We'll look at it from the console/WSL side.

Member

khouzam commented Apr 27, 2017

Hey @codeguy

We'll look at it from the console/WSL side.

@msftclas msftclas added cla-signed and removed cla-required labels Apr 27, 2017

@codeguy

This comment has been minimized.

Show comment
Hide comment
@codeguy

codeguy Apr 28, 2017

@khouzam I've gotten stdout from bash to work. However, I'm now seeing my Linux paths being auto-converted back to C:\ style path... not sure if Node is doing this, or Bash, or what. Doesn't happen when I invoke directly in console. It is happening in Node when compiled app is run via Electron. See screen cap.
output

codeguy commented Apr 28, 2017

@khouzam I've gotten stdout from bash to work. However, I'm now seeing my Linux paths being auto-converted back to C:\ style path... not sure if Node is doing this, or Bash, or what. Doesn't happen when I invoke directly in console. It is happening in Node when compiled app is run via Electron. See screen cap.
output

@codeguy

This comment has been minimized.

Show comment
Hide comment
@codeguy

codeguy Apr 28, 2017

Found a solution!!! I'll hook up custom VSCode php.validate.* settings tomorrow and squash commits.
woot

codeguy commented Apr 28, 2017

Found a solution!!! I'll hook up custom VSCode php.validate.* settings tomorrow and squash commits.
woot

@codeguy

This comment has been minimized.

Show comment
Hide comment
@codeguy

codeguy Apr 28, 2017

I've hooked up config settings. You can use these to validate via WSL shell:

{
    "php.validate.executablePath": "C:\\Windows\\sysnative\\bash.exe",
    "php.validate.executableIsShell": true,
    "php.validate.executableArgs: "-c",
    "php.validate.shellExecutablePath: "/usr/bin/php"
}

I still need to complete these tasks:

  • Ensure validation works with onType trigger mode
  • Ensure code formatting adheres to contributor guidelines
  • Squash commits

codeguy commented Apr 28, 2017

I've hooked up config settings. You can use these to validate via WSL shell:

{
    "php.validate.executablePath": "C:\\Windows\\sysnative\\bash.exe",
    "php.validate.executableIsShell": true,
    "php.validate.executableArgs: "-c",
    "php.validate.shellExecutablePath: "/usr/bin/php"
}

I still need to complete these tasks:

  • Ensure validation works with onType trigger mode
  • Ensure code formatting adheres to contributor guidelines
  • Squash commits
@dbaeumer

This comment has been minimized.

Show comment
Hide comment
@dbaeumer

dbaeumer Apr 28, 2017

Member

@codeguy thanks a lot for looking into this. A couple of first remarks: we should implement the feature in a way that it can be used to execute php in a shell as well. So the path conversion to WSL should only happen in Windows and only if the shell is bash.exe.

To make it consistent with other settings we have for tasks.json I would prefer something like this:

{
    "php.validate.executablePath": "/usr/bin/php",
    "php.validate.runInShell": {
        "executable": "C:\\Windows\\sysnative\\bash.exe",
        "shellArgs": [ "-c" ]
    }
}

we should also allow to use a boolean for "php.validate.runInShell" and then use the platforms default shell. Something like:

{
    "php.validate.executablePath": "/usr/bin/php",
    "php.validate.runInShell": true
}
Member

dbaeumer commented Apr 28, 2017

@codeguy thanks a lot for looking into this. A couple of first remarks: we should implement the feature in a way that it can be used to execute php in a shell as well. So the path conversion to WSL should only happen in Windows and only if the shell is bash.exe.

To make it consistent with other settings we have for tasks.json I would prefer something like this:

{
    "php.validate.executablePath": "/usr/bin/php",
    "php.validate.runInShell": {
        "executable": "C:\\Windows\\sysnative\\bash.exe",
        "shellArgs": [ "-c" ]
    }
}

we should also allow to use a boolean for "php.validate.runInShell" and then use the platforms default shell. Something like:

{
    "php.validate.executablePath": "/usr/bin/php",
    "php.validate.runInShell": true
}

@dbaeumer dbaeumer added this to the May 2017 milestone Apr 28, 2017

@codeguy

This comment has been minimized.

Show comment
Hide comment
@codeguy

codeguy Apr 28, 2017

@dbaeumer Thanks for the feedback. Most recent commit standardizes settings. You can now use:

{
    "php.validate.executablePath": "/usr/bin/php",
    "php.validate.runInShell": true
}

This will default to C:\Windows\sysnative\bash.exe with arguments -c. You could also use:

{
    "php.validate.executablePath": "/usr/bin/php",
    "php.validate.runInShell": {
        "shellArgs": ["-c"],
        "shellExecutable": "C:\\Windows\\sysnative\\bash.exe"
    }
}

I also attempted to declare the possible configuration settings in extensions/php/package.json, but I was unsure how/where to add localized strings for configuration setting descriptions. So I just used English for now.

codeguy commented Apr 28, 2017

@dbaeumer Thanks for the feedback. Most recent commit standardizes settings. You can now use:

{
    "php.validate.executablePath": "/usr/bin/php",
    "php.validate.runInShell": true
}

This will default to C:\Windows\sysnative\bash.exe with arguments -c. You could also use:

{
    "php.validate.executablePath": "/usr/bin/php",
    "php.validate.runInShell": {
        "shellArgs": ["-c"],
        "shellExecutable": "C:\\Windows\\sysnative\\bash.exe"
    }
}

I also attempted to declare the possible configuration settings in extensions/php/package.json, but I was unsure how/where to add localized strings for configuration setting descriptions. So I just used English for now.

@codeguy

This comment has been minimized.

Show comment
Hide comment
@codeguy

codeguy May 1, 2017

@dbaeumer I'm going to remove the last remaining console.log debugging stuff, and then squash my commits later today. Do you have any further feedback on the code in this PR?

codeguy commented May 1, 2017

@dbaeumer I'm going to remove the last remaining console.log debugging stuff, and then squash my commits later today. Do you have any further feedback on the code in this PR?

@dbaeumer

This comment has been minimized.

Show comment
Hide comment
@dbaeumer

dbaeumer May 2, 2017

Member

@codeguy more feedback:

  • regarding nls: you can use %key% as a value for a string in the package.json and then add the actual string into package.nls.json. You might want to use php.validate.executablePath as an example.

  • if runInShell is a boolean then we should use the platforms default shell for Windows, Mac and Linus. The current code simply uses bash for Windows. That path doesn't exist under Mac, Linux

  • we should only convert file paths from Windows to Linux when running in WSL. If we run in cmd.exe under Windows for example the paths must still be Windows.

And thanks again for looking into this.

Member

dbaeumer commented May 2, 2017

@codeguy more feedback:

  • regarding nls: you can use %key% as a value for a string in the package.json and then add the actual string into package.nls.json. You might want to use php.validate.executablePath as an example.

  • if runInShell is a boolean then we should use the platforms default shell for Windows, Mac and Linus. The current code simply uses bash for Windows. That path doesn't exist under Mac, Linux

  • we should only convert file paths from Windows to Linux when running in WSL. If we run in cmd.exe under Windows for example the paths must still be Windows.

And thanks again for looking into this.

@codeguy

This comment has been minimized.

Show comment
Hide comment
@codeguy

codeguy May 2, 2017

I keep forgetting this is cross-platform :) Will take care of these issues tonight.

codeguy commented May 2, 2017

I keep forgetting this is cross-platform :) Will take care of these issues tonight.

@codeguy

This comment has been minimized.

Show comment
Hide comment
@codeguy

codeguy May 7, 2017

@dbaeumer

Latest commits include the following changes:

  1. I use correct %key% syntax in package.json and package.nls.json
  2. If runInShell is a boolean...
    • If platform is win32, I use C:\\Windows\\sysnative\\bash.exe
    • Else I use default shell (i.e. process.env.SHELL else /bin/bash)
  3. File paths are converted from Windows to Linux if and only if platform is win32 and shell is bash.exe.

codeguy commented May 7, 2017

@dbaeumer

Latest commits include the following changes:

  1. I use correct %key% syntax in package.json and package.nls.json
  2. If runInShell is a boolean...
    • If platform is win32, I use C:\\Windows\\sysnative\\bash.exe
    • Else I use default shell (i.e. process.env.SHELL else /bin/bash)
  3. File paths are converted from Windows to Linux if and only if platform is win32 and shell is bash.exe.
Show outdated Hide outdated extensions/php/src/features/validationProvider.ts
@@ -147,6 +150,11 @@ export default class PHPValidationProvider {
let shellSettings = section.get<any>('validate.runInShell');
if (typeof(shellSettings) === 'boolean') {
this.runInShell = shellSettings;
if (this.platform.toLowerCase() === 'win32') {
this.shellExecutable = 'C:\\Windows\\sysnative\\bash.exe';

This comment has been minimized.

@dbaeumer

dbaeumer May 18, 2017

Member

I think we can't assume that a user wants the bash if runInShell is set to true. I would expect that the user wants to use what is defined in ComSpec under Windows.

And then we need to set the shellArgs accordingly as well since -c will not work with cmd.exe nor with powershell.exe. For cmd.exe this is /C and for PowerShell it is /Command

@dbaeumer

dbaeumer May 18, 2017

Member

I think we can't assume that a user wants the bash if runInShell is set to true. I would expect that the user wants to use what is defined in ComSpec under Windows.

And then we need to set the shellArgs accordingly as well since -c will not work with cmd.exe nor with powershell.exe. For cmd.exe this is /C and for PowerShell it is /Command

Show outdated Hide outdated extensions/php/src/features/validationProvider.ts
let linuxPath = windowsPath.trim().replace(/^([a-zA-Z]):\\/, '/mnt/$1/').replace(/\\/g, '/');
executableArgs.push(linuxPath);
// If win32 and bash.exe, transform Windows file path to Linux file path
if (this.platform === 'win32' && executable.indexOf('bash.exe') !== -1) {

This comment has been minimized.

@dbaeumer

dbaeumer May 18, 2017

Member

We can't simply test bash.exe here since git under Window installs a bash.exe as well. We either need to test for the full path or better see how bash from git mount the drive and do the path magic as well.

@dbaeumer

dbaeumer May 18, 2017

Member

We can't simply test bash.exe here since git under Window installs a bash.exe as well. We either need to test for the full path or better see how bash from git mount the drive and do the path magic as well.

@dbaeumer

This comment has been minimized.

Show comment
Hide comment
@dbaeumer

dbaeumer May 18, 2017

Member

Added some comments. Here is a screen shot how git bash handles this:

capture

Member

dbaeumer commented May 18, 2017

Added some comments. Here is a screen shot how git bash handles this:

capture

@codeguy

This comment has been minimized.

Show comment
Hide comment
@codeguy

codeguy Jun 25, 2017

Haven't forgotten about this. Haven't had time lately. Will try to wrap this up this week.

codeguy commented Jun 25, 2017

Haven't forgotten about this. Haven't had time lately. Will try to wrap this up this week.

@codeguy

This comment has been minimized.

Show comment
Hide comment
@codeguy

codeguy Jun 25, 2017

Attempting to re-create local dev environment to work on this pull request. Running into this bug #29310

codeguy commented Jun 25, 2017

Attempting to re-create local dev environment to work on this pull request. Running into this bug #29310

@codeguy

This comment has been minimized.

Show comment
Hide comment
@codeguy

codeguy Jul 3, 2017

I suspect I'm not knowledgeable enough in the Windows environment yet to auto-detect the various possible shells and how to differentiate Git bash.exe from WSL bash.exe. Would you accept and merge this PR were it only to allow validate.runInShell as an object requiring the VSCode user to explicitly define the Windows executable path, shell binary, and shell args?

codeguy commented Jul 3, 2017

I suspect I'm not knowledgeable enough in the Windows environment yet to auto-detect the various possible shells and how to differentiate Git bash.exe from WSL bash.exe. Would you accept and merge this PR were it only to allow validate.runInShell as an object requiring the VSCode user to explicitly define the Windows executable path, shell binary, and shell args?

codeguy added some commits Jul 3, 2017

Use ComSpec
Detect msys Git Bash
@codeguy

This comment has been minimized.

Show comment
Hide comment
@codeguy

codeguy Jul 3, 2017

I've made an attempt to honor ComSpec setting. I'm also setting correct shell args if cmd.exe, powershell.exe, or bash.exe. If bash.exe, I'm inspecting bash.exe --version output to detect "msys", and if found, applying correct filepath transform for Git Bash vs. WSL Bash.

codeguy commented Jul 3, 2017

I've made an attempt to honor ComSpec setting. I'm also setting correct shell args if cmd.exe, powershell.exe, or bash.exe. If bash.exe, I'm inspecting bash.exe --version output to detect "msys", and if found, applying correct filepath transform for Git Bash vs. WSL Bash.

codeguy added some commits Jul 3, 2017

@dbaeumer

This comment has been minimized.

Show comment
Hide comment
@dbaeumer

dbaeumer Jul 13, 2017

Member

@codeguy sorry for not coming back earlier but I was very busy with Tasks 2.0.0. And now I am about to heading out on vacation until beginning of August. Will look at the PR then.

Member

dbaeumer commented Jul 13, 2017

@codeguy sorry for not coming back earlier but I was very busy with Tasks 2.0.0. And now I am about to heading out on vacation until beginning of August. Will look at the PR then.

@dbaeumer dbaeumer modified the milestones: August 2017, May 2017 Jul 13, 2017

@msftgits msftgits removed the cla-signed label Sep 26, 2017

@Microsoft Microsoft deleted a comment from msftclas Sep 26, 2017

@Microsoft Microsoft deleted a comment from msftclas Sep 26, 2017

@roblourens roblourens modified the milestones: August 2017, October 2017 Sep 27, 2017

@alexandrudima alexandrudima removed this from the October 2017 milestone Dec 12, 2017

@benjaminprojas

This comment has been minimized.

Show comment
Hide comment
@benjaminprojas

benjaminprojas Jan 3, 2018

Would love to see this added. Any update on the progress?

benjaminprojas commented Jan 3, 2018

Would love to see this added. Any update on the progress?

@codeguy

This comment has been minimized.

Show comment
Hide comment
@codeguy

codeguy Jan 3, 2018

My work on this has stalled and I do not have availability any time soon to continue work on it. Someone is welcome to pick up where I left off or work on an alternative PR.

codeguy commented Jan 3, 2018

My work on this has stalled and I do not have availability any time soon to continue work on it. Someone is welcome to pick up where I left off or work on an alternative PR.

@roblourens roblourens closed this Aug 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment