Dereference symbolic links when copying files from ct data folder

[gonzalobf]

When running rebar3 ct, rebar_prv_common_test copies the data folders
to a different location. If {name}_SUITE_data folder contains relative
symbolic links, the symbolic links might stop working since the path
might be different.

A solution is to add the -L option to cp so it dereferences the
symbolic links and copies the original content in the target file.

This is only done for the data folder but and won't change any code that
previously used rebar_file_utils:cp_r.

I asked in erlangforum a few days ago because I thought it was common test
problem but today I realised that is rebar3 copying the files.

I'm not sure if this problem happens with Windows too since I don't have
any computer with it installed.

I'm not 100% sure about if this change will be wanted so I did a quick
implementation to discuss it.

Thank you

[ferd]

That's a good fix! For the implementation though, instead of using a boolean as a third argument, we should pass in options like [{dereference, true|false}] so that instead of checking for true or false to add the -L, we check proplists:get_value(dereference, Opts, false), which future-proofs the function.

[gonzalobf]

That's a good fix! For the implementation though, instead of using a boolean as a third argument, we should pass in options like [{dereference, true|false}] so that instead of checking for true or false to add the -L, we check proplists:get_value(dereference, Opts, false), which future-proofs the function.

Thank you :)

I added a commit changing the code to use a proplist. I'm not sure how should I add -L to the OptStr, so I used a case inside the cp_r function but I thought about these other possibilities:

• Moving the code to another function:

get_options_str(Options) ->
    DefaultOptStr = "-Rp"
    case proplists:get_value(dereference, Options, false) of
        true -> DefaultOptStr ++ "L";
        false -> DefaultOptStr
     end.

• Parse all the values in Options in a recursive way. It will be helpful for adding future options.

get_options_str(Options) ->
    get_options_str(Options, "-Rp").

get_options_str([], OptionsStr) ->
    OptionsStr;
get_options_str([{dereference, false} | Rest], OptionsStr) ->
    get_options_str(Rest, OptionsStr);
get_options_str([{dereference, true} | Rest], OptionsStr) ->
    get_options_str(Rest, OptionsStr ++ "L").

Let me know if you prefer any of these or if there is another way that I didn't think about

[ferd]

I prefer the current (and first) option, because it has clearer semantics in situations where the option is passed multiple times in the proplists: [{dereference, true}, {dereference, false}, {dereference, true}] would yield -RpL in the first case and -RpLL in the recursive case.

[ferd]

It would probably be worth it to write a quick test in https://github.com/erlang/rebar3/blob/main/apps/rebar/test/rebar_file_utils_SUITE.erl for this.

Since the code actually does nothing for windows, it would be alright to do a case os:type() of ... check that documents the act of not dereferencing anything. In fact, looking at https://docs.microsoft.com/en-us/windows-server/administration/windows-commands/robocopy it appears that robocopy dereferences symlinks by default and we should use /sl to do a literal link copy in the default case.

[gonzalobf]

I prefer the current (and first) option, because it has clearer semantics in situations where the option is passed multiple times in the proplists: [{dereference, true}, {dereference, false}, {dereference, true}] would yield -RpL in the first case and -RpLL in the recursive case.

I didn't think about that possibility. I'll add the test so we make sure this works and leave the rest as it is.

[gonzalobf]

In fact, looking at https://docs.microsoft.com/en-us/windows-server/administration/windows-commands/robocopy it appears that robocopy dereferences symlinks by default and we should use /sl to do a literal link copy in the default case.

@ferd I added the tests for cp_r. I tried to be as compact and clear as possible but they might need a change. Let me know want do you think.

I expect the test cp_r_copies_files to for fail and cp_r_dereferences_symbolic_links to pass since it is the default behaviour in Windows as you said. Would you mind to approve the workflow so we confirm it is true? (I need to install a VM to run the test it locally)

Once we confirmed they fail, I'll will adapt the Windows code so it uses /sl by default and doesn't use it when we pass [{dereference, true}]

[gonzalobf]

Thank you, it failed as expected. I pushed another commit doing the changes for Windows

[gonzalobf]

@ferd I run the tests in a Windows VM and I think I fixed them.

I thought the files where copied only using robocopy but cp_r_win32 does also file:copy when the Source is a file and not a folder. I expanded my test so cp_r_win32 file:copy is called when copying src/symbolic and robocopy with sub_dir.

I hope I didn't miss any other copy case.

[ferd]

Looks well-detailed, nice tests. As soon as CI passes, this is in.
Thanks for the contribution!

[gonzalobf]

Thank you for your help!