Add support for Windows #492

bioball · 2024-05-13T23:56:12Z

Closes #20

This adds support for Windows.
The in-language path separator is still /, to ensure Pkl programs are cross-platform.
The specifics behind this design are detailed in apple/pkl-evolution#7.

Log lines are written using CRLF endings on Windows.
Modules that are combined with --module-output-separator uses LF endings to ensure
consistent rendering across platforms.

jpkl does not work on Windows as a direct executable.
However, it can work with java -jar jpkl.

Additional details:

Adjust git settings for Windows
Add native executable for pkl cli
Add jdk17 windows Gradle check in CI
Adjust CI test reports to be staged within Gradle rather than by shell script.
Fix: encode more characters that are not safe Windows paths
Skip running tests involving symbolic links on Windows (these require administrator privileges to run).
Introduce custom implementation of IoUtils.relativize, because the current implementation delegates to Path, which will fail if the URI contains invalid Windows characters.
Allow Gradle to initialize ExecutableJar Property values
Add a way to enable remote debugging with ./gradlew test commands
Standard library pkl:platform's current.operatingSystem.name is "Windows" for all Windows OSes (match treatment of macOS/Linux).

bioball · 2024-05-20T14:39:11Z

.circleci/config.pkl


 import "jobs/BuildNativeJob.pkl"
 import "jobs/GradleCheckJob.pkl"
 import "jobs/DeployJob.pkl"
 import "jobs/SimpleGradleJob.pkl"

