- Notifications
You must be signed in to change notification settings - Fork670
support WSL#46
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
support WSL#46
Uh oh!
There was an error while loading.Please reload this page.
Conversation
@purocean Thank you for the contribution. I'll take a look at this PR these days. |
src/utils/wslUtils.ts Outdated
| } | ||
| export function toWinPath(path: string): string { | ||
| return cp.execFileSync("wsl", ["--", "wslpath", "-w", path]).toString().trim(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It looks the 1st param "--" doesn't work for me.
CMD>wsl wslpath -w /mnt/c/C:\CMD>wsl -- wslpath -w /mnt/c//bin/bash: --: invalid optionUsage: /bin/bash [GNU long option] [option] ... /bin/bash [GNU long option] [option] script-file ...GNU long options: --debug --debugger --dump-po-strings --dump-strings --help --init-file --login --noediting --noprofile --norc --posix --rcfile --restricted --verbose --versionShell options: -ilrsD or -c command or -O shopt_option (invocation only) -abefhkmnptuvxBCHP or -o optionThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Please check the Windows version, maybe this param can be removed.
C:\>verMicrosoft Windows [Version 10.0.17713.1000]C:\>wsl --helpUsage: wsl.exe [option] ...Options: -d, --distribution <DistributionName> Launch the specified distribition. -e, --exec <CommandLine> Execute the specified Linux command. The remainder of the arguments are used as the command line to execute. -u, --user <UserName> Run as the specified user. --help Display this usage information. -- Stop parsing arguments and pass the remainder to the Linux process.C:\>wsl -- wslpath -u "C:/"/mnt/c/There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Attaching mine.
C:\>verMicrosoft Windows [Version 10.0.17134.167]C:\>wsl --help/bin/bash: --: invalid optionUsage: /bin/bash [GNU long option] [option] ... /bin/bash [GNU long option] [option] script-file ...GNU long options: --debug --debugger --dump-po-strings --dump-strings --help --init-file --login --noediting --noprofile --norc --posix --rcfile --restricted --verbose --versionShell options: -ilrsD or -c command or -O shopt_option (invocation only) -abefhkmnptuvxBCHP or -o optionThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I thinkwsl.exe is a wrapper to run inner shell in your WSL. I suppose the parameters depend on what your inner shell is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@purocean If you remove the-- at your side, will this command still workable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
https://docs.microsoft.com/en-us/windows/wsl/release-notes#wsl-5
@jdneo yes, perfect. So we can remove it.
package.json Outdated
| "type": "boolean", | ||
| "default": false, | ||
| "scope": "window", | ||
| "description": "Use nodejs inside the Windows Subsystem for Linux." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
nodejs -> Node.js
Looks like there are several places which contains the logic like I think it could be better to have a pathUtils that wrap this kind of logic there.@purocean what do you think? |
| } | ||
| export function toWslPath(path: string): string { | ||
| return cp.execFileSync("wsl", ["wslpath", "-u", `${path.replace(/\\/g, "/")}`]).toString().trim(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
If we use" to wrap the path instead of replace\, will it be workable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Not Work :(
functiontoWslPath(path){returncp.execFileSync("wsl",["wslpath","-u",`"${path}"`]).toString().trim();}
module.js:549 throw err; ^Error: Cannot find module '/mnt/y/env/VSCode-win32-x64/Y:/env/VSCode-win32-x64/.vscode/extensions/shengchen.vscode-leetcode-0.7.0/node_modules/leetcode-cli/bin/leetcode' at Function.Module._resolveFilename (module.js:547:15) at Function.Module._load (module.js:474:25) at Function.Module.runMain (module.js:693:10) at startup (bootstrap_node.js:191:16) at bootstrap_node.js:612:3There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
WithpathUtils my point of view is not unnecessary at this time.
For this program, we just need do good job with file system and shell command for cross-platform. I think thecpUtils andworkspaceUtils is sufficiently abstract.
We can try to change these next time if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We could focus oncpUtils andworkspaceUtils provide service for others.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@purocean That is because by default the optionshell ofexecFileSync is false. If add{shell: true} to the argument list, then I think it should work.
Another thing I'm concerning is that here we useexecFileSync(). Personally, I prefer useasync/await wherever possible. Take a look at this:https://nodejs.org/en/docs/guides/blocking-vs-non-blocking/
So can we use executeCommand in the cpUtils?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
{shell: true} work perfectly.
We need a lot of works if we use async/await. Abstractleetcode command is a better option.
But we could improve step by step. Half a loaf is better than no bread.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
OK, make sense. Tracking issue:#48
@purocean Merged, thank you. |
Uh oh!
There was an error while loading.Please reload this page.
resolve#47