Skip to content
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

Mingw64 support #809

Merged
merged 30 commits into from
Nov 11, 2020
Merged

Mingw64 support #809

merged 30 commits into from
Nov 11, 2020

Conversation

martinbonnin
Copy link
Contributor

@martinbonnin martinbonnin commented Nov 8, 2020

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.

@martinbonnin martinbonnin marked this pull request as draft November 8, 2020 23:48
Copy link
Collaborator

@swankjesse swankjesse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat.

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

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

abstract val separator: String
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

okio/src/jvmMain/kotlin/okio/Pipe.kt Outdated Show resolved Hide resolved
okio/src/jvmTest/java/okio/WaitUntilNotifiedTest.java Outdated Show resolved Hide resolved
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`() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@swankjesse swankjesse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Getting close. Thanks for driving this.

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

abstract val separator: String
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

abstract val separator: String

/**
* Creates a temporary directory
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
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`() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
okio/src/jvmTest/java/okio/WaitUntilNotifiedTest.java Outdated Show resolved Hide resolved
* 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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@swankjesse swankjesse Nov 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@JakeWharton JakeWharton Nov 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, let me try

Copy link
Contributor Author

@martinbonnin martinbonnin Nov 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

val SYSTEM: Filesystem
get() {
return PLATFORM_FILESYSTEM
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

@swankjesse swankjesse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice PR!

* 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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense 👍 . Thanks!

@swankjesse swankjesse merged commit 0613e03 into square:master Nov 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MingwX64 support ?
4 participants