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

Fix for #471 - Do not create a junction (soft link) for files #472

Closed
wants to merge 2 commits into from

Conversation

emtenet
Copy link
Contributor

@emtenet emtenet commented Apr 24, 2016

Proposed fix for #471.

When symlink_or_copy/2 cannot use file:make_symlink/2 on Windows due to the
user lacking SeCreateSymbolicLinkPrivilege it tries a fall back.

Detect the source type and use an appropriate fall back:

  • junction for directories and,
  • copy for files.

Improve error detection in win32_make_junction/2 and make it repeatable when
the target exists.

Remove dead code cp_r/2 that must have been the fall back method before junctions where introduced.

The bulk of the code is hidden behind opposite checks:
* os:type() /= {win32, _} in symlink_or_copy/2
* os:type() == {win32, _} in cp_r/2

symlink_or_copy/2 is always using win32_symlink/2 when file:make_symlink/2 fails.
When symlink_or_copy/2 cannot use file:make_symlink/2 on Windows due to the
user lacking SeCreateSymbolicLinkPrivilege it tries a fall back.

Detect the source type and use an appropriate fall back:
* junction for directories and,
* copy for files.

Improve error detection in win32_make_junction/2 and make it repeatable when
the target exists
@tsloughter
Copy link
Member

Shouldn't it still use robocopy?

@emtenet
Copy link
Contributor Author

emtenet commented Apr 25, 2016

I do not think the existing logic ever used robocopy. To get to the robocopy code you need to get through two contradictory checks:

  1. case os:type() /= {win32, _} L231
  2. case os:type() == {win32, _} L251
    I think this logic even hides errors from file:make_symlink/2 when used to symlink a file on non-Windows.

On the assumption that win32_symlink/2 (mklink) works fine for directories as it does not need any special privileges unlike file:make_symlink/2 then the question is do we need robocopy for copying a single file?

For non-dev releases, rlx_prv_assembler copies app directories L298 and the erts directory L492 with ec_file:copy/2. If robocopy is important, why is not use in these other places?

@Taure
Copy link
Contributor

Taure commented Sep 8, 2016

I think the code should look more like the one in rebar3. Guess this solution didn't get the same care as in rebar3. the one in rebar3 does a check if it is a directory if not. And then use cp_r function in that way L251 will need its check.

Guess that the other part did not get into relx.

If you compare the code in relx_util and the one for rebar3.

I can see if I can look into this.

@tsloughter
Copy link
Member

@ferd do you mind taking a look at this :). You know the windows issues :)

@ferd
Copy link
Collaborator

ferd commented May 31, 2018

I'm not too aware of that stuff, but I could always try to run the branch on a windows computer tomorrow morning, and give it a more thorough look at the same time.

@ferd
Copy link
Collaborator

ferd commented Jun 1, 2018

So the thing that worries me about this PR after reviewing (the relx tests are painful to check on windows and the branch being two years old makes it hard to check with a current rebar3) is that in restructuring the error handling, a non-win32 system that used to fallback to a recursive copy in case of a non-eperm error now just gives up in that branch without first retrying.

This changes semantics of non-windows stuff. As for the windows logic, it appears to be fine though, so it might be worth porting.

@tsloughter
Copy link
Member

@emtenet rebase please.

But before I can accept this one I think the non-win32 logic needs to be the same, unless there is a reason I'm missing that it gets rid of the cp_r path for non-win32 systems. Pretty sure that is still a required fallback.

@tsloughter
Copy link
Member

@emtenet ping. If you can let me know if someone else needs to pick this PR up for you.

@tolbrino
Copy link
Contributor

@tsloughter Is #714 ok for you?

@tolbrino
Copy link
Contributor

#714 has been merged, so this can be closed.

@tsloughter tsloughter closed this Apr 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants