[WIP] Windows Support - esy + dune config by bryphe · Pull Request #199 · jaredly/reason-language-server

bryphe · 2018-11-24T03:05:29Z

Issue: reason-language-server isn't working for Windows configs. Uncommenting out the test is the quickest way to reproduce this.

There were several issues:

esy which gives a cygwin style path on Windows, so it's not what we want - for those paths, we then run them in the native shell, which will fail. I switched this to using esy where on Windows, which works as expected.
Path concatenation was not working in all cases, because it made assumptions about the form of the root directory - on Windows, a root directory could be something like E:\ or C:\, so added some detection there.

Still a bunch of things to fix - I'll leave comments for some open issues.

With my hacks in place, it's feeling pretty nice in Oni on Windows 🎉

bryphe · 2018-11-24T03:06:39Z

src/analyze/BuildSystem.re

 let getEsyCompiledBase = (root) => {
  let env = Unix.environment()->Array.to_list;

-  switch(Utils.getEnvVar(~env, "cur__original_root"), Utils.getEnvVar(~env, "cur__target_dir")) {


Sorry for the big diff here! I remove this first block that checks the environment for esy environment variables, so it impacted the indentation.

This caused problems specifically in our test environment - it would pick up the esy root/target for reason-language-server instead of the test case. The esy command-env --json strategy seems more reliable, so I just deferred to that. @anmonteiro - let me know if I missed something here!

@bryphe checking for the env var is there to support esy named sandboxes, i.e. having foo.json instead of esy.json and starting your editor with esy @foo code .. I use them pretty heavily personally and this is the only way we can support them right now.

I think this needs to stay there. Can you think of an alternative?

Thanks for the details @anmonteiro - supporting sandboxes makes sense.

I believe the issue here is limited to our tests - when we run esy dune exec ExamplesTests, it picks up the cur__original_root and cur__target_dir from the root reason-language-server esy config - this causes problems w/ the test.

A couple possible options:

We might be able to clear out the cur__original_root/cur__target_dir environment via the setenv stanza in dune

Alternatively, we could set an environment variable that we're in a test environment, and skip the sandbox that way.

Still looking into this but let me know if you have ideas. I think it's crucial to have a test covering the esy+dune scenario here.

I reverted the change, but as expected esy test fails... looking into it...

Looks like esy wasn't being launched in the correct spot. I think we'll also have to run ExamplesTest.exe outside the esy sandbox, otherwise the environment gets polluted with cur__original_root/cur__target_dir

Ah I think that makes sense. I'm glad the issue seems to be limited only to tests.

src/analyze/BuildSystem.re

util/Infix.re

jaredly · 2018-11-24T03:39:11Z

Awesome thanks for jumping into this!

src/analyze/BuildSystem.re

anmonteiro · 2018-11-26T16:53:26Z

I went through the PR and I think there's still some important issues to address here, specifically the support for esy named sandboxes, which we'll lose if we keep the current approach.

jaredly

see @anmonteiro's concerns

saitonakamura · 2019-02-10T17:15:33Z

May I bump this?

jaredly · 2019-03-25T13:15:01Z

src/analyze/BuildSystem.re

+  | (Some(projectRoot), Some(targetDir)) => Ok(Files.relpath(correctSlashesOnWindows(projectRoot), correctSlashesOnWindows(targetDir)))
  | (_, _) =>
-    switch (Commands.execResult("esy command-env --json")) {
+    switch (Commands.execResult(~cwd=root, "esy command-env --json")) {


so this root doesn't exist anywhere...

Et7f3 · 2019-08-11T04:20:27Z

util/Commands.re

+let execFull = (~input=?, ~pwd=?, ~env=Unix.environment(), cmd) => {
+  let cmd =
+    if (Sys.os_type == "Win32") {
+      Printf.sprintf("\"%s\"", cmd)


Shouldn't use %S it might be more robust with space and other special char ?

And use Sys.win32 ?

Et7f3 · 2019-08-11T04:20:37Z

I have tried to make it work on windows see: Et7f3@4681fb3.
When I wanted to open a PR I have seen this PR so I wanted to compare implementations.

One of the main difference:

which/where: where on windows search for all binaries. So if you have 2 ocaml in your PATH (that is my case) it will output both.
which/cygpath: cygpath with the option -w convert the result of which to windows path. which return only the first result and cygpath.

Also the native ocaml compiler use cygpath and it work fine see https://github.com/alainfrisch/flexdll/blob/5fde2bd70a088cd11271925aa6352fc29ec94291/reloc.ml#L230-L234 they use -m I have used -w 🤔 the difference is / or \ both are supported on windows 🤷‍♂.

Thanks bryphe for https://github.com/jaredly/reason-language-server/pull/199/files#diff-39f5b0dce8e3aa4cb0c9d1ced2d6d55aR289-R290 :) I could remove my global OCAMLLIB variable.

esy named sandbox don't seem to work properly.

jaredly · 2019-08-11T05:11:50Z

@Et7f3 awesome, thanks for taking up the torch! Do you want to open up a new PR?

bryphe added 4 commits November 23, 2018 18:53

Initial Windows fixes

a6b7556

Enable all tests

3564e77

Revert changes to State.re

f5afe29

Fix up whitespace

1558a42

bryphe commented Nov 24, 2018

View reviewed changes

src/analyze/BuildSystem.re Outdated Show resolved Hide resolved

bryphe commented Nov 24, 2018

View reviewed changes

src/analyze/BuildSystem.re Outdated Show resolved Hide resolved

bryphe commented Nov 24, 2018

View reviewed changes

util/Infix.re Outdated Show resolved Hide resolved

jaredly requested a review from anmonteiro November 24, 2018 03:39

anmonteiro reviewed Nov 26, 2018

View reviewed changes

src/analyze/BuildSystem.re Show resolved Hide resolved

jaredly requested changes Nov 26, 2018

View reviewed changes

bryphe added 5 commits November 26, 2018 13:51

Upgrade esy for Windows

20c99e1

Factor out 'withCwd' logic

0bed757

Fix getting '$OCAMLLIB' on Windows

0dfbb35

Fix robustness of path checking

6e01742

Fix off-by-one issue

20db56f

bryphe mentioned this pull request Nov 28, 2018

Refmt: CRLF is treated differently than LF reasonml/reason#1518

Closed

bryphe added 5 commits November 28, 2018 19:06

Revert removal of environment detection

de1495b

Fix esy execution path when detecting using command-env

78042af

Run tests directly from executable

11179c4

Check path for ExamplesTests

99e661a

Move files to predictable structure; run tests directly

7afcadd

bryphe mentioned this pull request Dec 1, 2018

[WIP] Azure pipelines #207

Closed

jaredly approved these changes Dec 14, 2018

View reviewed changes

jaredly added 2 commits March 25, 2019 06:55

add windows conditional

3fa909b

Merge branch 'master' into bryphe/windows-esy-fixes

269065c

jaredly requested changes Mar 25, 2019

View reviewed changes

jaredly added 2 commits March 25, 2019 13:50

fix

3412436

lol

c3fe600

Et7f3 reviewed Aug 11, 2019

View reviewed changes

Et7f3 mentioned this pull request Aug 12, 2019

RLS on Windows #318

Merged

zetashift mentioned this pull request Aug 21, 2019

[windows] [vscode] RLS won't start due to $OCAMLLIB parse error #319

Closed

Merge branch 'master' into bryphe

2407df0

Conversation

bryphe commented Nov 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jaredly commented Nov 24, 2018

Uh oh!

Uh oh!

anmonteiro commented Nov 26, 2018

Uh oh!

jaredly left a comment

Choose a reason for hiding this comment

Uh oh!

saitonakamura commented Feb 10, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Et7f3 commented Aug 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jaredly commented Aug 11, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

bryphe commented Nov 24, 2018 •

edited

Loading

Et7f3 commented Aug 11, 2019 •

edited

Loading