ocaml-multicore / eio

Effects-based direct-style IO for multicore OCaml
Other
559 stars 72 forks source link

Implement symlink logic for Windows in test_symlink #766

Closed create2000 closed 1 month ago

create2000 commented 1 month ago

This PR updates tests for improved compatibility across Windows and Unix-based systems. Specifically, it addresses the differences in symlink permissions and behaviors between these systems, and modifies the test_symlink function to handle these differences gracefully.

Key Changes

  1. Introduced symlink_safe, that attempts to create symlinks with ~to_dir flags for directories. This function catches permission errors and skips symlink creation if necessary, particularly on systems that restrict symlink permissions (e.g., Windows).
  2. Updated the test_symlink logic to check for the operating system and conditionally create symlinks.

Testing: Tests were conducted on both Windows and Unix-like environments (WSL) to validate the compatibility improvements. Special attention was given to:

  1. Permission Errors: Verified that permission-related errors are gracefully handled without disrupting the test flow.
  2. Directory Existence and Structure: Confirmed that symlink paths and directories are correctly created or handled based on the system's symlink behavior.
create2000 commented 1 month ago

So I am thinking, initially i used the Sys.os_type to check if the current system is windows and if it is, it should skip the whole test. I am trying to know which would be a better approach between that and using this:

if not Unix.has_symlink then () else begin
patricoferris commented 1 month ago

I think has_symlink is what we want. We still want to test on Windows when this is available :))

create2000 commented 1 month ago

@patricoferris -- I have updated the code

patricoferris commented 1 month ago

Thanks @create2000 -- this looks better, but the git history now is a little confusing given you've made and undone changes. If you could reset back to the start and add just the changes for the symlink test that would be very much appreciated :))

create2000 commented 1 month ago

@patricoferris I deleted this branch and have sent a new PR to ensure that my git history is not confusing. Hope this doesn't add more work. Thank you