Windows fixes #71

johnpangalos · 2021-01-25T19:53:52Z

Some fixes to the extension on windows including showing errors and running bsb.

The error path wasn't taking into account that on windows paths often start with C:/ while looking for a ':'.
On windows, you have to call child processes differently.

Thanks to @mewhhaha for doing all the work!

Fixes #41

Error diagnostics fixes for Windows

Move findlocationseparator into other function

Reformatted some code

Reverting package.json as it is a unnecessary change

chenglou

Thanks a lot! Left some comments =D

chenglou · 2021-01-26T13:52:29Z

server/src/utils.ts

-		cwd: projectRootPath,
-	});
-	return process;
+	switch (os.platform()) {


We use process.platform. Or are we all supposed to switch to os.platform()?

Sorry, missed that! Will change to process.platform. 😄

chenglou · 2021-01-26T13:52:42Z

server/src/utils.ts

-	});
-	return process;
+	switch (os.platform()) {
+		case "win32": return childProcess.exec(`${bsbPath}.cmd -w`, {


if-else please

Maybe all these switch statements I introduced should be if-else? Would that be better?

I changed all of them into if elses.

chenglou · 2021-01-26T14:01:56Z

client/package-lock.json

@@ -1,264 +1,8 @@
 {
 	"name": "rescript-language-client",
 	"version": "0.0.1",
-	"lockfileVersion": 2,
+	"lockfileVersion": 1,


Humm I guess this should not be in this diff? Also the team's using a newer version of npm, thus the lockfile version difference

You're right. Should restore the lock-file. 👍

chenglou · 2021-01-26T14:02:35Z

server/src/utils.ts

-		let location = fileAndLocation.substring(locationSeparator + 1);
+		let [fileAndLocationLine, ...diagnosticMessage] = parsedDiagnostic.content;
+
+		let fileAndLocation = fileAndLocationLine.trim()


Why the trim here?

I saw that the lines came in with spacing in front of the path, like: ' C:\rescript-project-template\src\Demo.res:1:14-20\r\r'. Would have to remove the whitespace so it's possible to slice off the drive letter and colon afterwards. I think this was done with .slice(2) before. Would you rather it be done using that method? Is it always 2 spaces?

It was substring(2) but that's the issue we saw. It can be done in the original way.

I do feel like trim makes it clearer what's going on, and can be more robust if the compiler decides to change the number of spaces (like @mewhhaha seems to be implying from 'Is it always 2 spaces')

Let us know which you prefer and it's an easy fix.

Ah yes, the 2 spaces is guaranteed. I've put a note here just now: https://github.com/rescript-lang/rescript-vscode/blob/20841214830a3a81140f63a8973ab5d08cfcd79f/CONTRIBUTING.md#compilerlog

Whenever it's not, it's a bug that shouldn't be covered up here, but rather fixed int the compiler. We've caught a few compiler diagnostics display bugs this way actually.

In general also, we'd like to avoid trimming: https://github.com/rescript-lang/rescript-vscode/blob/20841214830a3a81140f63a8973ab5d08cfcd79f/CONTRIBUTING.md#formatting-newline

FYI trimming has caused and hidden all sorts of bugs before. Sometime the formatting adds a newline, or two, then the editor trims, adds a new line, or whatever. Lots of little shuffling.

I'll go ahead and fix this and change it back to the cutting out the 2 spaces using substring.

Update from comments

chenglou · 2021-01-27T10:48:03Z

Btw, what about bsc?

rescript-vscode/server/src/utils.ts

Line 73 in 2084121

export let formatUsingValidBscPath = (

chenglou · 2021-01-27T10:54:06Z

server/src/utils.ts

+	let file = fileAndLocation.slice(0, locationSeparator)
+	let location = fileAndLocation.slice(locationSeparator + 1)
+	if (process.platform === "win32") {
+		return [`file:\\\\\\${file}`, location];


So what's this for?

Somewhat related: I had a note about file: at

rescript-vscode/server/src/utils.ts

Line 20 in 2084121

// TODO: this doesn't handle file:/// scheme

. Not sure if relevant. I think vscode had some utilities for handling file:

Yeah I think so too, I'll try it and see if that gives us the same result

Seems to be working on my Ubuntu and Windows 10. Do you see the current approach (Uri.file(..)) to be an issue?

I should be what @chenglou was talking about, the utility vscode has for handling files.

Use the file scheme for everything

This is a reversion

chenglou · 2021-01-28T11:01:24Z

Sorry, maybe I got ahead of myself but I thought the vscode-uri util would have been more useful; I guess not right now.
The extra dep isn't worth the one line usage; I'll remove it.

Merging now. Thanks so much! Could you test master afterward? Then I'll make a release.

mewhhaha · 2021-01-28T12:54:04Z

I tried it on my Windows 10 machine and it worked! Thanks!

chenglou · 2021-01-28T13:09:04Z

I've cleaned up the file and location parsing code to parse even more seriously in fc2e73f. Now it doesn't accidentally parse the wrong :, so doesn't need windows-specific heuristics in that one place anymore.

Will release tomorrow.

mewhhaha · 2021-01-28T14:15:36Z

It seems like a good idea to match on the consistent part of the incoming line, however this regex doesn't work for me as the incoming line looks like C:\\rescript-project-template\\src\\Demo.res:1:14-20\r\r. I'm guessing these carriage return characters are the issue.

chenglou · 2021-01-28T14:22:56Z

You mean \r\n right?

mewhhaha · 2021-01-28T14:24:43Z

I would've expected \r\n, but the debugger gives me \r\r. Is that just an issue with the debugger?

chenglou · 2021-01-28T14:27:48Z

Ah probably because of

rescript-vscode/server/src/utils.ts

Line 247 in 1cd7b81

let lines = content.split("\n");

mewhhaha · 2021-01-28T14:29:09Z

Maybe splitting on /\r?\n/ would help?

EDIT: I still get a \r left when splitting on /\r?\n/, which I could have expected (╯°□°）╯︵ ┻━┻

chenglou · 2021-01-28T14:38:36Z

I'm splitting on os.EOL now. Try that?

mewhhaha · 2021-01-28T14:41:21Z

It doesn't work for me. There are still lingering \r characters on some of the lines. Maybe it'd help if I posted the content of what's being read from the compiler.log:

'#Start(359482524)\r\n\r\r\n We've found a bug for you!\r\r\n C:\\rescript-project-template\\src\\Demo.res:1:14-20\r\r\n \r\r\n 1 │ let x: int = "asdas"\r\r\n \r\r\n This has type: string\r\r\n Somewhere wanted: int\r\r\n \r\r\n You can convert string to int with Belt.Int.fromString.\r\r\n \r\r\n#Done(359482556)\r\n'

chenglou · 2021-01-28T14:56:07Z

That log file consists of bsb, ninja and ocaml's format module, so I'm thinking maybe there's some mix of hard-coded \n in there and \r\n from one or more of these that properly handles newline. Except... that wouldn't produce a double \r...? Another way of looking at this is, obviously \r\n is inserted properly, so where does that first extra \r come from?

mewhhaha · 2021-01-28T15:09:23Z

Splitting on /\r?\r?\n/ would work, but maybe that's circumventing the issue. Seems like it's \r\n that's being translated into \r\r\n at least and it only occurs between #Start and #Done. Perhaps it looked like #Start(..)\n\r\n.. at some point and then all newlines were prefixed with an \r, like (\r)\n\r(\r)\n.

chenglou · 2021-01-28T15:23:45Z

Ugh, I guess I'll have to end up using trim() anyway...

See #71 (comment)

chenglou · 2021-01-28T15:27:02Z

Done!

mewhhaha · 2021-01-28T15:27:10Z

I think it'd be the most maintainable way of doing it, just trim instead of slice. \r?\r?\n? just feels like someone would go in and accidentally "correct" it later on. But you could limit the trim to Windows if you want. 🎈

mewhhaha · 2021-01-28T15:27:20Z

I'll try it out!

mewhhaha · 2021-01-28T15:30:23Z

It's working. Thank you very much! Great work!

chenglou · 2021-01-28T15:31:16Z

Np. Don't like the circumvention that hides more bug, but it'll have to do for now =)

mewhhaha · 2021-01-28T15:32:23Z

Yes, I understand. I think so too. But at least the first impression for Windows users will be much better now!

mewhhaha and others added 15 commits January 25, 2021 20:08

Add windows fixes for files

188abb0

Remove dependencies not in original repo

12fd2b7

Change const to let again

87512b8

Remove amusing letants

54bf369

Remove space

fa5934f

Remove the two first spaces since it is trimmed

37848cb

Refactor separatefileandlocation function

744d5ff

Fix spacing

ed6d903

Add one space

f484263

Merge pull request #1 from mewhhaha/master

1c4cb22

Error diagnostics fixes for Windows

Move findlocationseparator into other function

8b64c80

Merge pull request #2 from mewhhaha/master

c8ab02a

Move findlocationseparator into other function

Reformatted some code

856de60

Merge pull request #3 from mewhhaha/master

e984823

Reformatted some code

Revert package.json

e724c5e

Reverting package.json as it is a unnecessary change

chenglou requested changes Jan 26, 2021

View reviewed changes

mewhhaha and others added 3 commits January 26, 2021 16:54

Update from comments

c077b63

Revert clien package-lock.json

c74302a

Merge pull request #4 from mewhhaha/master

94699d9

Update from comments

chenglou reviewed Jan 27, 2021

View reviewed changes

mewhhaha and others added 9 commits January 27, 2021 12:44

Use vscode uri utils for file scheme

33b3e30

Add semicolons

3fee884

Add two semicolons

7ecb7ad

Merge pull request #5 from mewhhaha/master

88268bb

Use the file scheme for everything

Remove trim call as to follow repos best practices

afece3f

Merge branch 'master' of github.com:johnny88/rescript-vscode

763ac4f

Move to lockfile package 2

0a873e1

This is a reversion

Include spaces in colon separator search

c73947c

We need to remove the spaces for URI.file to work

76a9da9

chenglou merged commit 2388fa2 into rescript-lang:master Jan 28, 2021

chenglou added a commit that referenced this pull request Jan 28, 2021

Trim file+location line before parsing

01a8ef7

See #71 (comment)

chenglou mentioned this pull request Feb 6, 2021

Unify various exec calls #81

Closed

5 tasks

Windows fixes #71

Windows fixes #71

Conversation

johnpangalos commented Jan 25, 2021

chenglou left a 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

mewhhaha Jan 26, 2021 • 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

chenglou commented Jan 27, 2021

Choose a reason for hiding this comment

chenglou Jan 27, 2021 • 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

chenglou commented Jan 28, 2021

mewhhaha commented Jan 28, 2021

chenglou commented Jan 28, 2021 • edited Loading

mewhhaha commented Jan 28, 2021

chenglou commented Jan 28, 2021

mewhhaha commented Jan 28, 2021

chenglou commented Jan 28, 2021

mewhhaha commented Jan 28, 2021 • edited Loading

chenglou commented Jan 28, 2021

mewhhaha commented Jan 28, 2021 • edited Loading

chenglou commented Jan 28, 2021 • edited Loading

mewhhaha commented Jan 28, 2021 • edited Loading

chenglou commented Jan 28, 2021

chenglou commented Jan 28, 2021

mewhhaha commented Jan 28, 2021 • edited Loading

mewhhaha commented Jan 28, 2021

mewhhaha commented Jan 28, 2021

chenglou commented Jan 28, 2021

mewhhaha commented Jan 28, 2021 • edited Loading

mewhhaha Jan 26, 2021 •

edited

Loading

chenglou Jan 27, 2021 •

edited

Loading

chenglou commented Jan 28, 2021 •

edited

Loading

mewhhaha commented Jan 28, 2021 •

edited

Loading

mewhhaha commented Jan 28, 2021 •

edited

Loading

chenglou commented Jan 28, 2021 •

edited

Loading

mewhhaha commented Jan 28, 2021 •

edited

Loading

mewhhaha commented Jan 28, 2021 •

edited

Loading

mewhhaha commented Jan 28, 2021 •

edited

Loading