Bugfix - Dir - Windows: Paths not loaded correctly #251

bryphe · 2020-05-05T00:02:55Z

Issue: @glennsl and I started experimenting with using dir and fp in Onivim 2, in this PR: onivim/oni2#1703 - the API is perfect for us!

However, we hit a difficult-to-troubleshoot crash on startup when we integrated dir, and it wasn't clear what was going wrong - it was crashing before we even ran our CLI parsing or initialized our logging system.

It turned out, there were a couple of problems we hit:

First, there this was this exception:

(Invalid_argument "First token in path C:\\Users\\bryphe is not absolute.")
Raised at file "src/fp/Fp.re", line 220, characters 6-98
Called from file "src/dir/Dir.re", line 117, characters 16-35
Called from file "src/dir/Dir.re", line 187, characters 13-48
Called from file "src/dir/Dir.re", line 341, characters 8-20

This came from Fp, because it doesn't handle the windows back-slashes currently (this is documented limitation, but it's problematic with Dir - because it by default picks up windows-style paths for the directories, and breaks when it uses Fp to manipulate paths on windows).

I fixed this by normalizing the paths prior to sending to Fp (similar to what we do in esy) - however, the best longer-term fix would be to have Fp handle back-slashes too.

After fixing that, there was still a crash, and this time it was a native crash - not from OCaml. It turns out the sh_get_folder_path c stub wasn't actually returning the path string, so the runtime would crash natively when trying to process the Val_unit result as a string. I fixed this by returning a constructed option(string) from the API.

Lastly, I tweaked the API - because the directory construction methods have the possibility of throwing, I changed them to be deferred - it'll construct and give out the paths on request. This is preferable for use case, because we'd like to initialize our logging infra prior to anything that can throw, so if there is an error or exception, we can capture it and drop a crash log.

kyldvs · 2020-05-05T05:27:46Z

This looks great! Thanks for debugging. I'll let @jordwalke take a look before merging since he wrote most of this.

bryphe · 2020-05-06T02:45:24Z

Thanks @kyldvs ! Just chatted with @jordwalke - I realized I need to update the dir test cases to pick up the new API. It's also curious why the native crash wasn't manifesting on Windows - I would've expected a crash in Dir.home, so I'll take a look at that too.

kyldvs · 2020-05-06T03:03:39Z

It's possible windows CI just isn't actually running things, for example our test cmd is a no-op in windows so it always passes, but it will run in bash environments:

https://github.com/facebookexperimental/reason-native/blob/master/scripts/test-ci.cmd

src/dir/Dir.re

jordwalke · 2020-05-06T08:40:12Z

The test errors seem unrelated? It looks like refmterr tests behaving differently. Not sure how your changes could have caused that. I'll merge anyways if possible.

src/dir/dir.c

jordwalke · 2020-05-06T08:45:43Z

Looks like one of the test outputs has "because it is in the condition of an if-statement" in one of the error messages, which doesn't match what is expected, but that was likely added in a new ocaml compiler. I wonder if it's not using the lockfile to install / run tests.

.ci/build-platform.yml

bryphe · 2020-05-06T19:59:49Z

.ci/build-platform.yml

+      - script: "esy x TestDev.exe --filter Dir"
+        displayName: "TEMPORARY: Run subset of tests cross-platform"
+      # TODO: Fix full test run!
+      # - script: "esy test"
+      #  displayName: "Test command"


Since it looks like there are several other failures that are unrelated - I tweaked the CI to just run a subset of tests that are known-good across all platforms:

Wanted to see a green CI run for Dir/Fs/(Fp|Path) on all platforms. But I'm happy to revert this.

I'm not sure that the script is working as intended - it may be simpler to just run the test runner directly, and see if any of the ambient environment variables like REASON_NATIVE_ROOT that are needed could be inferred within the executable, rather than a wrapper script.

Okay thanks - this should unblock the merge and then I'll look into the failures asynchronously.

Does --filter Dir only run the Dir tests? Is that what you wanted?

Right, they're the only tests I know that work cross-plat at the moment. (It looks like it runs Path, Fs too) - so I was just picking up the subset that works cross-plat. Ideally the next step would be to unblock the remaining tests, so we can remove the --filter and just run esy x TestCI on all platforms as part of the continuous integration builds.

bryphe · 2020-05-06T20:01:24Z

