Mingw64 support #809

martinbonnin · 2020-11-08T23:18:58Z

Support for windows platform, mostly changes to Pipe timeouts to handle the coarse timer resolution on the Windows JVM and some tests relaxed. See #743 for more details (closes #743)
Edit: oops, I didn't see the okio-files module. I'll need to make this work on windows too, I'll revisit this in the coming days
Edit2: the Pipe timeouts changes got their own pull request: #811. So this pull request is mostly about okio-files now.

therefore not throw here

swankjesse

Neat.

Great minds think alike. I just added a native Windows file system.

swankjesse · 2020-11-09T03:27:01Z

okio-files/src/commonMain/kotlin/okio/Filesystem.kt

@@ -109,6 +109,8 @@ abstract class Filesystem {
  @Throws(IOException::class)
  abstract fun delete(path: Path)

+  abstract val separator: String


Difficult to get the name here. There's a path separator (: or ;) and a directory separator (\ or /).

I was halfway considering calling the val slash ?

I went with the same naming as Java that has separator and pathSeparator. The reasoning was that people coming from Java shouldn't be disoriented. Maybe that doesn't work too well for native/js but yea coming up with something that works everywhere is hard.

Yep, but in Java it's defined on File and this is on Filesystem. My preferred place for this is Path.slash or Path.directorySeparator.

Moved to Path.directorySeparator using a platform expect/actual.

okio-files/src/commonTest/kotlin/okio/files/FileSystemTest.kt

okio/src/jvmMain/kotlin/okio/Pipe.kt

okio/src/jvmTest/java/okio/WaitUntilNotifiedTest.java

This reverts commit da5fad2.

martinbonnin · 2020-11-09T10:55:40Z

okio-files/src/commonTest/kotlin/okio/files/FileSystemTest.kt

    Filesystem.SYSTEM.createDirectory(target)
    assertFailsWith<IOException> {
      Filesystem.SYSTEM.atomicMove(source, target)
    }
  }

  @Test
