Windows fixes #158

gregmagolan · 2018-03-01T03:13:06Z

gregmagolan · 2018-03-01T03:13:43Z

Curated and improved fixes from #155

alexeagle · 2018-03-01T16:42:48Z

WORKSPACE

-# programs produce source-mapped stack traces.
-RULES_NODEJS_VERSION = "926349cea4cd360afcd5647ccdd09d2d2fb471aa"
+# Using a pre-release snapshot to pick up a fix for puppeteer
+RULES_NODEJS_VERSION = "0162fdbe8ed986c9b5d5b79e53c98385ddaf6edd"


don't think you need this anymore?

you mean switch to git_repository from the skylark rules?

nevermind, this is just a fast-forward of master

alexeagle · 2018-03-01T16:44:28Z

internal/karma/karma.conf.js

+const fs = require('fs');
+const tmp = require('tmp');
+
+process.env.CHROME_BIN = require('puppeteer').executablePath();


in theory the ts_web_test should allow the user to select one or more browsers,
https://github.com/bazelbuild/rules_typescript/blob/master/internal/karma/ts_web_test.bzl#L22

just to keep in mind, that anything hard-coded will need to be revisited

I'll add a comment to that line explaining it a bit. Karma will just look for CHROME_BIN in the environment if it is configured to use Chrome. If another browser is selected then that value is not used.

alexeagle · 2018-03-01T16:44:53Z

internal/karma/karma.conf.js

+      throw new Error(`File not found in MANIFEST: ${f}`);
+    }
+  });
+} else {


do we need to support a case where there is no MANIFEST?

same as comment below. build_bazel_rules_typescript/external/ prefix is what is needed to match in the manifest and that needs to be dropped so it matches on runfiles.

I can add a comment here that explains this

Cleaned up. No MANIFEST case is not needed.

alexeagle · 2018-03-01T16:45:17Z

internal/karma/karma.conf.js

+
+let requireFiles = [
+  TMPL_files
+].map((f) => f.replace('build_bazel_rules_typescript/external/', ''));


why is rules_typescript special here? Could there be other external repos in the files?

that is to handle the two files from ts_web_test.bzl:

files_entries += [ "build_bazel_rules_typescript/external/build_bazel_rules_typescript_karma_deps/node_modules/requirejs/require.js", "build_bazel_rules_typescript/external/build_bazel_rules_typescript_karma_deps/node_modules/karma-requirejs/lib/adapter.js", ]

I think its correct. build_bazel_rules_typescript/external/ prefix is what is needed to match in the manifest and that needs to be dropped so it matches on runfiles. in the case of the two files from ts_web_test.bz, the external workspace is build_bazel_rules_typescript_karma_deps.

Oh. I took a closer look at the MANIFEST file. I think this code can be improved to get rid the build_bazel_rules_typescript/external/ bit. Will try it out.

Cleaned this up. No build_bazel_rules_typescript/external/ prefix is necessary.

alexeagle · 2018-03-01T16:45:49Z

internal/karma/karma.conf.js

-TMPL_files
-    ]
+
+    // files


stray comment?

yeah. a bit redundant. there was a comment before every other property and I didn't know what else to put.

alexeagle · 2018-03-01T16:48:36Z

internal/tsc_wrapped/strict_deps.ts

@@ -17,6 +17,7 @@

 import * as path from 'path';
 import * as ts from 'typescript';
+import {relative} from './path';


again, import * as path from 'path'

this is importing from the local ./path.ts which provides versions of the four path functions that were introducing forward slashes. the new versions maintain backslashes. you think it would be better as import * as path from './path'? may be a bit confusing.

yes, that's what I meant, that way we see the familiar path.relative below, and makes this delta smaller

hmmm. this will conflict with the existing path imports and uses in compiler_host.ts and tsconfig.ts.

I can just re-export the functions used out of './path'. That will be pretty clean.

alexeagle · 2018-03-01T16:48:45Z

internal/tsc_wrapped/tsc_wrapped.ts

@@ -12,6 +11,7 @@ import {PLUGIN as strictDepsPlugin} from './strict_deps';
 import {BazelOptions, parseTsconfig} from './tsconfig';
 import {fixUmdModuleDeclarations} from './umd_module_declaration_transform';
 import {debug, log, runAsWorker, runWorkerLoop} from './worker';
+import {resolve} from './path';


alexeagle · 2018-03-01T16:48:52Z

internal/tsc_wrapped/tsconfig.ts

@@ -17,6 +17,7 @@

 import * as path from 'path';
 import * as ts from 'typescript';
+import {resolve, normalize} from './path';


alexeagle · 2018-03-01T16:49:06Z

