fix: masking on macos #42
Conversation
MacOS ridiculously symlinks their `$TMPDIR = /var/folders/lz/<somestring>/T/` to `/private/var/folders/...`. This means that if for whatever reason someone prints the real path of a tmp folder instead of the symlinked path, TestO wouldnt mask it. Also, $TMPDIR on MacOS has a trailing slash for some reason, when on Linux `Filename.get_temp_dir_name` does not which also broke some things
|
Apologies for the extra diffs, my ocaml formatter does that for some reason in this repo |
| let real_tmpdir = Unix.realpath tmpdir in | ||
| let tmpdir = | ||
| if String.ends_with ~suffix:Filename.dir_sep tmpdir then | ||
| String.sub tmpdir 0 (String.length tmpdir - 1) |
There was a problem hiding this comment.
| String.sub tmpdir 0 (String.length tmpdir - 1) | |
| String.sub tmpdir 0 (String.length tmpdir - String.length Filename.dir_sep) |
| (* Some OS's *cough cough MacOS* symlink their $TMPDIR to a different path *) | ||
| (* This means that if an ocmal program ever uses realpath, they may print a valid *) | ||
| (* But different temp dir that's not equal to $TMPDIR *) | ||
| (* So let's include that in the pattern *) |
| let tmpdir_pat = Re.Pcre.quote tmpdir in | ||
| let real_tmpdir_pat = Re.Pcre.quote real_tmpdir in | ||
| let tmpdir_pat = Printf.sprintf "(?:%s|%s)" tmpdir_pat real_tmpdir_pat in |
There was a problem hiding this comment.
Looks good to me. One issue is that Unix.realpath requires OCaml >= 4.13 which is now 3 years old. It's fine if we really have to but not ideal. String.ends_with also requires OCaml >= 4.13 but is easier to reimplement ourselves.
If I understand correctly, there are two issues:
- The trailing slash
- Multiple temp folders
For (1), we need tests. In particular, we need to check that "/tmp//" works.
For (2), we also need tests. If depending on OCaml 4.13 is an issue, we could leave this responsibility to the user by letting them specify multiple temp folders. They can actually already do this today by applying two filters: one with the original path obtained with Filename.get_temp_dir_name () and another that is Filename.get_temp_dir_name () |> Unix.realpath.
There was a problem hiding this comment.
Another issue: String.sub tmpdir 0 (String.length tmpdir - 1) assumes the length of Filename.dir_sep is 1 which is not guaranteed. Better use String.length tmpdir - String.length Filename.dir_sep.
mask temporary paths obtained after applying Unix.realpath which is not available until ocaml 4.13. See semgrep/testo#42
) This is a workaround to the issue of temporary folders being symlinks as described in semgrep/testo#42. This is not done in Testo because I find it too restrictive for Testo to require OCaml >= 4.13 at this time. The other bug addressed by semgrep/testo#42 (trailing slashes in temp folder path) will be fixed in Testo. Test plan: CI checks must pass but symlinked temp folders still fail for other reasons: ``` $ ln -s /tmp link-to-tmp $ TMPDIR=$(pwd)/link-to-tmp make test ``` 1. If the temp folder is relative, the tests fail because of a chdir. It's understandable. 2. With an absolute path such as `/home/martin/link-to-tmp`, 12 tests fail. Here's the error we get for test 1d84c9d09005 (`Language Server (unit) > Session Targets > Test multiple workspaces with some dirty (only_git_dirty: false)`): ``` FAIL: The test raised an exception: Failure: cannot make path "/home/martin/semgrep2/link-to-tmp/test_workspace_9c93dc2d-def6-48f2-84cc-65eeade3e0d2" relative to project root "/tmp/test_workspace_9c93dc2d-def6-48f2-84cc-65eeade3e0d2". cwd: /home/martin/semgrep2 realpath for .: { Rfpath.fpath = .; rpath = (Rpath.Rpath /home/martin/semgrep2); cwd = (Rpath.Rpath /home/martin/semgrep2) } Sys.argv: ./test ``` It is a known bug that is not simple to fix. See #10003 Some other language server tests fail with 30-second timeouts. This should be fixed such that exceptions cause an immediate failure.
|
I redid this for compatibility with OCaml 4.08 and left the call to |
MacOS ridiculously symlinks their
$TMPDIR = /var/folders/lz/<somestring>/T/to
/private/var/folders/.... This means that if for whatever reason someone prints the real path of a tmp folder instead of the symlinked path, TestO wouldnt mask it.Also, $TMPDIR on MacOS has a trailing slash for some reason, when on Linux
Filename.get_temp_dir_namedoes not which also broke some things.Note that I didn't test this on linux, and am hoping there are some CI tests there, but if not will need someone to test it for me