tests/__tests__/fs/Fs_test.re

+let normalizePathSeparator = {
+  let backSlashRegex = Str.regexp("\\\\");
+  pathStr => pathStr |> Str.global_replace(backSlashRegex, "/");
+};
+
+let unresolvedExePath = Fp.absoluteExn(Sys.executable_name |> normalizePathSeparator);


This was also blocked Windows - the same issue that I set up for Dir - when we get a raw-windows-path-with-backslashes, we crash.

It makes me think that, maybe for now, we should update the Fp.absoluteExn API like:

let absoluteExn = (~normalizeBackSlashes=Sys.win32, path) => { .... };

At least in the mean-time, it would work OK for both Dir and these test cases - and the normalization behavior could be overridden if desired. LMK what you think, @jordwalke

I agree. Though I would be okay with making the complexity of cross platform paths front and center, at the time they parse the string. So that could be something like splitting the path parsing entrypoints into a couple of different named functions that make them acknowledge what they are doing. That is a little annoying, but the point is that once you get things into Paths, you never ever have to think about this stuff again.

let absoluteCurrentPlatformPathExn = (path) => { .... }; let absolutePlatformPathExn = (~fromPlatform, path) => { .... };

The second having no default argument. What do you think? Too annoying? These should be seldom used - but maybe in reality, it's not the case?

Seems reasonable (although maybe a bit verbose) - but at least this is only needed at 'ingestion' of paths. And if you use Dir - you shouldn't really need to touch these very often (especially if we provide the executingDirectory and currentWorkingDirectory as well somewhere).

The only other issue I see is that right now Fp doesn't have a concept of platform - so we'll just have to bring that over from Dir. But seems OK! I'll give it a shot and see what you think.

jordwalke · 2020-05-14T04:24:03Z

I think that this is needed in the esy.json file to make the tests pass. We run them internally with that version and they pass, but the esy.json here doesn't have that. Want to try adding that and seeing if it makes them pass?

  "devDependencies": {
    "ocaml": "~4.7.1"
  }

bryphe · 2020-05-15T21:13:08Z

