Add root to Path & root constructor #196
Conversation
|
I think this looks reasonable. Is it possible to make the change binary compatible? Something like val root: Path = ...
def root(s: String): Path = ... |
Thanks for a great library! Hopefully this change will increase interest among windows shell environment developers. This fixes #201 The essence of the fix is to support paths with a leading slash (`'/' or '\'`) on Windows. In this PR, this path type is referred to as `driveRelative`, due to subtle differences in how it is handled on `Windows` versus other platforms. Example code: ```scala val p1 = java.nio.file.Paths.get("/omg") printf("isAbsolute: %s\n", p1.isAbsolute) printf("%s\n", p1) printf("%s\n", os.Path("/omg")) ``` Output on Linux or OSX: ```sh isAbsolute: true /omg /omg ``` On Windows: ```scala isAbsolute: false \omg java.lang.IllegalArgumentException: requirement failed: \omg is not an absolute path at os.Path.<init>(Path.scala:474) at os.Path$.apply(Path.scala:426) at oslibChek$package$.main(oslibChek.sc:11) at oslibChek$package.main(oslibChek.sc) ``` ### Background On Windows, a `driveRelative` path is considered relative because a drive letter prefix is required to fully resolve it. Like other platforms, Windows also supports `posix relative` paths, which are relative to the `current working directory`. Because all platforms, including `Windows`, support absolute paths and `posix relative` paths, it's convenient when writing platform-independent code to view `driveRelative` paths as absolute paths, as they are on other platforms. On Windows, the `current working drive` is an immutable value that is captured on jvm startup. Therefore, a `driveRelative` path on Windows unambiguously refers to a unique file with a hidden drive letter. This PR treats `driveRelative` paths as absolute paths in Windows, and has no effect on other platforms. #### Making `os/test/src/PathTests` platform independent This PR enables `Unix()` tests in `os/test/src/PathTests.scala` so they also verify required semantics in Windows. ### How this PR relates to #170 and #196 The purpose of #196 seems to be to add full support on Windows, without resorting to `java.nio` classes and methods. The addition of `os.root(...)` or `os.drive(...)` would be convenient for people writing windows-only code, whereas this PR is intended to allow writing platform-independent code, so the concerns seem to be orthogonal. It seems important not to alter `Path.segments` in a way that is incompatible with `java.nio` segments. An alternate way to preserve drive letter information would be to add a method to one of the `Path.scala` traits that returns a drive letter if appropriate on `Windows` and an empty String elsewhere. ```scala trait PathChunk { def segments: Seq[String] def rootPrefix: String // empty String, or Windows drive letter def ups: Int } ``` With this approach, the following assertion would be valid on all platforms: ```scala os.Path(pwd.rootPrefix) ==> pwd ``` Then `driveRoot` in this PR would become `pwd.rootPrefix`. There is another (perhaps never used?) very subtle feature of `Windows` filesystem: each drive has a different value for `pwd`. It may not be necessary to model this in `os-lib`, since these values are immutable in a running jvm, and there are existing workarounds. ### Additional Details regarding unique aspects of Windows Filesystem Paths For completeness, the following lengthy `scala` REPL session illustrates Path values returned by `java.nio.file.Paths.get()`, some of which might be surprising. <details> Relevant information: My system drives are C: and F:, but not J: First, some unsurprising results: ```scala c:\work-directory> scala.bat scala> import java.nio.file.Paths Welcome to Scala 3.3.1-RC5 (17.0.2, Java OpenJDK 64-Bit Server VM). Type in expressions for evaluation. Or try :help. scala> Paths.get("/") val res0: java.nio.file.Path = \ scala> Paths.get("/").isAbsolute val res2: Boolean = false scala> Paths.get("/").toAbsolutePath val res1: java.nio.file.Path = C:\ scala> Paths.get("c:/").toAbsolutePath C:\work-directory scala> Paths.get("f:/").toAbsolutePath F:\ scala> Paths.get("f:/..").normalize.toAbsolutePath f:\ scala> Paths.get("c:/..").normalize.toAbsolutePath c:\ ``` Now some less obvious results: ```scala scala> Paths.get("c:").toAbsolutePath C:\work-directory scala> Paths.get("f:").toAbsolutePath F:\ scala> Paths.get("f:/..").normalize.toAbsolutePath f:\ scala> Paths.get("c:/..").normalize.toAbsolutePath c:\ scala> Paths.get("c:..").normalize.toAbsolutePath C:\work-directory scala> Paths.get("f:..").normalize.toAbsolutePath F:\.. scala> Paths.get("F:Users") val res1: java.nio.file.Path = F:Users scala> Paths.get("F:Users").toAbsolutePath val res2: java.nio.file.Path = F:\work-directory\Users scala> Paths.get("j:").toAbsolutePath java.io.IOError: java.io.IOException: Unable to get working directory of drive 'J' at java.base/sun.nio.fs.WindowsPath.toAbsolutePath(WindowsPath.java:926) at java.base/sun.nio.fs.WindowsPath.toAbsolutePath(WindowsPath.java:42) ... 35 elided Caused by: java.io.IOException: Unable to get working directory of drive 'J' ... 37 more ``` The error message returned for a non-existing drive seems to imply that existing drives have a `working directory`. This is consistent with the `Windows API` as described here: [GetFullPathNameA](https://learn.microsoft.com/en-gb/windows/win32/api/fileapi/nf-fileapi-getfullpathnamea) I can set the `working drive` for F: in a `CMD` session before I start up the `jvm`, and it does affect the output of `Paths.get()`. ```CMD c:\work-directory>F: f:\>cd work-directory F:\work-directory>scala.bat Welcome to Scala 3.3.1-RC5 (17.0.2, Java OpenJDK 64-Bit Server VM). Type in expressions for evaluation. Or try :help. scala> import java.nio.file.Paths scala> Paths.get("/") val res0: java.nio.file.Path = \ scala> Paths.get("/").toAbsolutePath val res1: java.nio.file.Path = C:\ scala> Paths.get("f:") val res2: java.nio.file.Path = f: scala> Paths.get("f:").toAbsolutePath val res3: java.nio.file.Path = F:\work-directory scala> Paths.get("c:").toAbsolutePath val res4: java.nio.file.Path = C:\work-directory scala> Paths.get("c:") val res5: java.nio.file.Path = c: ``` Regarding the interpretation of a drive letter expression not followed by a '/' or '\\', the rule is that it represents the `working directory` for that drive. These values are immutable: after the `JVM` starts up it's not possible to change a drive `working directory`. </details>
|
@lihaoyi seems to work all right |
|
@lihaoyi I pushed a commit that adds a separate field |
| val fileSystem = FileSystems.newFileSystem(uri, env); | ||
| val p = os.root("/", fileSystem) / "test" / "dir" | ||
| assert(p.root == "/") | ||
| assert(p.fileSystem == fileSystem) |
There was a problem hiding this comment.
Could we add a few more operations here? e.g.
-
Running more operations we expect should work on
foo.jar:os.read,os.read.*,os.write,os.write.*,os.list,os.walk,os.remove,os.remove.all,os.copy,os.move,os.makeDir -
Some negative examples of running operations that we should expect to fail when run on an
os.Pathinsidefoo.jar:os.symlink,os.tempwith an explicitdir: os.Path,os.stat,os.perms,os.proc()passing theos.Pathinsidefoo.jarboth as aos.Shellableandcwd/stdin/stdout/stderr. Assert on the exception type so we know what we're getting back on failure; the underlyingjava.nioexception is fine for the cases where things already fail when they should, though some things likeos.procwe may have to check and throw our own exception -
Further manipulation of the paths with a custom filesystem via
os.up,/ os.RelPath,/ os.SubPath, and ensuring the root ends up in the same place -
Checking that
os.Paths with the same segments but differentroots or differentfilesystemdo not return==to each other -
os.relativeToandos.subRelativeTobetween paths on different filesystems should probably throw an exception, so let's assert that. Should they also throw exceptions when run between paths on different roots on the same filesystem?
This is enough stuff that it's probably worth moving the test suite into a separate file
|
@szymon-rd looks good! Last comments on my side and then we can merge this:
It's a small code change, but with pretty wide interaction surface area with the rest of the library, so we should make sure it's reasonably thoroughly tested and documented so people can know what to expect when they inevitably start hitting edge cases. Once that's done we can merge this and cut a point release |
|
@lihaoyi I think I've addressed all of your comments |
|
Looks good to me. Thanks for working through all the feedback! It was definitely a lot of back and forth. It seems MIMA is complaining, but I suspect the problem is Scala 2.11.x only, since the encoding of I think I'd be ok with with just adding the MIMA exclusion to shut it up, or just turning off MIMA for 2.11 entirely. 2.11 support is already pretty best-effort-ish (we removed it once in the past and added it back), so I don't feel like we should contort the code to preserve strict compatibility there just for 2.11.x. @lolgab @lefou let me know if you have any opinion on this, otherwise I will add a MIMA exclusion and merge this. |
I wondered about the history on this, but I didn't find anything except #66 (which didn't come with an explanation) and #47 (which the requester withdrew). If y'all are considering simply dropping it again, I encourage you to do so. |
|
FS Root support, esp. for Windows, was my personal blocker for a 1.0 release. Now, it looks like that we can even keep binary compatibility, which is really nice. I think, we should cut a |
|
Added a mima exclude, will squashmerge once green |
|
@szymon-rd let me know if you'd like me to tag a release, or if you want to wait for your other PR to land first before tagging one |
|
I'd rather have the piping go into the release too |
Resolve #170
This PR adds a
def root(root: String, fileSystem: FileSystem): Pathinospackage object. Additionally, it addsrootandfilesystemmembers toPath. It addresses two problems:Filesystem
Filesystem was added as a field of the
os.Path. It was not added as a Root field for simplicity and consistency with Java'snio. Root is a part of the path; filesystem is the context of the whole path. One filesystem can have many roots; one root can have many subdirectories.Root as String
A string is the simplest type that can represent the path's root. It also corresponds to the mental model of the path - usually, developers perceive it as a String. Therefore - String was chosen as the type of root.
Pull request: #196