Repository: fsspec/filesystem_spec
Pull request #1059 (2022-10-03 12:15:22)

localfs: add link/symlink/islink methods
This PR adds `link`/`symlink` APIs to create hardlink/symlink, and `islink` to check for symlinks.
The APIs mirror what we have in `os` and `os.path`.

There are some open questions about APIs. In DVC, we use `hardlink()`, `symlink()` and `is_symlink()` APIs which I find to be much better, but I went with other APIs to be consistent with python. Python has not been very consistent though ([`Path.is_symlink()`](https://docs.python.org/3/library/pathlib.html#pathlib.Path.is_symlink), [`Path.symlink_to`](https://docs.python.org/3/library/pathlib.html#pathlib.Path.symlink_to), [`Path.hardlink_to`](https://docs.python.org/3/library/pathlib.html#pathlib.Path.hardlink_to), etc).

Also, do we need these APIs in `AbstractFileSystem`?

martindurant: Do I remember that there was a plan to implement this kind of thing in a filesystem-specific `.os.path`-like attribute/accessor? (cc @efiop )

> Also, do we need these APIs in AbstractFileSystem?

No, I think links are implemented on too few backends.
skshetry: > Do I remember that there was a plan to implement this kind of thing in a filesystem-specific `.os.path`-like attribute/accessor? (cc @efiop )

The proposed `fs.path` only performs path manipulations. @efiop might elaborate further.
The `symlink`/`hardlink` do modify the filesystem, so I don't think they fit inside that. `islink` could be pushed into that if we expand its role in the same way as `os.path`, but currently, it is strictly path manipulations. `islink` in this way is similar to `fs.isdir`/`fs.isfile`/`fs.exists`, and `os.path.{isdir,isfile,exists}` respectively.
efiop: @martindurant Saugat is right, `fs.path` is only for path manipulation. link/symlink/islink don't belong there.
skshetry: @efiop @martindurant, any thoughts on API namings?
martindurant: I am happy to follow the names in `os`, as the posix CLI corresponding operations are achieved with flags like `-l/L`, `-s`.

Before merging, any thoughts on more adding options for following symbolic links or not? Normally, I feel that `rm`/`unlink` (delete) should never follow links, so the option is not needed.
efiop: Naming makes sense to me. This is universal enough and in the spirit of FUSE/syscalls/etc. The only one that is a bit special is `islink`, which is a convenience python method, really, since you can easilly figure the same out from stat or `fs.info` call (we have `islink` there already in localf).

One thing that worries me here is that this is localfs only right now, while all three methods should probably be a part of spec, since multiple filesystems support it (local, ssh, hdfs, probably more). That would again be more in a spirit of FUSE.

Looking forward to @martindurant 's opinion on these things. 🙂 
martindurant: > all three methods should probably be a part of spec

Let's put them here and allow to sit for a while. Implementation in the spec would be just NotImplemented anyway

> local, ssh, hdfs, probably more

I don't think so? Does HDFS really?

Patch:
diff --git a/fsspec/implementations/local.py b/fsspec/implementations/local.py
index 693902c..d0d707b 100644
--- a/fsspec/implementations/local.py
+++ b/fsspec/implementations/local.py
@@ -138,6 +138,19 @@ class LocalFileSystem(AbstractFileSystem):
         path2 = self._strip_protocol(path2).rstrip("/")
         shutil.move(path1, path2)
 
+    def link(self, src, dst, **kwargs):
+        src = self._strip_protocol(src)
+        dst = self._strip_protocol(dst)
+        os.link(src, dst, **kwargs)
+
+    def symlink(self, src, dst, **kwargs):
+        src = self._strip_protocol(src)
+        dst = self._strip_protocol(dst)
+        os.symlink(src, dst, **kwargs)
+
+    def islink(self, path) -> bool:
+        return os.path.islink(self._strip_protocol(path))
+
     def rm_file(self, path):
         os.remove(path)
 


Test patch:
diff --git a/fsspec/implementations/tests/test_local.py b/fsspec/implementations/tests/test_local.py
index aabc1d6..e1b6ab7 100644
--- a/fsspec/implementations/tests/test_local.py
+++ b/fsspec/implementations/tests/test_local.py
@@ -785,3 +785,25 @@ def test_numpy_fromfile(tmpdir):
     arr = np.arange(10, dtype=dt)
     arr.tofile(fn)
     assert np.array_equal(np.fromfile(fn, dtype=dt), arr)
+
+
+def test_link(tmpdir):
+    target = os.path.join(tmpdir, "target")
+    link = os.path.join(tmpdir, "link")
+
+    fs = LocalFileSystem()
+    fs.touch(target)
+
+    fs.link(target, link)
+    assert fs.info(link)["nlink"] > 1
+
+
+def test_symlink(tmpdir):
+    target = os.path.join(tmpdir, "target")
+    link = os.path.join(tmpdir, "link")
+
+    fs = LocalFileSystem()
+    fs.touch(target)
+
+    fs.symlink(target, link)
+    assert fs.islink(link)