internal/tsc_wrapped/tsconfig_test.ts

 import * as ts from 'typescript';

 import {parseTsconfig} from './tsconfig';
+import {resolve} from './path';


alexeagle · 2018-03-01T16:49:15Z

internal/tsc_wrapped/worker.ts

@@ -1,4 +1,4 @@
-import * as path from 'path';
+import {resolve} from './path';


achew22 · 2018-03-02T00:02:02Z

@gregmagolan, does this address #152?

gregmagolan · 2018-03-02T00:04:13Z

@gregmagolan, does this address #152?

Nope. Although there is a fix for that that @alexeagle has submitted internally

gregmagolan · 2018-03-02T00:05:27Z

@achew22 Its been merged in. Fixed on rules_typescript master now. Commit is: Workaround for microsoft/TypeScript#22208

alexeagle · 2018-03-02T14:47:28Z

Okay I got @mprobst on chat, here is the code review:

split into two PRs, one for tsc_wrapped and one for karma.
for tsc_wrapped, copy this function https://github.com/Microsoft/TypeScript/blob/a564912d9a335fe67ce7c5e4dabce46015bad8d4/src/compiler/core.ts#L1943 into compiler_host.ts
and then call it only in the minimal places required to make Windows green - things like the change in worker.ts are only error reporting so that can't be strictly required.

gregmagolan · 2018-03-02T19:38:53Z

Replaced by #160, #161 and #162

googlebot added the cla: yes label Mar 1, 2018

gregmagolan force-pushed the windows-fixes-v3 branch 3 times, most recently from 80c5e1c to 1ab2d36 Compare March 1, 2018 05:57

Windows tsc_wrapped path fixes

5406923

gregmagolan force-pushed the windows-fixes-v3 branch from 1ab2d36 to 5406923 Compare March 1, 2018 06:02

gregmagolan added 2 commits February 28, 2018 22:02

Windows fixes for examples tests

5b3b510

ts_web_test & karma fixes for Windows

aaa5eae

alexeagle reviewed Mar 1, 2018

View reviewed changes

gregmagolan added 2 commits March 1, 2018 10:14

Add a comment re: CHROME_BIN usage in karma.conf.js

2e09a94

Simplify karma config

66f3a35

alexeagle requested a review from mprobst March 1, 2018 21:41

gregmagolan added 2 commits March 1, 2018 15:45

Cleanup for path fixes

dcb1a81

Fix comments in internal/tsc_wrapped/path.ts

eaf8483

gregmagolan mentioned this pull request Mar 2, 2018

WIP: Windows fixes #155

Closed

gregmagolan closed this Mar 2, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Windows fixes #158

Windows fixes #158

gregmagolan commented Mar 1, 2018

gregmagolan commented Mar 1, 2018

alexeagle Mar 1, 2018

gregmagolan Mar 1, 2018

alexeagle Mar 1, 2018

alexeagle Mar 1, 2018

gregmagolan Mar 1, 2018

alexeagle Mar 1, 2018

gregmagolan Mar 1, 2018

gregmagolan Mar 1, 2018

gregmagolan Mar 1, 2018

alexeagle Mar 1, 2018

gregmagolan Mar 1, 2018 •

edited

Loading

gregmagolan Mar 1, 2018

gregmagolan Mar 1, 2018

alexeagle Mar 1, 2018

gregmagolan Mar 1, 2018

alexeagle Mar 1, 2018

gregmagolan Mar 1, 2018 •

edited

Loading

alexeagle Mar 1, 2018

gregmagolan Mar 1, 2018

gregmagolan Mar 1, 2018

alexeagle Mar 1, 2018

alexeagle Mar 1, 2018

alexeagle Mar 1, 2018

alexeagle Mar 1, 2018

achew22 commented Mar 2, 2018

gregmagolan commented Mar 2, 2018

gregmagolan commented Mar 2, 2018

alexeagle commented Mar 2, 2018

gregmagolan commented Mar 2, 2018

		@@ -1,4 +1,4 @@
		import * as path from 'path';
		import {resolve} from './path';

Windows fixes #158

Windows fixes #158

Conversation

gregmagolan commented Mar 1, 2018

gregmagolan commented Mar 1, 2018

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

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

gregmagolan Mar 1, 2018 • 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

gregmagolan Mar 1, 2018 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

achew22 commented Mar 2, 2018

gregmagolan commented Mar 2, 2018

gregmagolan commented Mar 2, 2018

alexeagle commented Mar 2, 2018

gregmagolan commented Mar 2, 2018

gregmagolan Mar 1, 2018 •

edited

Loading

gregmagolan Mar 1, 2018 •

edited

Loading