-
Notifications
You must be signed in to change notification settings - Fork 85
[WIP] Windows Support - esy + dune config #199
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from 11 commits
a6b7556
3564e77
f5afe29
1558a42
20c99e1
0bed757
0dfbb35
6e01742
20db56f
de1495b
78042af
11179c4
99e661a
7afcadd
3fa909b
269065c
3412436
c3fe600
2407df0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -130,10 +130,19 @@ let detect = (rootPath, bsconfig) => { | |
| let getEsyCompiledBase = (root) => { | ||
| let env = Unix.environment()->Array.to_list; | ||
|
|
||
| let correctSlashesOnWindows = (p) => { | ||
| if (Sys.win32) { | ||
| let slashRegex = Str.regexp("/"); | ||
| Str.global_replace(slashRegex, "\\\\", p); | ||
| } else { | ||
| p | ||
| } | ||
| }; | ||
|
|
||
| switch(Utils.getEnvVar(~env, "cur__original_root"), Utils.getEnvVar(~env, "cur__target_dir")) { | ||
| | (Some(projectRoot), Some(targetDir)) => Ok(Files.relpath(projectRoot, targetDir)) | ||
| | (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")) { | ||
|
||
| | Ok(commandEnv) => | ||
| switch (Json.parse(commandEnv)) { | ||
| | exception (Failure(message)) => | ||
|
|
@@ -149,7 +158,7 @@ let getEsyCompiledBase = (root) => { | |
| Json.get("cur__original_root", json) |?> Json.string, | ||
| Json.get("cur__target_dir", json) |?> Json.string, | ||
| ) { | ||
| | (Some(projectRoot), Some(targetDir)) => Ok(Files.relpath(projectRoot, targetDir)) | ||
| | (Some(projectRoot), Some(targetDir)) => Ok(Files.relpath(correctSlashesOnWindows(projectRoot), correctSlashesOnWindows(targetDir))) | ||
| | _ => Error("Couldn't find Esy target directory (missing json entries)") | ||
| } | ||
| ) | ||
|
|
@@ -219,7 +228,8 @@ let getStdlib = (base, buildSystem) => { | |
| switch (Utils.getEnvVar(~env, "OCAMLLIB")) { | ||
| | Some(esy_ocamllib) => Ok([esy_ocamllib]) | ||
| | None => | ||
| let%try_wrap esy_ocamllib = getLine("esy -q sh -- -c 'echo $OCAMLLIB'", ~pwd=base); | ||
| let echoCommand = Filename.quote("echo $OCAMLLIB"); | ||
| let%try_wrap esy_ocamllib = getLine("esy -q sh -- -c " ++ echoCommand, ~pwd=base); | ||
| [esy_ocamllib]; | ||
| }; | ||
| | Dune(Opam(switchPrefix)) => | ||
|
|
@@ -234,10 +244,11 @@ let isRunningInEsyNamedSandbox = () => { | |
| }; | ||
|
|
||
| let getExecutableInEsyPath = (exeName, ~pwd) => { | ||
| let whichCommand = Sys.win32 ? "where" : "which" | ||
bryphe marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| if (isRunningInEsyNamedSandbox()) { | ||
| getLine("which " ++ exeName, ~pwd) | ||
| getLine(whichCommand ++ " " ++ exeName, ~pwd) | ||
| } else { | ||
| getLine("esy which " ++ exeName, ~pwd) | ||
| getLine("esy " ++ whichCommand ++ " " ++ exeName, ~pwd) | ||
| } | ||
| }; | ||
|
|
||
|
|
@@ -302,4 +313,4 @@ let inferPackageManager = (projectRoot) => { | |
| Log.log("Detected `esy` dependency manager for local use"); | ||
| Ok(Esy) | ||
| }; | ||
| }; | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,18 +2,8 @@ | |
|
|
||
| let shellEscape = path => Filename.quote(path); | ||
|
|
||
| let execFull = (~input=?, ~pwd=?, ~env=Unix.environment(), cmd) => { | ||
| let cmd = | ||
| if (Sys.os_type == "Win32") { | ||
| Printf.sprintf("\"%s\"", cmd) | ||
| } else { | ||
| cmd | ||
| } | ||
| let env = switch pwd { | ||
| | None => env | ||
| | Some(pwd) => Array.map(item => String.length(item) > 4 && String.sub(item, 0, 4) == "PWD=" ? "PWD=" ++ pwd : item, env) | ||
| }; | ||
| let prevCwd = switch pwd { | ||
| let withCwd = (~cwd, f) => { | ||
| let prevCwd = switch cwd { | ||
| | None => None | ||
| | Some(pwd) => | ||
| let prevCwd = Unix.getcwd(); | ||
|
|
@@ -24,11 +14,32 @@ let execFull = (~input=?, ~pwd=?, ~env=Unix.environment(), cmd) => { | |
| Some(prevCwd) | ||
| } | ||
| } | ||
| let (cmd_out, cmd_in, cmd_err) = Unix.open_process_full(cmd, env); | ||
|
|
||
| let ret = f(); | ||
|
|
||
| switch prevCwd { | ||
| | None => () | ||
| | Some(prevCwd) => Unix.chdir(prevCwd) | ||
| }; | ||
|
|
||
| ret; | ||
| }; | ||
|
|
||
| let execFull = (~input=?, ~pwd=?, ~env=Unix.environment(), cmd) => { | ||
| let cmd = | ||
| if (Sys.os_type == "Win32") { | ||
| Printf.sprintf("\"%s\"", cmd) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't use %S it might be more robust with space and other special char ?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And use Sys.win32 ? |
||
| } else { | ||
| cmd | ||
| } | ||
| let env = switch pwd { | ||
| | None => env | ||
| | Some(pwd) => Array.map(item => String.length(item) > 4 && String.sub(item, 0, 4) == "PWD=" ? "PWD=" ++ pwd : item, env) | ||
| }; | ||
|
|
||
| let (cmd_out, cmd_in, cmd_err) = withCwd(~cwd=pwd, () => { | ||
| Unix.open_process_full(cmd, env); | ||
| }) | ||
|
|
||
| switch input { | ||
| | None => () | ||
|
|
@@ -104,8 +115,8 @@ let execOption = cmd => { | |
| } | ||
| }; | ||
|
|
||
| let execResult = cmd => { | ||
| let (lines, success) = execSync(cmd); | ||
| let execResult = (~cwd=?, cmd) => { | ||
| let (lines, success) = withCwd(~cwd, () => execSync(cmd)); | ||
| if (success) { | ||
| RResult.Ok(String.concat("\n", lines)) | ||
| } else { | ||
|
|
@@ -138,4 +149,4 @@ let execWithInput = (cmd, input) => { | |
| } { | ||
| | End_of_file => ([], false) | ||
| } | ||
| }; | ||
| }; | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the big diff here! I remove this first block that checks the environment for
esyenvironment 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-serverinstead of the test case. Theesy command-env --jsonstrategy 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bryphe checking for the env var is there to support esy named sandboxes, i.e. having
foo.jsoninstead ofesy.jsonand starting your editor withesy @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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 thecur__original_rootandcur__target_dirfrom the rootreason-language-serveresy config - this causes problems w/ the test.A couple possible options:
cur__original_root/cur__target_direnvironment via thesetenvstanza in duneStill looking into this but let me know if you have ideas. I think it's crucial to have a test covering the
esy+dunescenario here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reverted the change, but as expected
esytest fails... looking into it...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like
esywasn't being launched in the correct spot. I think we'll also have to runExamplesTest.exeoutside the esy sandbox, otherwise the environment gets polluted withcur__original_root/cur__target_dirThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I think that makes sense. I'm glad the issue seems to be limited only to tests.