-
Notifications
You must be signed in to change notification settings - Fork 689
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
support WSL #46
Conversation
64683e7 to
024fd74
Compare
jdneo
commented
Jul 23, 2018
@purocean Thank you for the contribution. I'll take a look at this PR these days.
125a668 to
d19d2c4
Compare
src/utils/wslUtils.ts
Outdated
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 option
Usage: /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
--version
Shell 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:\>ver
Microsoft Windows [Version 10.0.17713.1000]
C:\>wsl --help
Usage: 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:\>ver
Microsoft Windows [Version 10.0.17134.167]
C:\>wsl --help
/bin/bash: --: invalid option
Usage: /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
--version
Shell options:
-ilrsD or -c command or -O shopt_option (invocation only)
-abefhkmnptuvxBCHP or -o option
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.
I think wsl.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
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
jdneo
commented
Jul 24, 2018
Looks like there are several places which contains the logic like wsl.useWsl() ? wsl.toWslPath(workFolder) : workFolder.
I think it could be better to have a pathUtils that wrap this kind of logic there. @purocean what do you think?
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 :(
function toWslPath(path) { return cp.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:3
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.
With pathUtils 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 the cpUtils and workspaceUtils 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 on cpUtils and workspaceUtils 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 option shell of execFileSync 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 use execFileSync(). Personally, I prefer use async/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. Abstract leetcode 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
jdneo
commented
Jul 24, 2018
@purocean Merged, thank you.
Uh oh!
There was an error while loading. Please reload this page.
resolve #47