Hey sorry for the slow reply @jordwalke ! Thanks for the tip re: 4.7.1 - I ran on OSX and almost everything passes, except for the refmterr test:

 FAIL  refmterr
  • refmterr › file_SyntaxError

    expect.string(received).toEqual(expected)
    
    Expected value to equal:
      "\027[27m\027[24m\027[2m\027[1m# Unformatted Error Output:\027[27m\027[24m\027[22m
      \027[2m# \027[22m\027[2mFile \"tests/__tests__/refmterr/__fixtures__/file_SyntaxError/file_SyntaxError_7.re\", line 20, characters 22-23:\027[22m
      \027[2m# \027[22m\027[2mError: Syntax error: ')' expected\027[22m
...
    Received:
      "\027[27m\027[24m\027[2m\027[1m# Unformatted Error Output:\027[27m\027[24m\027[22m
      \027[2m# \027[22m\027[2mFile \"tests/__tests__/refmterr/__fixtures__/file_SyntaxError/file_SyntaxError_7.re\", line 20, characters 22-23:\027[22m
      \027[2m# \027[22m\027[2mError: Unclosed \"{\" (opened line 10, column 4)\027[22m
...
    Difference:
      # Unformatted Error Output:
      # File "tests/__tests__/refmterr/__fixtures__/file_SyntaxError/file_SyntaxError_7.re", line 20, characters 22-23:
      # Error: Unclosed "{" (opened line 10, column 4)# Unformatted Error Output:
      # File "tests/__tests__/refmterr/__fixtures__/file_SyntaxError/file_SyntaxError_7.re", line 20, characters 22-23:
      # Error: Syntax error: ')' expected22m

I'll push up the change and the test command w/o the filter and we'll see what happens - maybe it's just an issue on my local machine 🤞

kyldvs · 2020-05-16T00:19:27Z

@bryphe I recall having issues with this refmterr test when updating things, it might just be a bad test that we can disable.

bryphe · 2020-05-26T23:15:01Z

@bryphe I recall having issues with this refmterr test when updating things, it might just be a bad test that we can disable.

Thanks @kyldvs ! I disabled that test on Windows. Linux / macOS are green now.

Looks like, unfortunately, there are still failures on Windows, though (besides the Dir/Fp modules that were fixed in this PR).

bryphe · 2020-05-27T00:17:18Z

I just applied the --filter Dir for the Windows test - so we can run the subset that were unblocked here. As a next step - we can start to unblock more and enable the full test suite

jordwalke

Let's try to remove the dependency on unix/str (at least for fp) so that fp can work as a compile-to-js path library as well.

Windows Fixes: Return path; defer loading

fd0225c

facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label May 5, 2020

bryphe mentioned this pull request May 5, 2020

feat(workspace/#640): persistence infrastructure + persist workspace and window state onivim/oni2#1703

Merged

2 tasks

bryphe added 2 commits May 4, 2020 17:49

Fix up compilation / tests

e29b67f

Formatting

5af3b18

jordwalke reviewed May 6, 2020

View reviewed changes

src/dir/Dir.re Outdated Show resolved Hide resolved

jordwalke approved these changes May 6, 2020

View reviewed changes

src/dir/dir.c Show resolved Hide resolved

bryphe and others added 4 commits May 6, 2020 12:38

Get a subset of windows tests running on CI

16ae7ac

Add msg parameter to getOrThrow

f3cb55d

More CI tweaks

0db5cc5

macOS 10.13 -> 10.14

bd9d7e8

bryphe commented May 6, 2020

View reviewed changes

.ci/build-platform.yml Show resolved Hide resolved

bryphe commented May 6, 2020

View reviewed changes

Add ~fromPlatform to absolute Fp methods

249a54c

bryphe mentioned this pull request May 7, 2020

Dir: Add executablePath and workingDirectory #252

Open

Add devDep for ocaml 4.7.1, update lockfile

44dd8cd

bryphe added 3 commits May 26, 2020 13:10

Fix broken yml

83246db

Remove failing test case

fb1eb8d

Fix invalid yml again

fefe67a

Run a subset of tests on windows

37fbe06

neq -> ne

5a7a756

This was referenced Nov 25, 2020

fix: normalize filesystem paths to always be / character grain-lang/grain#433

Merged

Remove path separator normalization when possible grain-lang/grain#437

Closed

jordwalke approved these changes Feb 17, 2021

View reviewed changes

jordwalke merged commit aec0ac6 into reasonml:master Feb 17, 2021

phated mentioned this pull request Oct 1, 2021

The uname call in Fp causes it to not be JSOO portable #267

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bugfix - Dir - Windows: Paths not loaded correctly #251

Bugfix - Dir - Windows: Paths not loaded correctly #251

bryphe commented May 5, 2020

kyldvs commented May 5, 2020

bryphe commented May 6, 2020

kyldvs commented May 6, 2020

jordwalke commented May 6, 2020

jordwalke commented May 6, 2020

bryphe May 6, 2020

jordwalke May 7, 2020

jordwalke May 7, 2020 •

edited

Loading

bryphe May 7, 2020

bryphe May 6, 2020 •

edited

Loading

jordwalke May 7, 2020

bryphe May 7, 2020

jordwalke commented May 14, 2020

bryphe commented May 15, 2020

kyldvs commented May 16, 2020

bryphe commented May 26, 2020

bryphe commented May 27, 2020

jordwalke left a comment

Bugfix - Dir - Windows: Paths not loaded correctly #251

Bugfix - Dir - Windows: Paths not loaded correctly #251

Conversation

bryphe commented May 5, 2020

kyldvs commented May 5, 2020

bryphe commented May 6, 2020

kyldvs commented May 6, 2020

jordwalke commented May 6, 2020

jordwalke commented May 6, 2020

bryphe May 6, 2020

Choose a reason for hiding this comment

jordwalke May 7, 2020

Choose a reason for hiding this comment

jordwalke May 7, 2020 • edited Loading

Choose a reason for hiding this comment

bryphe May 7, 2020

Choose a reason for hiding this comment

bryphe May 6, 2020 • edited Loading

Choose a reason for hiding this comment

jordwalke May 7, 2020

Choose a reason for hiding this comment

bryphe May 7, 2020

Choose a reason for hiding this comment

jordwalke commented May 14, 2020

bryphe commented May 15, 2020

kyldvs commented May 16, 2020

bryphe commented May 26, 2020

bryphe commented May 27, 2020

jordwalke left a comment

Choose a reason for hiding this comment

jordwalke May 7, 2020 •

edited

Loading

bryphe May 6, 2020 •

edited

Loading