[WIP] Windows Support - esy + dune config #199
Conversation
| @@ -130,32 +130,42 @@ let detect = (rootPath, bsconfig) => { | |||
| let getEsyCompiledBase = (root) => { | |||
| let env = Unix.environment()->Array.to_list; | |||
|
|
|||
| switch(Utils.getEnvVar(~env, "cur__original_root"), Utils.getEnvVar(~env, "cur__target_dir")) { | |||
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
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_direnvironment via thesetenvstanza 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.
There was a problem hiding this comment.
I reverted the change, but as expected esy test fails... looking into it...
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Ah I think that makes sense. I'm glad the issue seems to be limited only to tests.
|
Awesome thanks for jumping into this! |
|
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. |
|
May I bump this? |
src/analyze/BuildSystem.re
Outdated
| | (_, _) => | ||
| switch (Commands.execResult("esy command-env --json")) { | ||
| switch (Commands.execResult(~cwd=root, "esy command-env --json")) { |
There was a problem hiding this comment.
so this root doesn't exist anywhere...
| let execFull = (~input=?, ~pwd=?, ~env=Unix.environment(), cmd) => { | ||
| let cmd = | ||
| if (Sys.os_type == "Win32") { | ||
| Printf.sprintf("\"%s\"", cmd) |
There was a problem hiding this comment.
Shouldn't use %S it might be more robust with space and other special char ?
|
I have tried to make it work on windows see: Et7f3@4681fb3. One of the main difference:
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. |
|
@Et7f3 awesome, thanks for taking up the torch! Do you want to open up a new PR? |
Issue:
reason-language-serverisn't working for Windows configs. Uncommenting out the test is the quickest way to reproduce this.There were several issues:
esy whichgives 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 usingesy whereon Windows, which works as expected.E:\orC:\, 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 馃帀
