-
Notifications
You must be signed in to change notification settings - Fork 257
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
Add support for Windows #492
Conversation
42c2cdb
to
57da202
Compare
cdd599f
to
64086cd
Compare
.circleci/config.pkl
Outdated
|
||
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" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will remove this from the CI definition before merging so we don't run build native jobs on PRBs.
find . -type f -regex ".*/build/test-results/.*xml" -exec cp {} ~/test-results/ \\; | ||
""" | ||
`when` = "always" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
|
||
/// Whether this is a release build or not. | ||
isRelease: Boolean = false | ||
|
||
/// The OS to run on | ||
os: "macOS"|"linux"|"windows" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved this from BuildNativeJob
, because this adds a ./gradlew check
job for Windows to PRBs.
.start() | ||
process.waitFor().also { exitCode -> | ||
if (exitCode == -1) throw RuntimeException(process.errorStream.reader().readText()) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
|
||
@get:Input | ||
val jvmArgs: ListProperty<String> = project.objects.listProperty() | ||
abstract val jvmArgs: ListProperty<String> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is unrelated to Windows. Happy to move this to a separate PR if you'd prefer.
@@ -158,7 +160,7 @@ constructor( | |||
} else { | |||
if (output.isNotEmpty()) { | |||
outputFile.writeString( | |||
options.moduleOutputSeparator + IoUtils.getLineSeparator(), | |||
options.moduleOutputSeparator + '\n', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ...
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CRLF
^^ re: consistency for pkl eval
; I'd prefer consistency across commands.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
7e2e363
to
349b069
Compare
@@ -4,3 +4,4 @@ | |||
/docs/** linguist-documentation | |||
|
|||
*.pkl linguist-language=Groovy | |||
* text eol=lf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needed for git, and for spotless apply.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be worth an .editorconfig
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
chmod +x jpkl | ||
./jpkl --version | ||
Invoke-WebRequest '{uri-pkl-windows-download}' -OutFile pkl.exe | ||
chmod +x pkl.exe |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does chmod
exist on cmd
or PowerShell
, or is this taking in consideration you are running bash?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, good catch.
@@ -180,6 +192,10 @@ public static String getLineSeparator() { | |||
return System.getProperty("line.separator"); | |||
} | |||
|
|||
public static Boolean isWindows() { | |||
return System.getProperty("os.name").contains("Windows"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: with the amount of times this function may be called perhaps would be a good idea to cache it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll switch this to use Platform.current().operatingSystem()
, which avoids the contains
call every time.
@@ -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("@"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a bugfix? isAbsoluteImport("pkl:math")
returned false
before this change, now returns true
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments, but no blockers.
@@ -134,6 +136,11 @@ local buildNativeJobs: Mapping<String, BuildNativeJob> = new { | |||
musl = true | |||
isRelease = _dist == "release" | |||
} | |||
["pkl-cli-windows-amd64-\(_dist)"] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we considering Windows-on-ARM just-not-there-yet? With the upcoming SnapDragon's, I expect this to be a thing real quick.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -4,3 +4,4 @@ | |||
/docs/** linguist-documentation | |||
|
|||
*.pkl linguist-language=Groovy | |||
* text eol=lf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be worth an .editorconfig
?
./jpkl --version | ||
Invoke-WebRequest '{uri-pkl-windows-download}' -OutFile pkl.exe | ||
chmod +x pkl.exe | ||
.\pkl.exe --version |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit; .\
& .exe
read as unidiomatic to me. I thought .
was part of `%PATH% by default?
.\pkl.exe --version | |
pkl --version |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my shell, .\
is needed. Looks like .exe
can be omitted, though.
@@ -158,7 +160,7 @@ constructor( | |||
} else { | |||
if (output.isNotEmpty()) { | |||
outputFile.writeString( | |||
options.moduleOutputSeparator + IoUtils.getLineSeparator(), | |||
options.moduleOutputSeparator + '\n', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
@@ -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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ;)
// (require `/` as the path separator on all OSes) | ||
var uriPath = uri.getPath(); | ||
if (java.io.File.separatorChar == '\\' && uriPath != null && uriPath.contains("\\")) { | ||
throw new FileNotFoundException(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair point; will add an extra hint here about it. But I'm thinking let's just show this on every OS. It may or may not be useful to users on *nix platforms that are used to Windows style paths. Also, having this be Windows only greatly complicates our testing.
// Windows throws `AccessDeniedException` when reading directories. | ||
// Sync error between different OSes. | ||
if (Files.isDirectory(path)) { | ||
throw new IOException("Is a directory"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we not preserve the trace? I'd, at least, add e
as a cause.
throw new IOException("Is a directory"); | |
throw new IOException("Is a directory", e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While this is generally a good idea, it works against us here because the cause gets rendered in the -- Pkl Error --
message. The idea here is that we want to make this error message identical on the two platforms.
@@ -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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... 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.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be inclined to push it in
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())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually find the former a little easier to read, because of fewer binary operators grouped in the if
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
Co-authored-by: Philip K.F. Hölzenspies <holzensp@gmail.com>
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 ensureconsistent rendering across platforms.
jpkl
does not work on Windows as a direct executable.However, it can work with
java -jar jpkl
.Additional details:
IoUtils.relativize
, because the current implementation delegates toPath
, which will fail if the URI contains invalid Windows characters.Property
values./gradlew test
commandspkl:platform
'scurrent.operatingSystem.name
is"Windows"
for all Windows OSes (match treatment of macOS/Linux).