stdlib/os: handle symlinks in copy/move functions

[rominf]

• Added optional options argument to copyFile, copyFileToDir, and
  copyFileWithPermissions. By default, symlinks are followed (copy files
  symlinks point to). On Windows, options == {cfSymlinkAsIs} do not affect.
• copyDir and copyDirWithPermissions copy symlinks as they are (instead of
  skipping them as it was before). On Windows, symlinks are followed.
• moveFile and moveDir move symlinks as they are (instead of skipping them
  sometimes as it was before).
• Added optional followSymlinks argument to setFilePermissions.

See also: nim-lang/RFCs#319

[rominf]

Well, we get:

/home/vsts/work/1/s/lib/pure/os.nim(1653, 6) Error: 'copyFile' is not GC-safe as it calls 'expandSymlink'

What's the way to fix this? I'm not qualified enough yet to fix this...

[timotheecour]

Error: 'copyFile' is not GC-safe as it calls 'expandSymlink'
What's the way to fix this? I'm not qualified enough yet to fix this...

follow the error messages in logs; it shows it fails with bin/nim c -o:bin/nimsuggest -d:release -d:danger nimsuggest/nimsuggest.nim;

then run that locally, it fails; then see why it fails: it's because nimsuggest/nimsuggest.nim.cfg contains --threads:on

then run nim r --lib:lib tests/stdlib/tos.nim , it now also fails

the fix: add gcsafe annotations for decl+impl for createSymlink, expandSymlink

• PR welcome for adding a single test that simply imports a bunch of stdlib files and compiles with --threads:on, to prevent future regressions along those lines that wouldn't be caught by simply compiling nimsuggest

[juancarlospaco]

Check that those still work on NimScript because they currently do.

I really appreciate the effort, if you are going to improve it remember that Nim can not differentiate a file from a folder. 🤷

[rominf]

How other languages handle symlinks in copyDir analogs

Python

shutil.copytree(src, dst, symlinks=False, ignore=None, copy_function=copy2, ignore_dangling_symlinks=False, dirs_exist_ok=False)
...
If symlinks is true, symbolic links in the source tree are represented as symbolic links in the new tree and the metadata of the original links will be copied as far as the platform allows; if false or omitted, the contents and metadata of the linked files are copied to the new tree.

Docs: https://docs.python.org/3/library/shutil.html#shutil.copytree

Rust

There is no copyDir in the standard library.

SO question: https://stackoverflow.com/questions/26958489/how-to-copy-a-folder-recursively-in-rust

C++

options controlling the effects of copy() on symbolic links
--
none | Follow symlinks (default behavior)

Docs: https://en.cppreference.com/w/cpp/filesystem/copy_options

Go

There is no copyDir in the standard library.

A most popular library for Go copies symlinks instead of following them.

Sources: https://github.com/otiai10/copy/blob/1dfb4e4bf632edd08bc1a853160ad03ff18e68b5/options.go#L66

Julia

cp(src::AbstractString, dst::AbstractString; force::Bool=false, follow_symlinks::Bool=false)
...
If follow_symlinks=false, and src is a symbolic link, dst will be created as a symbolic link.

Docs: https://docs.julialang.org/en/v1/base/file/#Base.Filesystem.cp

Conclusions

Some languages don't have copyDir at all; some follow symlinks, some do not. 🤷‍♂️

My thoughts

I'm tempted to make it the same way as C++ does: follow symlinks by default and to pass the options (ORed enum CopyOptions which has values none, copy_symlinks, skip_symlinks).

Reasoning:

1. I think we need to keep backward compatibility, so copyFile should follow symlinks.
2. I think we need to be consistent. So, since copyDir doesn't copy symlinks at all, which is a bug, in my opinion, we can break backward compatibility and (to be consistent) follow symlinks.
3. In the future, there could be a need for more options for copying than "follow symlinks." Also, the large variety of implementations show that it's not a trivial function. So I would prefer to be on the safe side, adding the options ORed enum which can be extended in the future.

So, what do you think?

[timotheecour]

I think we need to keep backward compatibility, so copyFile should follow symlinks.

agreed here, and also it's a reasonable default for copyFile

In the future, there could be a need for more options for copying than "follow symlinks."

