Skip to content

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

Sign up for GitHub

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

Open
wants to merge 19 commits into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
a6b7556
Initial Windows fixes
bryphe Nov 24, 2018
3564e77
Enable all tests
bryphe Nov 24, 2018
f5afe29
Revert changes to State.re
bryphe Nov 24, 2018
1558a42
Fix up whitespace
bryphe Nov 24, 2018
20c99e1
Upgrade esy for Windows
bryphe Nov 26, 2018
0bed757
Factor out 'withCwd' logic
bryphe Nov 26, 2018
0dfbb35
Fix getting '$OCAMLLIB' on Windows
bryphe Nov 26, 2018
6e01742
Fix robustness of path checking
bryphe Nov 26, 2018
20db56f
Fix off-by-one issue
bryphe Nov 26, 2018
de1495b
Revert removal of environment detection
bryphe Nov 29, 2018
78042af
Fix esy execution path when detecting using command-env
bryphe Nov 29, 2018
11179c4
Run tests directly from executable
bryphe Nov 30, 2018
99e661a
Check path for ExamplesTests
bryphe Nov 30, 2018
7afcadd
Move files to predictable structure; run tests directly
bryphe Dec 1, 2018
3fa909b
add windows conditional
jaredly Mar 25, 2019
269065c
Merge branch 'master' into bryphe/windows-esy-fixes
jaredly Mar 25, 2019
3412436
fix
jaredly Mar 25, 2019
c3fe600
lol
jaredly Mar 25, 2019
2407df0
Merge branch 'master' into bryphe
jaredly Jan 5, 2020
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
67 changes: 39 additions & 28 deletions src/analyze/BuildSystem.re
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")) {
Copy link
Contributor Author

@bryphe bryphe Nov 24, 2018

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

@anmonteiro anmonteiro Nov 26, 2018

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

@bryphe bryphe Nov 27, 2018

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

@bryphe bryphe Nov 29, 2018

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

@bryphe bryphe Nov 29, 2018

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

@anmonteiro anmonteiro Nov 29, 2018

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))
| (_, _) =>
switch (Commands.execResult("esy command-env --json")) {
| Ok(commandEnv) =>
switch (Json.parse(commandEnv)) {
| exception (Failure(message)) =>
Log.log("Json response");
Log.log(commandEnv);
Error("Couldn't find Esy target directory (invalid json response: parse fail): " ++ message);
| exception exn =>
Log.log(commandEnv);
Error("Couldn't find Esy target directory (invalid json response) " ++ Printexc.to_string(exn));
| json =>
Json.Infix.(
switch (
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))
| _ => Error("Couldn't find Esy target directory (missing json entries)")
}
)
let correctSlashesOnWindows = (p) => {
if (Sys.win32) {
let slashRegex = Str.regexp("/");
Str.global_replace(slashRegex, "\\\\", p);
} else {
p
}
| err => err
};

let prevCwd = Unix.getcwd();
Unix.chdir(root);
let res = Commands.execResult("esy command-env --json")
Unix.chdir(prevCwd);

switch (res) {
| Ok(commandEnv) =>
switch (Json.parse(commandEnv)) {
| exception (Failure(message)) =>
Log.log("Json response");
Log.log(commandEnv);
Error("Couldn't find Esy target directory (invalid json response: parse fail): " ++ message);
| exception exn =>
Log.log(commandEnv);
Error("Couldn't find Esy target directory (invalid json response) " ++ Printexc.to_string(exn));
| json =>
Json.Infix.(
switch (
Json.get("cur__original_root", json) |?> Json.string,
Json.get("cur__target_dir", json) |?> Json.string,
) {
| (Some(projectRoot), Some(targetDir)) => Ok(Files.relpath(correctSlashesOnWindows(projectRoot), correctSlashesOnWindows(targetDir)))
| _ => Error("Couldn't find Esy target directory (missing json entries)")
}
)
}
| err => err
}
};

Expand Down Expand Up @@ -219,7 +229,7 @@ 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%try_wrap esy_ocamllib = getLine("esy b echo $OCAMLLIB", ~pwd=base);
[esy_ocamllib];
};
| Dune(Opam(switchPrefix)) =>
Expand All @@ -234,10 +244,11 @@ let isRunningInEsyNamedSandbox = () => {
};

let getExecutableInEsyPath = (exeName, ~pwd) => {
let whichCommand = Sys.win32 ? "where" : "which"
if (isRunningInEsyNamedSandbox()) {
getLine("which " ++ exeName, ~pwd)
getLine(whichCommand ++ " " ++ exeName, ~pwd)
} else {
getLine("esy which " ++ exeName, ~pwd)
getLine("esy " ++ whichCommand ++ " " ++ exeName, ~pwd)
}
};

Expand Down Expand Up @@ -302,4 +313,4 @@ let inferPackageManager = (projectRoot) => {
Log.log("Detected `esy` dependency manager for local use");
Ok(Esy)
};
};
};
4 changes: 2 additions & 2 deletions src/analyze_example_tests/ExamplesTests.re
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ let projects = [
("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"), */
("example-esy-dune-project", ["lib", "bin"], "esy"),
];


Expand Down Expand Up @@ -83,4 +83,4 @@ if (failures == []) {
| `FileFail(uri, message) => print_endline("- Failed to get compilation info for " ++ uri ++ " : " ++ message)
});
exit(10)
}
}
2 changes: 1 addition & 1 deletion util/Infix.re
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ let logIfAbsent = (message, x) => switch x {
};

let maybeConcat = (a, b) => {
if (b != "" && b.[0] == '/') {
if (b != "" && (b.[0] == '/' || b.[1] == ':')) {
b
} else {
fileConcat(a, b)
Expand Down