Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Windows Support - esy + dune config #199

Open
wants to merge 19 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion examples/example-esy-dune-project/lib/.merlin
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
EXCLUDE_QUERY_DIR
B /Users/jared/clone/tools/reason-language-server/examples/example-esy-dune-project/_esy/default/store/b/hello-bb5af4c3/default/lib/.lib.objs/byte
B C:/Users/User/clone/reason-language-server/examples/example-esy-dune-project/_esy/default/store/b/hello-022fadb7\default/lib/.lib.objs/byte
S .
FLG -open Lib -w -40
31 changes: 21 additions & 10 deletions src/analyze/BuildSystem.re
Original file line number Diff line number Diff line change
Expand Up @@ -187,8 +187,17 @@ let detectFull = projectDir => {
let getEsyCompiledBase = () => {
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")) {
Copy link
Contributor Author

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 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!

Copy link
Collaborator

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.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?

Copy link
Contributor Author

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 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.

Copy link
Contributor Author

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 esy test fails... looking into it...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

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.

| (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")) {
| Ok(commandEnv) =>
Expand All @@ -206,7 +215,7 @@ let getEsyCompiledBase = () => {
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)")
}
)
Expand Down Expand Up @@ -276,9 +285,10 @@ let getStdlib = (base, buildSystem) => {
switch (Utils.getEnvVar(~env, "OCAMLLIB")) {
| Some(esy_ocamllib) => Ok([esy_ocamllib])
| None =>
let command = v < "0.5.6" ?
"esy -q sh -- -c 'echo $OCAMLLIB'" : "esy -q sh -c 'echo $OCAMLLIB'";
let%try_wrap esy_ocamllib = getLine(command, ~pwd=base);
let echoCommand = Filename.quote("echo $OCAMLLIB");
let commandPrefix = v < "0.5.6" ?
"esy -q sh -- -c " : "esy -q sh -c ";
let%try_wrap esy_ocamllib = getLine(commandPrefix ++ echoCommand, ~pwd=base);
[esy_ocamllib];
};
| Dune(Opam(switchPrefix)) =>
Expand All @@ -293,12 +303,13 @@ let isRunningInEsyNamedSandbox = () => {
};

let getExecutableInEsyPath = (exeName, ~pwd) => {
let whichCommand = Sys.win32 ? "where" : "which"
bryphe marked this conversation as resolved.
Show resolved Hide resolved
let ret = if (isRunningInEsyNamedSandbox()) {
getLine("which " ++ exeName, ~pwd)
getLine(whichCommand ++ " " ++ exeName, ~pwd)
} else {
getLine("esy which " ++ exeName, ~pwd)
getLine("esy " ++ whichCommand ++ " " ++ exeName, ~pwd)
};
if (Sys.win32) {
/*if (Sys.win32) {
switch ret {
| RResult.Ok(ret) =>
let ret = if (isRunningInEsyNamedSandbox()) {
Expand All @@ -309,9 +320,9 @@ let getExecutableInEsyPath = (exeName, ~pwd) => {
ret
| Error(a) => Error(a)
}
} else {
} else {*/
ret
}
//}
};

let getCompiler = (rootPath, buildSystem) => {
Expand Down
4 changes: 2 additions & 2 deletions src/analyze_example_tests/ExamplesTests.re
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,6 @@ let checkExampleProject = (describe, name, rootPath, sourcePaths, prepareCommand
let esyProjects = [
("dune", ["src", "both"], "esy"),
("dune-complex", ["src", "both", "awesome"], "esy"),
("example-esy-dune-project", ["lib", "bin"], "esy"),
];

// Don't run the esy examples on windows, they're failing right now.
Expand All @@ -64,6 +63,7 @@ let projects = (Sys.os_type == "Unix" ? esyProjects : []) @ [
("example-react", ["src", "__tests__"], "npm install"),
("name_with_underscore", ["src"], "npm install"),
("bs-3.1.5", ["src"], "npm install"),
("example-esy-dune-project", ["lib", "bin"], "esy"),
];

// let main = (baseDir, _verbose, args) => {
Expand Down Expand Up @@ -118,4 +118,4 @@ let baseDir = Sys.getcwd();
// | `FileFail(uri, message) => print_endline("- Failed to get compilation info for " ++ uri ++ " : " ++ message)
// });
// exit(10)
// }
// }
41 changes: 26 additions & 15 deletions util/Commands.re
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The 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 ?

Copy link
Contributor

Choose a reason for hiding this comment

The 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 => ()
Expand Down Expand Up @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion util/Infix.re
Original file line number Diff line number Diff line change
Expand Up @@ -43,4 +43,4 @@ let maybeConcat = (a, b) => {
}
};

let (/+) = fileConcat;
let (/+) = fileConcat;