Flag use of Unicode less-than and greater-than characters in FilePath to be a Windows-only change #215
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
JoshRosen
commented
Nov 22, 2024
sjsonnet/src/sjsonnet/Util.scala
Outdated
| @@ -33,4 +33,20 @@ object Util{ | |||
| new String(range.dropWhile(_ < 0).takeWhile(_ < s.length).map(s).toArray) | |||
| } | |||
| } | |||
|
|
|||
| val isWindows: Boolean = System.getProperty("os.name").toLowerCase.startsWith("windows") | |||
There was a problem hiding this comment.
This matches
, but we can't directly that other implementation here.There was a problem hiding this comment.
Come to think of it, though, I'm wondering whether this should be lifted to Platform to ensure it won't blow up on Scala JS.
There was a problem hiding this comment.
Yep, we need to wrap in options to avoid an NPE under ScalaJS: d745f75
See https://scastie.scala-lang.org/VuigbI2gS5G1WOnTYCdTdg for seeing some default properties in ScalaJS (look at Javascript console output after running).
JoshRosen
commented
Nov 22, 2024
stephenamar-db
approved these changes
Nov 22, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes a regression introduced by #208
That PR aimed to fix #200, an issue where
std.extVarfailed 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-UTF8LANG(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=Cin 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
Eitherof either an actualos.Pathor 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.