-local prbJobs: Listing<String> = gradleCheckJobs.keys.toListing()
+local prbJobs: Listing<String> = (gradleCheckJobs.keys.toListing()) {
+  "pkl-cli-windows-amd64-snapshot"


I will remove this from the CI definition before merging so we don't run build native jobs on PRBs.

bioball · 2024-05-20T14:40:13Z

.circleci/jobs/GradleJob.pkl

-        find . -type f -regex ".*/build/test-results/.*xml" -exec cp {} ~/test-results/ \\;
-        """
-      `when` = "always"
-    }


This step breaks on Windows.

Instead of trying to figure out how to write a Windows version, I added some logic to specify the test reports dir (see line 29).

bioball · 2024-05-20T14:41:18Z

.circleci/jobs/GradleJob.pkl


 /// Whether this is a release build or not.
 isRelease: Boolean = false

+/// The OS to run on
+os: "macOS"|"linux"|"windows"


Moved this from BuildNativeJob, because this adds a ./gradlew check job for Windows to PRBs.

bioball · 2024-05-20T14:43:25Z

buildSrc/src/main/kotlin/BuildInfo.kt

+        .start()
+      process.waitFor().also { exitCode ->
+        if (exitCode == -1) throw RuntimeException(process.errorStream.reader().readText())
+      }


Tangential improvement: changed this because the failure was getting swallowed and I wasn't sure why the commit ID was not being populated correctly on Windows.

bioball · 2024-05-20T14:43:58Z

buildSrc/src/main/kotlin/ExecutableJar.kt


  @get:Input
-  val jvmArgs: ListProperty<String> = project.objects.listProperty()
+  abstract val jvmArgs: ListProperty<String>


This change is unrelated to Windows. Happy to move this to a separate PR if you'd prefer.

bioball · 2024-05-20T14:47:06Z

pkl-cli/src/main/kotlin/org/pkl/cli/CliEvaluator.kt

@@ -158,7 +160,7 @@ constructor(
          } else {
            if (output.isNotEmpty()) {
              outputFile.writeString(
-                options.moduleOutputSeparator + IoUtils.getLineSeparator(),
+                options.moduleOutputSeparator + '\n',


Changed this to be LF and not platform dependent. Otherwise, pkl eval foo.pkl bar.pkl will give you different results on different OSes, which seems quite undesirable.

will give you different results on different OSes

True, but alternatively phrased "will give you results appropriate for the OS it runs on." I can't think of any, but are we sure nothing breaks on Windows when using LF instead of CRLF? Should this, alternatively, be a pkl:settings setting?

This is configuration output meant to be saved to a file, or applied to a system.

The standard use-case here is to generate a YAML document stream. For example, pkl eval podA.pkl podB.pkl > output.yaml.

Note that if the -o option is set, it already uses LF endings. So, without this change, these two would have had different results:

pkl eval podA.pkl podB.pkl > output.yaml

pkl eval podA.pkl podB.pkl -o output.yaml

bioball · 2024-05-20T14:48:50Z

pkl-cli/src/test/kotlin/org/pkl/cli/CliProjectPackagerTest.kt

+      .out${sep}project2@2.0.0${sep}project2@2.0.0.zip
+      .out${sep}project2@2.0.0${sep}project2@2.0.0.zip.sha256
+      .out${sep}project2@2.0.0${sep}project2@2.0.0
+      .out${sep}project2@2.0.0${sep}project2@2.0.0.sha256


Log lines on Windows will output CRLF endings, and also output \ as the path separator.

The rationale is that these are log lines, which are meant to be useful for the host machine specifically (e.g. pkl project package | xargs ...).

CRLF ^^ re: consistency for pkl eval; I'd prefer consistency across commands.

Can you elaborate on what is not consistent here?

The principle is:

Log lines are written to be appropriate for the target system

Configuration output is written to be consistent across operating systems

bioball · 2024-05-20T14:51:28Z

pkl-core/src/main/java/org/pkl/core/module/ModuleKeyFactories.java

@@ -141,15 +141,20 @@ public Optional<ModuleKey> create(URI uri) {
  private static class File implements ModuleKeyFactory {
    @Override
    public Optional<ModuleKey> create(URI uri) {
-      Path path;
-      try {
-        path = Path.of(uri);


Needed to adjust this because Path.of(URI.create("file:///foo:bar")) throws an InvalidPathException, but yet should be treated as a file module key.

Changed this to initialize Path within the module key logic rather than here.

Does it?

| Welcome to JShell -- Version 17.0.6 | For an introduction type: /help intro jshell> Path.of(URI.create("file:///C:/foo/bar")) $1 ==> /C:/foo/bar jshell> Path.of(URI.create("file:///C:foo")) $2 ==> /C:foo jshell> Path.of(URI.create("file:///foo:bar")) $3 ==> /foo:bar jshell>

^^ Ran that not on Windows. I'm just astonished if it throws on the one platform where it's relevant ;)

Yup; this is what we get on Windows:

jshell> Path.of(URI.create("file:///foo:bar")) | Exception java.nio.file.InvalidPathException: Illegal char <:> at index 4: /foo:bar | at WindowsPathParser.normalize (WindowsPathParser.java:182) | at WindowsPathParser.parse (WindowsPathParser.java:153) | at WindowsPathParser.parse (WindowsPathParser.java:77) | at WindowsPath.parse (WindowsPath.java:92) | at WindowsUriSupport.fromUri (WindowsUriSupport.java:166) | at WindowsFileSystemProvider.getPath (WindowsFileSystemProvider.java:98) | at Path.of (Path.java:203) | at (#6:1)

This adds support for Windows. The in-language path separator is still `/`, to ensure Pkl programs are cross-platform. Log lines are written using CRLF endings on Windows. Modules that are combined with `--module-output-separator` uses LF endings to ensure consistent rendering across platforms. `jpkl` does not work on Windows as a direct executable. However, it can work with `java -jar jpkl`. Additional details: * Adjust git settings for Windows * Add native executable for pkl cli * Add jdk17 windows Gradle check in CI * Adjust CI test reports to be staged within Gradle rather than by shell script. * Fix: encode more characters that are not safe Windows paths * Skip running tests involving symbolic links on Windows (these require administrator privileges to run). * Introduce custom implementation of `IoUtils.relativize` * Allow Gradle to initialize ExecutableJar `Property` values

bioball · 2024-05-21T05:28:45Z

.gitattributes

@@ -4,3 +4,4 @@
 /docs/** linguist-documentation

 *.pkl linguist-language=Groovy
+* text eol=lf


Needed for git, and for spotless apply.

Might be worth an .editorconfig?

stackoverflow

Looks good.

stackoverflow · 2024-05-21T11:28:40Z

docs/modules/pkl-cli/pages/index.adoc

-chmod +x jpkl
-./jpkl --version
+Invoke-WebRequest '{uri-pkl-windows-download}' -OutFile pkl.exe
+chmod +x pkl.exe


Does chmod exist on cmd or PowerShell, or is this taking in consideration you are running bash?

Oh, good catch.

stackoverflow · 2024-05-21T12:45:06Z

pkl-core/src/main/java/org/pkl/core/util/IoUtils.java

@@ -180,6 +192,10 @@ public static String getLineSeparator() {
    return System.getProperty("line.separator");
  }

+  public static Boolean isWindows() {
+    return System.getProperty("os.name").contains("Windows");


nit: with the amount of times this function may be called perhaps would be a good idea to cache it.

I'll switch this to use Platform.current().operatingSystem(), which avoids the contains call every time.

stackoverflow · 2024-05-21T12:54:35Z

pkl-core/src/main/java/org/pkl/core/project/ProjectPackager.java

@@ -363,7 +370,7 @@ private List<Path> collectPackageElements(Project project, Package pkg) {
  }

  private boolean isAbsoluteImport(String importStr) {
-    return importStr.matches("\\w:.*") || importStr.startsWith("@");
+    return importStr.matches("\\w+:.*") || importStr.startsWith("@");


Is this a bugfix? isAbsoluteImport("pkl:math") returned false before this change, now returns true.

holzensp

Some comments, but no blockers.

holzensp · 2024-05-22T12:56:50Z

.circleci/config.pkl

@@ -134,6 +136,11 @@ local buildNativeJobs: Mapping<String, BuildNativeJob> = new {
      musl = true
      isRelease = _dist == "release"
    }
+    ["pkl-cli-windows-amd64-\(_dist)"] {


Are we considering Windows-on-ARM just-not-there-yet? With the upcoming SnapDragon's, I expect this to be a thing real quick.

GraalVM doesn't have Windows ARM support. I'm actually not sure if they plan on supporting it; my question about it in Slack has not received any responses: https://graalvm.slack.com/archives/CNBFR78F9/p1715635155814669

holzensp · 2024-05-22T13:23:44Z

.gitattributes

@@ -4,3 +4,4 @@
 /docs/** linguist-documentation

 *.pkl linguist-language=Groovy
+* text eol=lf


Might be worth an .editorconfig?

holzensp · 2024-05-22T13:39:30Z

docs/modules/language-reference/pages/index.adoc

+====
+Relative paths use the `/` character as the directory separator on all platforms, including Windows.
+
+Paths that contain drive letters (e.g. `C:`) must be declared as an absolute file URI, for example: `import "file:///C:/path/to/my/module.pkl"`. Otherwise, they would be interpreted as a URI scheme.


Suggested change

Paths that contain drive letters (e.g. `C:`) must be declared as an absolute file URI, for example: `import "file:///C:/path/to/my/module.pkl"`. Otherwise, they would be interpreted as a URI scheme.

Paths that contain drive letters (e.g. `C:`) must be declared as an absolute file URI, for example: `import "file:///C:/path/to/my/module.pkl"`. Otherwise, they are interpreted as a URI scheme.

holzensp · 2024-05-22T13:56:10Z

docs/modules/pkl-cli/pages/index.adoc

-./jpkl --version
+Invoke-WebRequest '{uri-pkl-windows-download}' -OutFile pkl.exe
+chmod +x pkl.exe
+.\pkl.exe --version


Nit; .\ & .exe read as unidiomatic to me. I thought . was part of `%PATH% by default?

Suggested change

.\pkl.exe --version

pkl --version

In my shell, .\ is needed. Looks like .exe can be omitted, though.

holzensp · 2024-05-22T14:05:04Z

pkl-cli/src/main/kotlin/org/pkl/cli/CliEvaluator.kt

@@ -158,7 +160,7 @@ constructor(
          } else {
            if (output.isNotEmpty()) {
              outputFile.writeString(
-                options.moduleOutputSeparator + IoUtils.getLineSeparator(),
+                options.moduleOutputSeparator + '\n',


will give you different results on different OSes

True, but alternatively phrased "will give you results appropriate for the OS it runs on." I can't think of any, but are we sure nothing breaks on Windows when using LF instead of CRLF? Should this, alternatively, be a pkl:settings setting?

holzensp · 2024-05-22T15:57:33Z

pkl-core/src/main/java/org/pkl/core/module/ModuleKeyFactories.java

@@ -141,15 +141,20 @@ public Optional<ModuleKey> create(URI uri) {
  private static class File implements ModuleKeyFactory {
    @Override
    public Optional<ModuleKey> create(URI uri) {
-      Path path;
-      try {
-        path = Path.of(uri);


Does it?

| Welcome to JShell -- Version 17.0.6 | For an introduction type: /help intro jshell> Path.of(URI.create("file:///C:/foo/bar")) $1 ==> /C:/foo/bar jshell> Path.of(URI.create("file:///C:foo")) $2 ==> /C:foo jshell> Path.of(URI.create("file:///foo:bar")) $3 ==> /foo:bar jshell>

^^ Ran that not on Windows. I'm just astonished if it throws on the one platform where it's relevant ;)

holzensp · 2024-05-22T16:02:01Z

pkl-core/src/main/java/org/pkl/core/module/ModuleKeys.java

+      // (require `/` as the path separator on all OSes)
+      var uriPath = uri.getPath();
+      if (java.io.File.separatorChar == '\\' && uriPath != null && uriPath.contains("\\")) {
+        throw new FileNotFoundException();


The error experience could be better if someone, on Windows, assumes that #"foo\bar\baz.pkl"# is a relative file path. I wouldn't blame them for it. This is sufficient, but wouldn't it be better to add an extra check to see whether it's on Windows and then to provide an error message stating that paths still require / separators, even on Windows?

holzensp · 2024-05-22T16:04:40Z

pkl-core/src/main/java/org/pkl/core/module/ResolvedModuleKeys.java

+        // Windows throws `AccessDeniedException` when reading directories.
+        // Sync error between different OSes.
+        if (Files.isDirectory(path)) {
+          throw new IOException("Is a directory");


Should we not preserve the trace? I'd, at least, add e as a cause.

Suggested change

throw new IOException("Is a directory");

throw new IOException("Is a directory", e);

holzensp · 2024-05-22T16:13:44Z

pkl-doc/src/test/kotlin/org/pkl/doc/CliDocGeneratorTest.kt

@@ -219,6 +226,11 @@ class CliDocGeneratorTest {
      .withFailMessage("Test bug: $actualFile should exist but does not.")
      .exists()

+    // symlinks on Git and Windows is rather finnicky; they create shortcuts by default unless
+    // a core Git option is set. Also, by default, symlinks require administrator privileges to run.


... but a .lnk is a text file with the redirected path in, no? Do you also need admin to read? If not; Is it an option to read actualFile instead and replace it with the contents? (Would possibly require a while, because of link-to-link cases.)

As far as I can tell, there's no easy way to parse a lnk file in Java and figure out where it's pointed to. It requires a 3rd party library to parse them.

holzensp · 2024-05-22T16:19:39Z

pkl-executor/src/main/java/org/pkl/executor/EmbeddedExecutor.java

+    return rootDir == null ? modulePath : relativize(modulePath, rootDir);
+  }
+
+  // Windows relativize will fail if the two paths have different roots.
+  private static Path relativize(Path path, Path base) {
+    if (path.isAbsolute() && base.isAbsolute() && !path.getRoot().equals(base.getRoot())) {


I'd be inclined to push it in

Suggested change

return rootDir == null ? modulePath : relativize(modulePath, rootDir);

}

// Windows relativize will fail if the two paths have different roots.

private static Path relativize(Path path, Path base) {

if (path.isAbsolute() && base.isAbsolute() && !path.getRoot().equals(base.getRoot())) {

return relativize(modulePath, rootDir);

}

// Windows relativize will fail if the two paths have different roots.

private static Path relativize(Path path, Path base) {

if (base == null || path.isAbsolute() && base.isAbsolute() && !path.getRoot().equals(base.getRoot())) {

bioball force-pushed the windows-build branch 2 times, most recently from 42c2cdb to 57da202 Compare May 14, 2024 00:03

bioball marked this pull request as draft May 14, 2024 17:16

bioball force-pushed the windows-build branch 5 times, most recently from cdd599f to 64086cd Compare May 17, 2024 22:30

bioball mentioned this pull request May 20, 2024

SPICE-0006: Import Paths on Windows apple/pkl-evolution#7

Open

bioball marked this pull request as ready for review May 20, 2024 03:17

bioball commented May 20, 2024

View reviewed changes

bioball force-pushed the windows-build branch 3 times, most recently from 065dc9b to a80a62c Compare May 20, 2024 19:21

bioball force-pushed the windows-build branch from a80a62c to ed79a4e Compare May 20, 2024 23:17

bioball added 2 commits May 20, 2024 16:27

Run spotlessApply

8aa894a

Fix parsing module names of windows absolute paths

349b069

bioball force-pushed the windows-build branch from 7e2e363 to 349b069 Compare May 21, 2024 04:19

bioball added 2 commits May 20, 2024 21:46

Fix encoding and triple-slash paths

75b6662

Fix remaining build errors

669d4f2

bioball commented May 21, 2024

View reviewed changes

Adjust note about jvm debugging

2073d06

Docs adjustments

b8a0b87

stackoverflow approved these changes May 21, 2024

View reviewed changes

bioball added 2 commits May 21, 2024 08:25

Adjustments from code review, set platform to Windows

bdddedc

Set union type on pkl.platform#OperatingSystem.name

908d4e5

holzensp approved these changes May 22, 2024

View reviewed changes

	Paths that contain drive letters (e.g. `C:`) must be declared as an absolute file URI, for example: `import "file:///C:/path/to/my/module.pkl"`. Otherwise, they would be interpreted as a URI scheme.
	Paths that contain drive letters (e.g. `C:`) must be declared as an absolute file URI, for example: `import "file:///C:/path/to/my/module.pkl"`. Otherwise, they are interpreted as a URI scheme.

	throw new IOException("Is a directory");
	throw new IOException("Is a directory", e);

Add support for Windows #492

Are you sure you want to change the base?

Add support for Windows #492

Conversation

bioball commented May 13, 2024 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bioball May 20, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bioball May 23, 2024 • edited

Choose a reason for hiding this comment

bioball May 20, 2024 • edited

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

stackoverflow left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holzensp left a 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

Choose a reason for hiding this comment

bioball May 22, 2024 • edited

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

bioball May 22, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bioball commented May 13, 2024 •

edited

bioball May 20, 2024 •

edited

bioball May 23, 2024 •

edited

bioball May 20, 2024 •

edited

bioball May 22, 2024 •

edited

bioball May 22, 2024 •

edited