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

Error converting relative paths to absolute paths in Windows if relative path has leading slash or backslash #2359

Closed
philwalk opened this issue Aug 25, 2023 · 10 comments · Fixed by #2516
Labels
bug Something isn't working

Comments

@philwalk
Copy link
Contributor

philwalk commented Aug 25, 2023

Version(s)

$ scala-cli --version
Scala CLI version: 1.0.4
Scala version (default): 3.3.0

Describe the bug
The following idiom is intended to convert a relative path to an absolute path:

//> using lib "com.lihaoyi::os-lib::0.8.1"
val abspath = Path(relativeDir, pwd)

This works as expected unless relativeDir arg has a leading slash or backslash. Although such a path is absolute everywhere else, in Windows it's technically a relative path, but one that can only be made absolute by prefixing a drive letter. If not specified, the current working drive (typically C:) is assumed. It's a type of relative path that cannot be resolved by prefixing pwd. It resembles an absolute path on other platforms, and in practice, it's often seen that way, particularly on a system with a single drive, and especially by developers mostly familiar with other platforms.

The proposed os-lib term for this type of Windows path is drive-relative. See com-lihaoyi/os-lib#202

For relative paths that are drive-relative, the code above accidentally works as expected if pwd=='C:\' or similar.
For most values of pwd, it produces an incorrect result, and differs from the behavior of java.nio.file.Paths.get().

Windows paths are described here: windows-file-path-formats. A reference to what I'm calling drive-relative is here.
To Reproduce
The following script shows the problem and a solution for interpreting root-relative-paths in Windows.

#!/usr/bin/env -S scala-cli shebang
//> using scala "3.2.0"
//> using lib "com.lihaoyi::os-lib::0.8.1"
//> using lib "com.lihaoyi::pprint::0.8.0"

import pprint._
import os._

val driveRelPathScalaCliBug = Path("/opt")
printf("the current scala-cli value: [%s]\n", driveRelPathScalaCliBug)

var defaultDrive = pwd.toString.take(2).mkString
printf("pwd: %s\n", pwd)
printf("os.root: %s\n", os.root)
printf("defaultDrive: %s\n", defaultDrive)
val driveRelPathScalaCliFix = Path(s"$defaultDrive/opt")
printf("driveRelPathScalaCliFix: [%s]\n", driveRelPathScalaCliFix)
val driveRelPathJava = java.nio.file.Paths.get("/opt")
printf("driveRelPathJava:        [%s]\n", driveRelPathJava.toAbsolutePath)

The output of the script when pwd == 'F:\data':

$ C:/opt/ue/jsrc/scalaCliBug.sc
the current scala-cli value: [F:\data\opt]
pwd: F:\data
os.root: F:\
defaultDrive: F:
driveRelPathScalaCliFix: [F:\opt]
driveRelPathJava:        [F:\opt]

This result shows an incorrect value for os.root, if it's supposed to match environment variable SystemDrive.

Here's the output when pwd == 'C:\Users\philwalk\workspace\scala-cli':

$ C:/opt/ue/jsrc/scalaCliBug.sc
the current scala-cli value: [C:\Users\philwalk\workspace\scala-cli\opt]
pwd: C:\Users\philwalk\workspace\scala-cli
os.root: C:\
defaultDrive: C:
driveRelPathScalaCliFix: [C:\opt]
driveRelPathJava:        [C:\opt]

In this case, os.root may be accidentally correct.

Another way this bug manifests, when using the following scala-cli hashbang:

#!/usr/bin/env -S scala-cli shebang

If the path referenced by JAVA_HOME does not include a drive letter, the following crash occurs:

JAVA_HOME=/opt/jdk
/opt/ue/jsrc/scalaCliBug.sc
java.io.IOException: Cannot run program "C:\Users\philwalk\workspace\os-lib-issue-201\opt\jdk\bin\java.exe" (in directory "C:\Users\philwalk\workspace\os-lib-issue-201"): CreateProcess error=2, The system cannot find the file specified

The workaround is to prefix various things on the command ine with the current working drive ("C:" in this example).
So instead of this:

/opt/ue/jsrc/scalaCliBug.sc

the command line must be issued like this (extra stuff in bold):

JAVA_HOME=C:$JAVA_HOME C:/opt/ue/jsrc/scalaCliBug.sc

After the os-lib fix is available, the same command line can be used in Windows as on other platforms.

Expected behaviour
If a path has a leading slash or backslash, convert it to an absolute path by prefixing the default drive letter.
This is also how root-relative-paths are interpreted in CMD.EXE and everywhere.