+  @Ignore // somehow the behaviour is different on windows
  fun `atomicMove source is directory and target is file`() {


There's something with atomicMove behaving differently depending the platforms, we might have to write platform dependant tests? Or maybe filesystem dependant tests?

cf https://github.com/square/okio/pull/810/files#diff-5c23c164810a5a043102f66733c86a74ebcb9935847579369f0adea38ba6f2f2R168 too

Yep. Windows cannot do this atomically, so callers need to delete the target first. We need to fix the test to handle either behavior.

swankjesse

Getting close. Thanks for driving this.

swankjesse · 2020-11-09T11:45:22Z

okio-files/src/commonMain/kotlin/okio/Filesystem.kt

@@ -109,6 +109,8 @@ abstract class Filesystem {
  @Throws(IOException::class)
  abstract fun delete(path: Path)

+  abstract val separator: String


Yep, but in Java it's defined on File and this is on Filesystem. My preferred place for this is Path.slash or Path.directorySeparator.

swankjesse · 2020-11-09T11:52:57Z

okio-files/src/commonMain/kotlin/okio/Filesystem.kt

+  abstract val separator: String
+
+  /**
+   * Creates a temporary directory


No, that's too much responsibility. This should be “Returns a writable temporary directory on the current file system. This is java.io.tmpdir on the JVM platform and the TMPDIR environment variable on the POSIX platform”.

We don't want to create a child directory, that's the caller’s option.

okio-files/src/commonMain/kotlin/okio/Filesystem.kt

okio-files/src/commonTest/kotlin/okio/files/FileSystemTest.kt

swankjesse · 2020-11-09T11:58:14Z

okio-files/src/commonTest/kotlin/okio/files/FileSystemTest.kt

    Filesystem.SYSTEM.createDirectory(target)
    assertFailsWith<IOException> {
      Filesystem.SYSTEM.atomicMove(source, target)
    }
  }

  @Test
+  @Ignore // somehow the behaviour is different on windows
  fun `atomicMove source is directory and target is file`() {


Yep. Windows cannot do this atomically, so callers need to delete the target first. We need to fix the test to handle either behavior.

okio-files/src/jvmMain/kotlin/okio/JvmSystemFilesystem.kt

okio/src/jvmTest/java/okio/WaitUntilNotifiedTest.java

…nd will" This reverts commit 132b824.

okio-files/src/commonMain/kotlin/okio/Path.kt

JakeWharton · 2020-11-09T13:28:34Z

okio-files/src/commonMain/kotlin/okio/Filesystem.kt

+   * Returns a writable temporary directory on the current file system.
+   * This is the java.io.tmpdir system property on the JVM platform and the TMPDIR environment variable on the POSIX platform
+   */
+  abstract fun temporaryDirectory(): Path


This doesn't feel like it belongs here since it's not a property intrinsic to a file system. What's the temporary directory for a read-only zip file mounted as a filesystem? A temp dir is a property intrinsic to an operating system or platform.

Seems like it's only used by tests? Maybe just an actual/expect in there to start with?

I wanna use this to write multiplatform tests that use files. For example, tests of DiskLruCache.

A zip file is not an exemplar file system. For example, it doesn't support random access writing.

(and if you are doing a Zip filesystem, you can just return Filesystem.SYSTEM.temporaryDirectory().)

That argument doesn't sway me. I think you've only radicalized me further. Temporary directories aren't an intrinsic property of a file system, they're a property of an operating system. At best it's a path resolvable by FileSystem.SYSTEM but it is not intrinsic to all file systems. It belongs elsewhere.

I am still opposed. What do you do when it returns null?

What about a Samba or NFS filesystem mounted over the network? Or exposing S3 as a filesystem either through HTTP calls or the AWS S3 SDK? A zip file just an easy example to litmus test the API against that should quickly allow us to identify what is something primitive to a filesystem and what is not.

In your DiskLruCache example the test should have no behavioral difference whether given the system FS or an in-memory FS. The test doesn't get to ask for a temporary directory, it's given an FS and a base Path. If the test runner (or parameterized factory or whatever) wants to use the system FS and the OS temporary directory it can, but for an in-memory FS the temporary path can be the root dir.

@martinbonnin wanna pull this back from the public API? @JakeWharton and I will figure out the API in a follow-up.

Sure, let me try

I just made temporaryDirectory() internal. I think that's enough to remove it from the public API. Let me know if you want me to bury it a bit deeper

martinbonnin · 2020-11-09T13:58:46Z

okio-files/src/commonMain/kotlin/okio/Filesystem.kt

+    val SYSTEM: Filesystem
+    get() {
+      return PLATFORM_FILESYSTEM
+    }


There's something fishy here... Not using the getter syntax triggers a NullPointerException in CI. Haven't been able to reproduce locally

swankjesse

Nice PR!

swankjesse · 2020-11-11T01:57:26Z

okio-files/src/commonMain/kotlin/okio/Filesystem.kt

+   * Returns a writable temporary directory on the current file system.
+   * This is the java.io.tmpdir system property on the JVM platform and the TMPDIR environment variable on the POSIX platform
+   */
+  internal abstract fun temporaryDirectory(): Path


In a follow up I’ll move this elsewhere. Otherwise users can’t extend this class because they can’t implement this method.

Makes sense 👍 . Thanks!

martinbonnin added 9 commits November 8, 2020 22:08

add the mingwX64 target

74be8e3

added windows to the CI

630fda8

try compiling windows with java 8

bd7227f

fix some encoding issues

87e0a65

On windows, the JVM might wait for slightly less than asked and will

132b824

therefore not throw here

added a publish-windows job

d8e321b

compile with Java 8

fcf5033

introduce timeout.waitUntil

da5fad2

make ktlint happy

dee5386

martinbonnin marked this pull request as draft November 8, 2020 23:48

martinbonnin added 2 commits November 9, 2020 01:06

trying to make okio-files work on windows

f54b952

fix some more windows tests

232d378

swankjesse requested changes Nov 9, 2020

View reviewed changes

martinbonnin added 8 commits November 9, 2020 11:07

move tmpDirectory to the filesystem

0bdd1c2

remove unecessary string substitution

530baa7

replace line feeds before the asserts

09c8471

Revert "introduce timeout.waitUntil"

d581c7a

This reverts commit da5fad2.

fix some bad copy/paste and leftover files

f309738

make ktlint happy

a30a95a

disable one atomicMove test on windows

8b77c51

remove bad javadoc

26596b5

martinbonnin commented Nov 9, 2020

View reviewed changes

swankjesse reviewed Nov 9, 2020

View reviewed changes

martinbonnin added 7 commits November 9, 2020 13:08

Revert "On windows, the JVM might wait for slightly less than asked a…

c4a9d6b

…nd will" This reverts commit 132b824.

tweak tempDirectory()

18dbd7a

remove wildcard import

9e371ae

assume not windows

07d2a54

more assumeNotWindows tests

9176a5e

move Filesystem.separator to Path.directorySeparator

fb4fd92

fix ktlint

758712d

PromanSEW reviewed Nov 9, 2020

View reviewed changes

okio-files/src/commonMain/kotlin/okio/Path.kt Outdated Show resolved Hide resolved

fix typo

67b2d15

JakeWharton reviewed Nov 9, 2020

View reviewed changes

martinbonnin added 2 commits November 9, 2020 14:51

apparently make tests work again

b202bd3

fix indentation

860fa22

martinbonnin commented Nov 9, 2020

View reviewed changes

martinbonnin marked this pull request as ready for review November 9, 2020 15:33

make temporaryDirectory internal

56f61d5

swankjesse approved these changes Nov 11, 2020

View reviewed changes

swankjesse merged commit 0613e03 into square:master Nov 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mingw64 support #809

Mingw64 support #809

martinbonnin commented Nov 8, 2020 •

edited

Loading

swankjesse left a comment

swankjesse Nov 9, 2020

martinbonnin Nov 9, 2020

swankjesse Nov 9, 2020

martinbonnin Nov 9, 2020

martinbonnin Nov 9, 2020

swankjesse Nov 9, 2020

swankjesse left a comment

swankjesse Nov 9, 2020

swankjesse Nov 9, 2020

swankjesse Nov 9, 2020

JakeWharton Nov 9, 2020

JakeWharton Nov 9, 2020

swankjesse Nov 10, 2020

swankjesse Nov 10, 2020 •

edited

Loading

JakeWharton Nov 10, 2020

JakeWharton Nov 10, 2020 •

edited

Loading

swankjesse Nov 10, 2020

martinbonnin Nov 10, 2020

martinbonnin Nov 10, 2020 •

edited

Loading

swankjesse Nov 11, 2020

martinbonnin Nov 9, 2020

swankjesse left a comment

swankjesse Nov 11, 2020

martinbonnin Nov 11, 2020

Mingw64 support #809

Mingw64 support #809

Conversation

martinbonnin commented Nov 8, 2020 • edited Loading

swankjesse 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

swankjesse 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

swankjesse Nov 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JakeWharton Nov 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinbonnin Nov 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swankjesse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinbonnin commented Nov 8, 2020 •

edited

Loading

swankjesse Nov 10, 2020 •

edited

Loading

JakeWharton Nov 10, 2020 •

edited

Loading

martinbonnin Nov 10, 2020 •

edited

Loading