Skip to content

Commit

Permalink
Flag use of Unicode less-than and greater-than characters in FilePath…
Browse files Browse the repository at this point in the history
… to be a Windows-only change (#215)

This PR fixes a regression introduced by
#208

That PR aimed to fix #200,
an issue where `std.extVar` failed on Windows because it tried to
resolve mock paths which contained `<` and `>` characters which are
forbidden in Windows filenames.

That PR's solution was to use Unicode less-than and greater-than
characters in place of ASCII `<` and `>`, but that broke things for
Unix/Linux platforms with a non-UTF8 `LANG` (see
#208 (review)).

In this PR, I aim to fix this by flagging the other PR's change to only
occur for Windows, while continuing to use ASCII `<` and `>` as before
on other platforms.

I also added a regression test by pinning `LANG=C` in our GitHub Actions
test setup. This successfully reproduced the bug fixed in this PR, and
also uncovered a minor test-only issue related to the use of default
character encodings (which I've fixed by pinning the test code to
UTF-8).

Note that these file paths aren't actually materialized onto disk:
rather, these are "placeholder / mock" paths used to indicate when
jsonnet is reading from synthetic paths. In theory, we might be able to
avoid the need to ever perform resolution in the first place by changing
uses of these paths to be an `Either` of either an actual `os.Path` or a
placeholder, but that's a much larger and more invasive change than I
want to make right now. Here, I've chosen to pursue a narrowly-targeted
tactical fix-forward.
  • Loading branch information
JoshRosen authored Nov 26, 2024
1 parent 759cea7 commit 0fff9f9
Show file tree
Hide file tree
Showing 5 changed files with 29 additions and 6 deletions.
3 changes: 3 additions & 0 deletions .github/workflows/pr-build.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@ jobs:
runs-on: ubuntu-latest
strategy:
fail-fast: false
env:
# Set LANG=C to simulate least-common-denominator target deployment environments:
LANG: C

steps:
- uses: actions/checkout@v2
Expand Down
2 changes: 1 addition & 1 deletion sjsonnet/src-jvm-native/sjsonnet/SjsonnetMain.scala
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,7 @@ object SjsonnetMain {
std: Val.Obj = new Std().Std): Either[String, String] = {

val (jsonnetCode, path) =
if (config.exec.value) (file, wd / "\uFE64exec\uFE65")
if (config.exec.value) (file, wd / Util.wrapInLessThanGreaterThan("exec"))
else {
val p = os.Path(file, wd)
(os.read(p), p)
Expand Down
2 changes: 1 addition & 1 deletion sjsonnet/src/sjsonnet/Interpreter.scala
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ class Interpreter(extVars: Map[String, String],


def parseVar(k: String, v: String) = {
resolver.parse(wd / s"\uFE64$k\uFE65", StaticResolvedFile(v))(evaluator).fold(throw _, _._1)
resolver.parse(wd / Util.wrapInLessThanGreaterThan(k), StaticResolvedFile(v))(evaluator).fold(throw _, _._1)
}

lazy val evaluator: Evaluator = createEvaluator(
Expand Down
19 changes: 19 additions & 0 deletions sjsonnet/src/sjsonnet/Util.scala
Original file line number Diff line number Diff line change
Expand Up @@ -33,4 +33,23 @@ object Util{
new String(range.dropWhile(_ < 0).takeWhile(_ < s.length).map(s).toArray)
}
}

val isWindows: Boolean = {
// This is normally non-null on the JVM, but it might be null in ScalaJS hence the Option:
Option(System.getProperty("os.name")).exists(_.toLowerCase.startsWith("windows"))
}

/**
* Wrap the given string in '<' and '>' brackets for pretty printing.
* On Windows, this uses Unicode less-than and greater-than characters, while on
* other platforms it uses ASCII '<' and '>;
* see https://github.com/databricks/sjsonnet/pull/208 for motivation and context.
*/
def wrapInLessThanGreaterThan(s: String): String = {
if (isWindows) {
s"\uFE64$s\uFE65"
} else {
s"<$s>"
}
}
}
Original file line number Diff line number Diff line change
@@ -1,19 +1,20 @@
package sjsonnet

import utest._
import java.io.{File, FileWriter}

import java.io.{File, FileOutputStream}
import java.nio.file.Files
import scala.util.Random

object BufferedRandomAccessFileTests extends TestSuite {
// Utility function to create a temporary file with known content
def createTempFile(content: String): File = {
val tempFile = Files.createTempFile(null, null).toFile
val writer = new FileWriter(tempFile)
val fos = new FileOutputStream(tempFile)
try {
writer.write(content)
fos.write(content.getBytes("UTF-8"))
} finally {
writer.close()
fos.close()
}
tempFile
}
Expand Down

0 comments on commit 0fff9f9

Please sign in to comment.