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

Fix path normalization for Windows OS #3215

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

DimaVilda
Copy link

Issue:
The issue goes from kafka-ui thread kafbat/kafka-ui#261 which uses 'Location.get(...)' function from your library.

  • Location paths don't work correctly on Windows due to path separator differences
  • Windows uses backslashes () while proto imports consistently use forward slashes (/)
  • This causes multiple NPEs on Linker.link(...) method in kafka-ui tests on Windows as the path from 'Location.get(...)' gets incorrect result.

Example:
If in Location.get(...) we pass:
1st param base - C:\Users\username\IdeaProjects\kafka-ui\api\target\test-classes\protobuf-serde
2nd param path - language\language.proto
the result is - C:\Users\username\IdeaProjects\kafka-ui\api\target\test-classes*protobuf-serde/language\language.proto*

And then Linker.link(...) throws NPE:
Screenshot 2024-12-15 130433

Solution:

  • Normalized paths in Location.get(...) method to consistently use forward slashes
  • Added unit test coverage for Location.get(...) method

@JakeWharton JakeWharton changed the title NOTASK - fix path normalization for Windows OS, add unit tests Fix path normalization for Windows OS, add unit tests Dec 15, 2024
@JakeWharton JakeWharton changed the title Fix path normalization for Windows OS, add unit tests Fix path normalization for Windows OS Dec 15, 2024
@oldergod
Copy link
Member

@DimaVilda Thanks for the PR. If existing tests are not affected, I'm happy to merge your fix. It looks like some tests on failing on Windows right now.

@DmytroVilda
Copy link

DmytroVilda commented Dec 16, 2024

@DimaVilda Thanks for the PR. If existing tests are not affected, I'm happy to merge your fix. It looks like some tests on failing on Windows right now.

hi @oldergod ! Lemme check it asap and in case I was wrong will fix it

@DimaVilda
Copy link
Author

DimaVilda commented Dec 16, 2024

@DimaVilda Thanks for the PR. If existing tests are not affected, I'm happy to merge your fix. It looks like some tests on failing on Windows right now.

hi again there @oldergod
I figured out what is the issue of a failed test. Ehh, it's a bit long topic but I try to put it shortly and then we can decide the next steps:
The problem of a failed SchemaLoaderTest test is here:

  private val fs = FakeFileSystem().apply {
    if (Path.DIRECTORY_SEPARATOR == "\\") emulateWindows() else emulateUnix()
  }

the in emptyPackagedProtoMessage() test we get NPE:

    loader.initRoots(
      sourcePath = listOf(Location.get(fs.workingDirectory.toString())),  --> NPE
      protoPath = listOf(Location.get(fs.workingDirectory.toString())),
    )

Cause:
When we pass reverted 'F:/' it fails because of how the current implementation of okio jvm Path toPath(). This function only recognizes Windows volume paths (e.g., "F:") when they use backslashes, despite Windows accepting both forward and backward slashes.
Example:
Call "F:/".toPath()
Expected result: A path representing "F:/"
Actual result: A path representing just "F:" (without the slash)

This issue comes from:

private fun Buffer.startsWithVolumeLetterAndColon(slash: ByteString): Boolean {
    if (slash != BACKSLASH) return false  // This causes forward slashes to fail
    ...
}

While "F:\".toPath() works correctly, "F:/".toPath() doesn't, even though both formats are valid on Windows. Since Windows accepts both slash types interchangeably, the path parsing should ideally handle both formats consistently.
We can either fix test or put this question to okio Path team like why they don't accept both SLASH and BACKSLASH in the volume letter check.
Hope my point is clear

@oldergod
Copy link
Member

It doesn't look like it's part of the spec though that Windows supports or not forward slashes: https://retrocomputing.stackexchange.com/a/28348

@swankjesse did you look into it when making Okio's FileSystem?

@DimaVilda
Copy link
Author

hey hi @oldergod and @swankjesse !
You got any updates about it ?

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.

3 participants