I agree. But I'd prefer using a

type CopyOptions* = object
  followSymlinks*: bool
proc initCopyOptions(): CopyOptions = ...

for same rationale as in jsonutils #15133 (comment)
instead of a raw bool or instead of a set[SomeOptions]. An object is more flexible than a set of enum (eg can add in future non-bool options; there are other advantages); the main advantage of set, efficiency, is not a concern here given the cost of copy.

eg:

let opt = initCopyOptions()
opt.follow
copyDir(src, dst, opt)

I think we need to be consistent. So, since copyDir doesn't copy symlinks at all, which is a bug, in my opinion, we can break backward compatibility and (to be consistent) follow symlinks.

that's the only part i disagree with, and I disagree strongly. following symlinks should be opt in when it comes to copyDir which is recursive. Imagine you call:

mkdir foo
touch foo/test.txt
ln -s ~ foo/myhome

copyDir "foo", "foo2"
before PR: foo/myhome symlink is skipped
after PR with followSymlinks defaulting to true for coyDir: user's home directory ~ is (recursively) copied, a large regression
after PR with followSymlinks defaulting to false for coyDir: the symlink is copied as is

and then there are other problems too, eg handling internal symlinks etc (these are in fact common), or even cyclic links.

[rominf]

@juancarlospaco, you wrote:

Check that those still work on NimScript because they currently do.

I don't think so, I've just tried it:

$ cat test_nimscript.nims
#!/usr/bin/env nim
mode = ScriptMode.Silent

import os

copyFile("./test_nimscript.nims", "./test_nimscript_copy.nims")
$ ./test_nimscript.nims
Hint: used config file '/home/rominf/dev/nim/config/nim.cfg' [Conf]
Hint: used config file '/home/rominf/dev/nim/config/config.nims' [Conf]
/tmp/test_nimscript.nims(6, 1) Error: this proc is not available on the NimScript/js target; usage of 'copyFile' is an {.error.} defined at /home/rominf/dev/nim/lib/pure/os.nim(1650, 1)

[timotheecour]

@rominf for nimscript, try instead cpFile, cpDir (https://nim-lang.github.io/Nim/nimscript.html)

no need to add API's to std/nimscript though (the preferred way is for nims to import os intead of using its own aliases in nimscript), but yes, we should ensure cpFile, cpDir still work

just extend tests/test_nimscript.nims with the minimum needed

[Araq]

IMO most sensible behavior is as follows: for copyDir on windows, try to copy symlinks, and if it fails, write a sensible error message on stderr (with runtime context), but don't fail. Future work can add options for copyDir to control this.

What?! No, we don't produce output on stderr in undocumented ways.

[rominf]

@Araq Ok, I understand your point. How do you propose to solve this?

[Araq]

Ok, I understand your point. How do you propose to solve this?

Pretend for the time being that Windows has no symlinks and fix copyDir for Posix only.

[Araq]

Merging this now, will clean it up a bit later.

[timotheecour]

@rominf how about on windows, what is the behavior now? (even if it hasn't changed); when reading the changelog, one might wonder

[timotheecour]

what's the behavior for windows when source is a symlink? noop? remove source? something else?

[timotheecour]

in future work, copyDirWithPermissions and copyDir should be refactored with a common implementation copyDirImpl to avoid the duplication (but see also timotheecour#570 which suggests using a single copyDir with options, which would make copyDirImpl un-necessary)

[timotheecour]

• pre-existing but what's the behavior when dest already exists? (on each OS)
• also, we should document that dest.parentDir doesn't need to exist

[timotheecour]

we should add:
doAssert fpath2.readFile == "some text"

[timotheecour]

use a template to avoid this entire block being duplicated:
eg:

template test(algo) =
  ...
test copyFile
test copyFileWithPermissions

[timotheecour]

likewise, there should be a way to avoid duplication even for copyFileToDir block

[timotheecour]

why not: when symlinksAreHandled: ?

[timotheecour]

ditto, when symlinksAreHandled:?

[timotheecour]

@rominf excellent change, and also the added tests are really good. I added some post-review comments for future PRs;

IMO we should still support symlinks on windows, but deferring this to a future PR makes sense as this PR was already complex enough