Windows support #1033

ForNeVeR · 2017-05-20T09:01:03Z

In this branch, I've included the exact changes I've done to execute black-screen on my Windows machine.

There're many unwanted changes in this PR;

eventually I'll squash the changes together and provide a clean PR without all the debug / logging / leftover stuff.

Issues remaining

⚠ At some places, I've replaced Linux commands / options with their Windows counterparts; I will remove that and fix properly, but I'd like community help on that part. (It's not always obvious how do I change the architecture to provide platform-specific options.)
⚠ I've disabled some compiler checks to make it compile; need to enable checks back and fix issues properly.
I've added some excessive logging; it should be removed.
There're troubles with node-pty typings; waiting for my pull request Add "types" to package.json microsoft/node-pty#83 to be merged and published. We could write our own typings if necessary (e.g. maintainer won't answer for a while).
Replace "start": "bash housekeeping/start.sh", with something more portable; we have no bash on Windows.
Conduct final testing before passing to the review

Most of the code come from quick hacks and fixes, so, please, help me to fix it properly and follow the project architecture.

Impact

This will close issue #800; we should urge Windows users to open new issues for every missing/misbehaving feature after that (e.g. PowerShell support, proper alias support on Windows etc. etc.)

ForNeVeR · 2017-05-20T09:03:06Z

package.json

@@ -83,7 +82,7 @@
    "lint": "tslint `find src -name '*.ts*'` `find test -name '*.ts*'`",
    "cleanup": "rm -rf compiled/src",


We need to use portable rimraf for that command.

ForNeVeR · 2017-05-20T09:03:49Z

package.json

@@ -83,7 +82,7 @@
    "lint": "tslint `find src -name '*.ts*'` `find test -name '*.ts*'`",
    "cleanup": "rm -rf compiled/src",
    "copy-html": "mkdir -p compiled/src/views && cp src/views/index.html compiled/src/views",


Rewrite in portable way (because Windows have no mkdir and cp).

ForNeVeR · 2017-05-20T09:04:00Z

package.json

@@ -83,7 +82,7 @@
    "lint": "tslint `find src -name '*.ts*'` `find test -name '*.ts*'`",
    "cleanup": "rm -rf compiled/src",
    "copy-html": "mkdir -p compiled/src/views && cp src/views/index.html compiled/src/views",
-    "compile": "npm run cleanup && npm run tsc && npm run copy-html",
+    "compile": "npm run tsc",


Remember to put these commands back after fix.

ForNeVeR · 2017-05-20T09:06:44Z

src/PTY.ts

 import {loginShell} from "./utils/Shell";
 import {debug} from "./utils/Common";

+let smth = true ? null : pty.createTerminal(undefined, undefined, undefined);
+type ITerminal = typeof smth;


I had to do that hack because:
a) node-pty doesn't export its ITerminal type
b) microsoft/TypeScript#6606 isn't (yet?) implemented, so we cannot directly do type ITerminal = typeof(pty.createTerminal(undefined, undefined, undefined))

Both could be solved by either exporting the proper types from node-pty library or writing our own type definitions (it's not that complex).

ForNeVeR · 2017-05-20T09:08:35Z

src/PTY.ts


    // TODO: write proper signatures.
    // TODO: use generators.
    // TODO: terminate. https://github.com/atom/atom/blob/v1.0.15/src/task.coffee#L151
    constructor(words: EscapedShellWord[], env: ProcessEnvironment, dimensions: Dimensions, dataHandler: (d: string) => void, exitHandler: (c: number) => void) {
-        const shellArguments = [...loginShell.noConfigSwitches, "-i", "-c", words.join(" ")];


⚠ Warning: Unix-specific options were replaced by Windows-specific; need to find a portable way to express this option set.

(It has been fixed)

ForNeVeR · 2017-05-20T09:09:18Z

src/PTY.ts

+            dataHandler(data);
+        });
+        (this.terminal as any).on("exit", (code: number) => {
+            console.log('PTY: exit');
            exitHandler(code);
        });