@philwalk philwalk added the bug Something isn't working label Aug 25, 2023
@philwalk
Copy link
Contributor Author

I found os.Path here: https://github.com/com-lihaoyi/os-lib/blob/main/os/src/Path.scala
and I suspect this bug should be filed there. I will investigate ...

@philwalk
Copy link
Contributor Author

philwalk commented Aug 27, 2023

BTW, this bug also manifests in the following situation:

A scala-cli script located at C:/opt/ue/jsrc/scalaCliBug.sc should be callable without the drive letter prefix, if the path exists on the current default drive. Here's what happens currently, in that case:

philwalk@d5 ~/workspace/os-lib-phil
$ /opt/ue/jsrc/scalaCliBug.sc
[error]  File not found: C:\Users\philwalk\workspace\os-lib-phil\opt\ue\jsrc\scalaCliBug.sc

The same invocation with an explicit drive letter works.

I have a PR that I'm about to submit to os-lib with a fix.

@tgodzik
Copy link
Member

tgodzik commented Aug 28, 2023

Thanks for reporting! I see that you already created an issue in os-lib. There is nothing currently we can do in Scala CLI, closing in favour of com-lihaoyi/os-lib#201

@tgodzik tgodzik closed this as completed Aug 28, 2023
@philwalk
Copy link
Contributor Author

I'd like to verify that this is fixed by com-lihaoyi/os-lib#202.
Is there an easy way to test scala-cli with an alternate version of os-lib?

@tgodzik
Copy link
Member

tgodzik commented Aug 30, 2023

Och wait, I think I lost the fact that the issue is connected to scala-cli usage of os-lib. The issue you reported seemed only relevant to os-lib itself.

How did this show up in scala-cli? When using #!/usr/bin/env -S scala-cli shebang with JAVA_HOME without a drive or were there some other cases?

@philwalk
Copy link
Contributor Author

How did this show up in scala-cli? When using #!/usr/bin/env -S scala-cli shebang with JAVA_HOME without a drive or were there some other cases?

This bug causes any path with a leading slash to crash, whether in a script or on the command-line.

Generally speaking, os-lib and scala-cli are difficult to use in Windows, at least for developers working in cygwin, bash-git, msys2, etc. I have hundreds of scripts that are portable between OSX, Linux and Windows that I would like to convert to scala-cli, but am unable to because of this bug, since portable scripts necessarily use only posix style paths, such as /opt/ue/jsrc/myscript.sc. Being forced to add drive letters breaks portability.

The os-lib PR (currently under review) fixes this problem, AFAICT. If I can figure out how to test scala-cli with an alternate copy of the os-lib jar, I will verify it.

@tgodzik tgodzik reopened this Aug 31, 2023
@tgodzik
Copy link
Member

tgodzik commented Aug 31, 2023

Ok sure! We can wait for the os-lib to be released with the fix or try to figure out a workaround ourselves if that is not accepted. Sorry for misunderstanding the issue at hand.

You could try releasing os-lib locally, changing it in https://github.com/VirtusLab/scala-cli/blob/main/project/deps.sc#L141 and running ./mill -i scala …arguments…. More on that in https://github.com/VirtusLab/scala-cli/blob/main/DEV.md

@philwalk
Copy link
Contributor Author

philwalk commented Nov 7, 2023

Now that os-lib has released 0.9.2, it should be possible to fix this bug.

I'd like to verify that it fixes the problem, although I'm not sure how to get the current version of scala-cli to use a different version of os-lib, any suggestions?

It does seem to fix this issue, although I've only done a very superficial amount of manual testing.
I will see what I can find out.

@philwalk
Copy link
Contributor Author

philwalk commented Nov 7, 2023

Should I be able to build in Windows 10? Or is it possible to cross-build for Windows on another platform?

I was able to build in Windows 10 by running the following command line:

.github/scripts/generate-native-image.sh

I'll play with it awhile and report back.

@philwalk
Copy link
Contributor Author

philwalk commented Nov 8, 2023

I created a PR #2516 that fixes this issue, and one other

@Gedochao Gedochao linked a pull request Nov 8, 2023 that will close this issue
Gedochao added a commit that referenced this issue Nov 24, 2023
* fix for #2359, drive-relative paths accepted in Windows SHELL environments

* added anti-regression test handling of drive-relative for os.Path

* fix classpath-split regex String to accept either forward slash or backslash

* added integration tests for JAVA_HOME setting to RunScriptTestDefinitions.scala

* apply suggested changes

* fix for cli.base-image.nativeImage in Windows

* fix JAVA_HOME test; tweak mill script

* Apply suggestions from code review

---------

Co-authored-by: Piotr Chabelski <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants