fix for issue-201 #202

philwalk · 2023-08-27T15:35:12Z

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:

    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:

isAbsolute: true
/omg
/omg

On Windows:

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.

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:

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.

Relevant information: My system drives are C: and F:, but not J:

First, some unsurprising results:

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> 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

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().

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.

philwalk · 2023-08-27T19:03:24Z

Subsequent commit fixes compile errors for scala 2 versions.

philwalk · 2023-08-27T22:51:43Z

Just noticed that scalafmt complained, will push corrected format.

lefou

TBH, I have not completely made up my mind about this issue and your approach to handle it. Nevertheless, thank you for this PR. I already have some review comments to share.

The issue involves file system roots, a topic we have a dedicated issue for (#170) and also a PR (#196). We should aim for a consistent solution for both issues.

os/src/Path.scala

os/test/src/PathTests.scala

philwalk · 2023-08-28T16:32:04Z

TBH, I have not completely made up my mind about this issue and your approach to handle it. Nevertheless, thank you for this PR. I already have some review comments to share.

The issue involves file system roots, a topic we have a dedicated issue for (#170) and also a PR (#196). We should aim for a consistent solution for both issues.

Sounds good. I will try to see how this PR can be made compatible with (#196). I assume we want to be compatible with Java's Paths.get() behavior if at all possible.

verify compatibile with (#196); combine conditionals, remove unused code

philwalk · 2023-08-28T20:01:24Z

I edited in the changes in (#196) and verified no conflicts, so the two PRs should merge without any issues. They appear to be mostly orthogonal concerns. Afterwards, I removed the other PR code and did more testing.
I applied your suggestions, made some simplifications, and squashed my commits into one.

lefou

Sorry for the longish review. I'm still making up my mind about this issue. It's really hard to reason about the specifics of an OS I don't use.

.gitignore

os/test/src/PathTests.scala

os/src/Path.scala

lefou · 2023-08-29T07:26:57Z

Oh, and please avoid squashing while we are in the review process. It hinders to follow the incremental process. We will squash when we merge.

philwalk · 2023-08-29T20:58:01Z

Oh, and please avoid squashing while we are in the review process. It hinders to follow the incremental process. We will squash when we merge.

Makes sense.

philwalk · 2023-08-29T21:29:26Z

After some thought, a better name than windowsWorkingDrive or currentWorkingDrive is platformPrefix.
The idea is that, a platform might require a prefix to resolve a rootRelative path. In Windows, the necessary prefix is the current working drive. If you prefer a 'working-drive' reference, let me know and I'll update it.

philwalk · 2023-09-02T21:46:24Z

I have been verifying that this PR fixes the scala-cli problem, and I discovered an issue. I'm now verifying an addition change, and hope to be able to commit soon.

…ive tooManyUps exception

philwalk · 2023-09-03T22:21:17Z

It turns out that the tests in os/test/src/PathTests.scala that weren't enabled when building in Windows revealed that a rootRelative path wasn't being properly checked for PathError.AbsolutePathOutsideRoot.

I enabled all PathsTests.scala tests, fixing a few of them so they pass or fail, as required.
The previously disabled tests are enabled with flag enableTests in place of the Unix() reference, to simplify seeing the changes.

I just noticed an incorrect comment, I'll fix it.

philwalk · 2023-09-04T16:06:05Z

I manually verified that this fixes the scala-cli bug VirtusLab/scala-cli#2359

lihaoyi · 2023-09-09T16:45:21Z

os/test/src/PathTests.scala

+  implicit class ExtendString(s: String) {
+    def posix: String = s.replace('\\', '/')
+    def norm: String = if (s.startsWith(platformPrefix)) {
+      s.posix // already has platformPrefix
+    } else {
+      s"$platformPrefix${s.posix}"
+    }
+  }
+  implicit class ExtendOsPath(p: os.Path) {
+    def posix: String = p.toNIO.toString.posix
+    def norm: String = p.toString.norm
+  }
+  implicit class ExtendPath(p: java.nio.file.Path) {
+    def posix: String = p.toString.posix
+    def norm: String = p.toString.norm


Let's use normal helper methods here, rather than extension methods, unless we have good reason to do otherwise.

done (will push changes soon)

lihaoyi · 2023-09-09T16:45:47Z

os/test/src/PathTests.scala

 import utest.{assert => _, _}
 import java.net.URI
 object PathTests extends TestSuite {
+  def enableTest = true


If enableTest is true, we should be able to remove it and remove all the conditionals guarding the test cases

I'll remove them.

lihaoyi · 2023-09-09T16:47:53Z

os/src/Path.scala

+    val normalized = {
+      val f = if (rootRelative(f0)) {
+        Paths.get(s"$platformPrefix$f0")
+      } else {
+        implicitly[PathConvertible[T]].apply(f0)
+      }
+      if (f.iterator.asScala.count(_.startsWith("..")) > f.getNameCount / 2) {
+        throw PathError.AbsolutePathOutsideRoot
+      }


We can re-arrange this to move the throw outside of the val normalized? All these are method-local vals anyway, so there's no visibility/privacy concerns, and it would be nice to remove a layer of nesting

lihaoyi · 2023-09-09T16:51:40Z

Ieft a few comments. I am also not super familiar with windows, but if the old tests continue passing and the new tests pass then that gives us some confidence.

@philwalk could you update the PR description to:

Remove all the stuff about Scala-CLI. Linking to the issue in Scala-CLI should be enough for people to follow, but for a PR to OS-Lib the description of the problem should focus on exactly what's the problem with OS-Lib and not how it manifests in some downstream project
Explain what your solution is, in english. We already have your code in the diff, but it would be much easier to review for us and for future maintainers if the PR description had an english summary of your changes explaining what changes you needed to make, where, and why.
How does this PR relate to Add root to Path & root constructor #196? Do we want both? Does one supersede the other? Are they incompatible?

After that, I think I'd be OK with merging this, unless @lefou has any other concerns

lihaoyi · 2023-09-09T16:52:35Z

os/src/Path.scala

+   */
+  def rootRelative[T: PathConvertible](f0: T): Boolean = rootRelative(f0.toString)
+
+  def rootRelative(s: String): Boolean = {


Do we need this overload, given that the implicit object StringConvertible extends PathConvertible[String] already exists? If not we should remove it

Combined the two:

def rootRelative[T: PathConvertible](f0: T): Boolean = { if (platformPrefix.isEmpty) { false // non-Windows os } else { f0.toString.take(1) match { case "\\" | "/" => true case _ => false } } }

philwalk · 2023-09-09T22:19:51Z

After that, I think I'd be OK with merging this, unless @lefou has any other concerns
All your suggestions are implemented, thanks for the feedback!

BTW, some tests that pass on github but always fail on my Windows system:

X test.os.FilesystemMetadataTests.isFile.0 17ms
X test.os.FilesystemMetadataTests.isDir.0 18ms
X test.os.FilesystemMetadataTests.isLink.0 18ms
X test.os.FilesystemMetadataTests.mtime.0 18ms
X test.os.ListingWalkingTests.list.0 15ms
X test.os.ListingWalkingTests.walk.0 19ms
X test.os.ManipulatingFilesFoldersTests.exists.0 27ms
X test.os.ManipulatingFilesFoldersTests.followLink.0 17ms

If I'm not mistaken, these rely on finding the following in the PATH:

TestUtil.isInstalled("curl")
TestUtil.isInstalled("gzip")
TestUtil.isInstalled("shasum")
TestUtil.isInstalled("echo")
TestUtil.isInstalled("python")

All of these are in my PATH, although I haven't tracked down the exact cause of the failures.
It would be useful to know how the Windows build system is configured.

lefou · 2023-09-10T14:47:43Z

os/src/Path.scala

+   * @return current working drive if Windows, empty string elsewhere.
+   * Paths.get(platformPrefix) == current working directory on all platforms.
+   */
+  lazy val platformPrefix: String = Paths.get(".").toAbsolutePath.getRoot.toString match {


Overall I'm fine with this PR, but I'm still struggling with the term platformPrefix. Hhow about rootPrefix or maybe also something that has the "drive" in it?

I like driveRoot instead of platformPrefix
and driveRelative instead of rootRelative.

philwalk · 2023-09-10T15:13:54Z

I merged #196 with this PR as an experiment, and all tests pass.
~~there may be a simplification to this PR that is now possible.~~

lefou reviewed Aug 28, 2023

View reviewed changes

os/src/Path.scala Outdated Show resolved Hide resolved

os/src/Path.scala Outdated Show resolved Hide resolved

os/test/src/PathTests.scala Outdated Show resolved Hide resolved

fix for issue-201

abc911e

verify compatibile with (#196); combine conditionals, remove unused code

lefou reviewed Aug 29, 2023

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

os/test/src/PathTests.scala Outdated Show resolved Hide resolved

os/src/Path.scala Outdated Show resolved Hide resolved

os/src/Path.scala Outdated Show resolved Hide resolved

os/src/Path.scala Outdated Show resolved Hide resolved

updated PR based on suggestions and discussion

d20e417

philwalk mentioned this pull request Aug 30, 2023

Error converting relative paths to absolute paths in Windows if relative path has leading slash or backslash VirtusLab/scala-cli#2359

Closed

fix scalafmt errors

6c04015

all os/test/src/PathTests.sc enabled, new test added, allow rootRelat…

0707d4c

…ive tooManyUps exception

removed incorrect comment

1c37739

lihaoyi reviewed Sep 9, 2023

View reviewed changes

changes suggested by @lihaoyi

5bee44c

philwalk added 2 commits September 9, 2023 18:54

fix scalafmt errors, supplement PathTests.scala near line 25 test

1570df4

fix test "Paths.get(platformPrefix) sameFile as pwd"

fc78871

lefou reviewed Sep 10, 2023

View reviewed changes

renamed platformPrefix and rootRelative, fixed java8 failing test

430951e

philwalk added 2 commits September 10, 2023 16:55

fix java8 trailing slash test error

93ef3aa

scalafmt fix

c1b0cab

lihaoyi merged commit f79f62f into com-lihaoyi:main Sep 12, 2023

philwalk deleted the handle-semiabsolute-paths branch September 12, 2023 20:10

lefou added this to the after 0.9.1 milestone Oct 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix for issue-201 #202

fix for issue-201 #202

philwalk commented Aug 27, 2023 •

edited

Loading

philwalk commented Aug 27, 2023 •

edited

Loading

philwalk commented Aug 27, 2023 •

edited

Loading

lefou left a comment

philwalk commented Aug 28, 2023

philwalk commented Aug 28, 2023 •

edited

Loading

lefou left a comment

lefou commented Aug 29, 2023

philwalk commented Aug 29, 2023

philwalk commented Aug 29, 2023 •

edited

Loading

philwalk commented Sep 2, 2023

philwalk commented Sep 3, 2023 •

edited

Loading

philwalk commented Sep 4, 2023

lihaoyi Sep 9, 2023

philwalk Sep 9, 2023

lihaoyi Sep 9, 2023

philwalk Sep 9, 2023

lihaoyi Sep 9, 2023

philwalk Sep 9, 2023

lihaoyi commented Sep 9, 2023 •

edited

Loading

lihaoyi Sep 9, 2023

philwalk Sep 9, 2023

philwalk commented Sep 9, 2023 •

edited

Loading

lefou Sep 10, 2023

philwalk Sep 10, 2023 •

edited

Loading

philwalk commented Sep 10, 2023 •

edited

Loading

fix for issue-201 #202

fix for issue-201 #202

Conversation

philwalk commented Aug 27, 2023 • edited Loading

Background

Making os/test/src/PathTests platform independent

How this PR relates to #170 and #196

Additional Details regarding unique aspects of Windows Filesystem Paths

philwalk commented Aug 27, 2023 • edited Loading

philwalk commented Aug 27, 2023 • edited Loading

lefou left a comment

Choose a reason for hiding this comment

philwalk commented Aug 28, 2023

philwalk commented Aug 28, 2023 • edited Loading

lefou left a comment

Choose a reason for hiding this comment

lefou commented Aug 29, 2023

philwalk commented Aug 29, 2023

philwalk commented Aug 29, 2023 • edited Loading

philwalk commented Sep 2, 2023

philwalk commented Sep 3, 2023 • edited Loading

philwalk commented Sep 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lihaoyi commented Sep 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philwalk commented Sep 9, 2023 • edited Loading

Choose a reason for hiding this comment

philwalk Sep 10, 2023 • edited Loading

Choose a reason for hiding this comment

philwalk commented Sep 10, 2023 • edited Loading

philwalk commented Aug 27, 2023 •

edited

Loading

Making `os/test/src/PathTests` platform independent

philwalk commented Aug 27, 2023 •

edited

Loading

philwalk commented Aug 27, 2023 •

edited

Loading

philwalk commented Aug 28, 2023 •

edited

Loading

philwalk commented Aug 29, 2023 •

edited

Loading

philwalk commented Sep 3, 2023 •

edited

Loading

lihaoyi commented Sep 9, 2023 •

edited

Loading

philwalk commented Sep 9, 2023 •

edited

Loading

philwalk Sep 10, 2023 •

edited

Loading

philwalk commented Sep 10, 2023 •

edited

Loading