For some reason, ITerminal doesn't declare its on method. So I had to do these ugly as any casts.

ForNeVeR · 2017-05-20T09:10:26Z

src/PTY.ts

@@ -65,9 +81,11 @@ export function executeCommand(
            ...execOptions,
            env: _.extend({PWD: directory}, process.env),
            cwd: directory,
-            shell: "/bin/bash",
+            shell: "cmd",


⚠ Warning: honestly I'm not sure what's this option impact; probably need to use some portable path definition from loginShell.

ForNeVeR · 2017-05-20T09:11:04Z

src/PTY.ts

    const sourceCommands = (await loginShell.existingConfigFiles()).map(fileName => `source ${fileName} &> /dev/null`);

-    return await linedOutputOf(loginShell.executableName, ["-c", `'${[...sourceCommands, command].join("; ")}'`], process.env.HOME);
+    return await linedOutputOf(loginShell.executableName, ["/c", `"${[...sourceCommands, command].join(" && ")}"`], process.env.HOME);


⚠ Warning: Unix-specific options were replaced by Windows-specific; need to find a portable way to express this option set.

ForNeVeR · 2017-05-20T09:11:45Z

src/shell/Aliases.ts

+        //lines.map(parseAlias).forEach(parsed => aliasesFromConfig[parsed.name] = parsed.value);
+    } catch (error) {
+        console.error(error);
+    }


⚠ Warning: As far as I know, Windows' cmd shell doesn't have a concept of alias. That's why I had to comment these out. We'll need to add an option haveAliases to loginShell or something like that.

Windows PowerShell has aliasing feature which you can know more
by opening a PowerShell, then run

Get-Help about_Aliases

I know about that, although I've decided to add only cmd support for now. I love PowerShell, but alternate shell support on Windows is another issue, and I'd like to limit scope of this issue to just cmd.exe.

ForNeVeR · 2017-05-20T09:12:46Z

src/shell/Environment.ts

