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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disable Windows symlinking test #759

Open
patricoferris opened this issue Oct 2, 2024 · 16 comments
Open

Disable Windows symlinking test #759

patricoferris opened this issue Oct 2, 2024 · 16 comments
Labels
good first issue Good for newcomers windows

Comments

@patricoferris
Copy link
Collaborator

patricoferris commented Oct 2, 2024

Symlinking on Windows is a priveleged operation. From OCaml's Unix.symlink documentation.

The other caveat [on windows] is that by default symbolic links are a privileged operation. Administrators will always need to be running elevated (or with UAC disabled) and by default normal user accounts need to be granted the SeCreateSymbolicLinkPrivilege via Local Security Policy (secpol.msc) or via Active Directory.

Unix.has_symlink can be used to check that a process is able to create symbolic links.

Currently, I think the tests can fail because of this. We should only run symlink tests on Windows if we can (i.e. any tests that use Unix.symlink).

@create2000
Copy link
Contributor

Hello @patricoferris

My name is Anthony Onah, an outreachy applicant for the December 24 cohort. I will like to work on this good first issue with your assistance. I have attached a screenshot of my terminal running the Eio "Hello world."
Screenshot 2024-10-02 172749

@patricoferris
Copy link
Collaborator Author

@create2000 did you not already sign up for working on #575 ?

@create2000
Copy link
Contributor

@patricoferris I was going to sign up for the #322 but it was referenced to the #575 which i do not know if it is a good first issue. If it is a good first issue, then i'd go with it.

@patricoferris
Copy link
Collaborator Author

It is a challenging issue I think (as @talex5 pointed out). But if you feel confident then I think even just pushing that PR in the right direction based on the feedback is a great contribution, even if it doesn't quite get merged in time for the end of the contribution phases (merged PRs are not the only way to contribute).

@create2000
Copy link
Contributor

Confident? Not so sure, but i'd like to take up the challenge. Afterall that's what makes us know our strength. I have sent a screenshot of my terminal running the "Hello world" on top. Do i need to resend it? And do i need to begin from this issue(cloning the repo to work on) or do i work straight from the #322 repo?

@patricoferris
Copy link
Collaborator Author

I think you want to fork this repository and fetch @avsm's branch (https://github.com/avsm/eio/tree/sockopt) and work on the changes suggested at the end of #575 -- does that make sense ?

@patricoferris patricoferris mentioned this issue Oct 4, 2024
9 tasks
@create2000
Copy link
Contributor

@patricoferris . Yes, it does. I have been on it. Will relate back to you soon on some challenges i have been encountering. Thank you.

@create2000
Copy link
Contributor

Hello @patricoferris -- In trying to run a test on the code to check if it disables Windows symlinking tests, I am constantly getting this error:
Screenshot 2024-10-06 212210
I have checked the Eio documentation, and have installed it as required. Perhaps I am missing something?

@patricoferris
Copy link
Collaborator Author

Hey @create2000,

It looks like you are trying to run the compiler by hand. Eio uses the dune tool for compiling OCaml programs. Running dune build should be enough here. You can read more about dune on their documentation site.

@create2000
Copy link
Contributor

Thank you for the direction. I was already on it. When i run dune build and after dune runtest, to verify that the Windows symlinking test is passed, do i need to specifically test run it on Windows to check if the tests are passed, or do i need to add test inputs and possibly print out to the console the inputs (whatever it may be) to verify the test?

@patricoferris
Copy link
Collaborator Author

We need to use the Unix.has_symlink function to dynamically disable any tests on a platform (most likely windows) that doesn't support symlinking. This will sometimes work on windows if the user has the correct privileges.

@create2000
Copy link
Contributor

Hello @patricoferris -- so, i have been trying to get the issue solved but have been running into some errors ranging from

unbound value.
to

This expression has type string but an expression was expected of type

Let me walk you through what i have done and where i am stuck at:

I was able to read and to my best of knowledge and some stack overflow answers, i set up a test_symlink.ml
file where i wrote the logic to disable windows symlink. I proceeded to install some deps and modules which were been shown as errors whenever i run the dune build and dune runtest. To get a broader details of the errors, i ran dune runtest --verbose

At the moment, this is how my file looks like:

open Alcotest
open Eio

let disable_symlink_creation () =
Eio.Fiber.run (fun env ->
let fs = Eio.Stdenv.fs env in
let dir = Eio.Path.(fs / "test_dir") in
let target_file = Eio.Path.(dir / "target_file.txt") in
let symlink_file = Eio.Path.(dir / "symlink_to_target.txt") in

(* Create the test directory *)
Eio.Path.mkdir ~perm:0o755 dir;

(* Create a target file *)
Eio.Path.with_open_out ~create:(`If_missing 0o644) target_file (fun flow ->
  Eio.Flow.copy_string "This is the target file content" flow
);

(* Attempt to create a symlink and handle the expected failure *)
let result =
  try
    Eio.Path.symlink ~link_to:target_file symlink_file;
    false  (* If we reach this line, symlink creation succeeded, which we don't want *)
  with
  | _ -> true  (* An exception indicates that symlink creation failed as expected *)
in

(* Check that symlink creation was indeed prevented *)
check bool "Symlink creation should be disabled" true result;

(* Clean up the test files and directory *)
Eio.Path.remove target_file;
Eio.Path.rmdir dir

)

let () =
disable_symlink_creation ()
Screenshot 2024-10-09 142322
Screenshot 2024-10-09 142341

And i am repeatedly getting this error message : Error: Unbound value Eio.Fiber.run

I think there's something I am not doing right and would appreciate pointing me to the right direction.

@patricoferris
Copy link
Collaborator Author

Hey @create2000,

I think there has been some confusion here. Let me help clear things up.

There are tests for Windows that use symlinking to check for certain properties. At the moment, there is actually only one:

let test_symlink env () =
.

This test fails by raising an error. However, Unix.symlink may fail because the current user on Windows is not allowed to make symlinks. We don't want the test to fail in that case so instead we should allow it to succeed by simply doing nothing.

And i am repeatedly getting this error message : Error: Unbound value Eio.Fiber.run

Hmm, I'm curious where did you see that this function could be used? There is no Eio.Fiber.run function: https://ocaml-multicore.github.io/eio/eio/Eio/Fiber/index.html. There is Eio_main.run but that is handled in the test.ml file.

This issue should not require any new files to be created.

@create2000
Copy link
Contributor

Thank you so greatly @patricoferris. Now I understand better.

@create2000
Copy link
Contributor

Hello @patricoferris -- a clarification. Would checking the system OS to see if it's Windows and thereby skipping the test_symlink logic suffice? I have something like this:

 
  if Sys.os_type = "Win32" then
    Printf.printf "Skipping test_symlink on Windows.\n"
  else
    let cwd = Eio.Stdenv.cwd env in
    try_mkdir (cwd / "sandbox");
    Unix.symlink ~to_dir:true ".." "sandbox\\to-root";
    Unix.symlink ~to_dir:true "subdir" "sandbox\\to-subdir";
    Unix.symlink ~to_dir:true "foo" "sandbox\\dangle";

Or should I rather handle the situation where the current user lacks permission to create symlinks?

@create2000
Copy link
Contributor

Hello @patricoferris -- I made a PR for this Issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers windows
Projects
None yet
Development

No branches or pull requests

2 participants