@@ -34,7 +34,7 @@ export const preprocessEnv = (lines: string[]) => {

 export const processEnvironment: Dictionary<string> = {};
 export async function loadEnvironment(): Promise<void> {
-    const lines = preprocessEnv(await executeCommandWithShellConfig("env"));
+    const lines = preprocessEnv(await executeCommandWithShellConfig("set"));


⚠ Warning: Unix-specific command was replaced by a Windows-specific; need to find a portable way to express this command.

ForNeVeR · 2017-05-20T09:13:42Z

src/utils/Shell.ts

 };

 const shell = () => {
+    if (!process.env.SHELL) {
+        process.env.SHELL = 'C:\\Windows\\System32\\cmd.exe';
+    }


Debug leftover; just drop this.

Although we'll probably better add Windows detection and fallback to cmd.exe instead of /bin/bash on Windows below.

$SHELL typically isn't set on Windows, they have a similar environment variable COMSPEC but it defaults to the 32-bit version of cmd.exe, as does "cmd.exe" on the path. I've found defaulting to %windir%\Sysnative\cmd.exe on 64-bit machine and %windir%\System32\cmd.exe on 32-bit machines is the best way to go

That's a good suggestion. I think I'll implement it, thanks.

ForNeVeR · 2017-05-20T09:14:26Z

tsconfig.json

    "noImplicitThis": true,
-    "inlineSourceMap": true,
-    "noUnusedLocals": true,
-    "noUnusedParameters": true


These are disabled checks I was talking about in the PR description; ⚠ we need to put them all back.

ForNeVeR · 2017-05-25T16:12:42Z

Squashed and cleaned the PR. Please review.

ForNeVeR · 2017-05-25T17:02:01Z

Well, the tests are failing; obviously I've broken something. If anybody wants to help me to fix that issue — you're welcome, guys!

Odonno · 2017-05-25T20:40:25Z

I can see the current master branch is failing. I suppose you need to wait until a fix is done and then merge the new commits.

ForNeVeR · 2017-05-27T11:07:20Z

Alright. Rebased my branch on top of current master, and everything is okay now. Ready for review.

vlad-shatskyi · 2017-05-28T06:16:56Z

@ForNeVeR great job, thanks! I'm not able to test if it works, so I'll just merge it. Please mention me next time, so that I see your messages sooner.

ForNeVeR · 2017-05-28T06:21:18Z

Thank you! I can confirm that it still works in master.

ForNeVeR commented May 20, 2017

View reviewed changes

Tyriar mentioned this pull request May 22, 2017

Add "types" to package.json microsoft/node-pty#83

Merged

ForNeVeR mentioned this pull request May 22, 2017

Windows Support #800

Open

ForNeVeR changed the title ~~[WIP] [HELP WANTED] Windows support~~ [WIP] Windows support May 24, 2017

ForNeVeR force-pushed the feature/800-windows-support branch from b41b254 to 423a7bd Compare May 25, 2017 16:10

ForNeVeR changed the title ~~[WIP] Windows support~~ Windows support May 25, 2017

ForNeVeR force-pushed the feature/800-windows-support branch 3 times, most recently from 81c5089 to d9fe19b Compare May 25, 2017 16:52

railsware#800: support for Windows' cmd shell

c06298e

ForNeVeR force-pushed the feature/800-windows-support branch from d9fe19b to c06298e Compare May 27, 2017 10:59

vlad-shatskyi merged commit d0daa7a into railsware:master May 28, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Windows support #1033

Windows support #1033

ForNeVeR commented May 20, 2017 •

edited

Loading

ForNeVeR May 20, 2017

ForNeVeR May 20, 2017 •

edited

Loading

ForNeVeR May 20, 2017 •

edited

Loading

ForNeVeR May 20, 2017 •

edited

Loading

ForNeVeR May 20, 2017 •

edited

Loading

ForNeVeR May 20, 2017

ForNeVeR May 20, 2017 •

edited

Loading

ForNeVeR May 20, 2017

ForNeVeR May 20, 2017 •

edited

Loading

ShadowRZ May 20, 2017

ForNeVeR May 20, 2017 •

edited

Loading

ForNeVeR May 20, 2017

ForNeVeR May 20, 2017

Tyriar May 22, 2017

ForNeVeR May 22, 2017

ForNeVeR May 20, 2017

ForNeVeR commented May 25, 2017

ForNeVeR commented May 25, 2017

Odonno commented May 25, 2017

ForNeVeR commented May 27, 2017

vlad-shatskyi commented May 28, 2017

ForNeVeR commented May 28, 2017

		@@ -83,7 +82,7 @@
		"lint": "tslint `find src -name '.ts'` `find test -name '.ts'`",
		"cleanup": "rm -rf compiled/src",

Windows support #1033

Windows support #1033

Conversation

ForNeVeR commented May 20, 2017 • edited Loading

Issues remaining

Impact

Choose a reason for hiding this comment

ForNeVeR May 20, 2017 • edited Loading

Choose a reason for hiding this comment

ForNeVeR May 20, 2017 • edited Loading

Choose a reason for hiding this comment

ForNeVeR May 20, 2017 • edited Loading

Choose a reason for hiding this comment

ForNeVeR May 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ForNeVeR May 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ForNeVeR May 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ForNeVeR May 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ForNeVeR commented May 25, 2017

ForNeVeR commented May 25, 2017

Odonno commented May 25, 2017

ForNeVeR commented May 27, 2017

vlad-shatskyi commented May 28, 2017

ForNeVeR commented May 28, 2017

ForNeVeR commented May 20, 2017 •

edited

Loading

ForNeVeR May 20, 2017 •

edited

Loading

ForNeVeR May 20, 2017 •

edited

Loading

ForNeVeR May 20, 2017 •

edited

Loading

ForNeVeR May 20, 2017 •

edited

Loading

ForNeVeR May 20, 2017 •

edited

Loading

ForNeVeR May 20, 2017 •

edited

Loading

ForNeVeR May 20, 2017 •

edited

Loading