-
Notifications
You must be signed in to change notification settings - Fork 2.3k
feat: add Windows support by handling empty path case in PathSplit function #5944
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
base: master
Are you sure you want to change the base?
feat: add Windows support by handling empty path case in PathSplit function #5944
Conversation
The committers listed above are authorized under a signed CLA. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: pcanilho The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Welcome @pcanilho! |
|
Hi @pcanilho. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
Hi @pcanilho Thanks for open PR. https://github.com/kubernetes-sigs/kustomize/blob/master/CONTRIBUTING.md#reviewer-guide
|
|
Sorry I missed your message, @koba1t! Regarding the test coverage: I noticed the existing test suite for this functionality intentionally excludes Windows (https://github.com/kubernetes-sigs/kustomize/blob/master/kyaml/filesys/util_test.go#L4-L5). Would you prefer that I adapt those tests to be cross-platform compatible, or should I create a separate Windows-specific test suite instead? I'm happy to take either approach and just want to follow the project's preferred testing pattern. |
|
I think the test cases in |
Goal: Fix
PathSplitfunction to handle empty path case forWindowssupportContext: This PR fixes an issue I was having when adding
Windowssupport for some of my utilities by updating the function to handle the case when is empty.The issue
When running on
Windows, I was getting errors because the function wasn't handling empty paths correctly. The problem occurs becauseWindowsuses backslashes (\) as path separators, so when resolves to\, the path splitting logic behaves differently than on Unix systems.For example, I was seeing errors like this locally:
Causing a
stack overflow:click here to see the stack trace... ❗️
fatal error: stack overflow runtime stack: runtime.throw({0x7ff62c817a83?, 0x7ff62a2ef630?}) C:/Program Files/Go/src/runtime/panic.go:1101 +0x38 fp=0x26bffffd60 sp=0x26bffffd30 pc=0x7ff62a331418 runtime.newstack() C:/Program Files/Go/src/runtime/stack.go:1107 +0x464 fp=0x26bffffea0 sp=0x26bffffd60 pc=0x7ff62a318094 runtime.morestack() C:/Program Files/Go/src/runtime/asm_arm64.s:342 +0x70 fp=0x26bffffea0 sp=0x26bffffea0 pc=0x7ff62a336d60 goroutine 52 gp=0x400072a700 m=10 mp=0x4000742008 [running]: internal/filepathlite.1({0x40020ea7b0, 0x7}) C:/Program Files/Go/src/internal/filepathlite/path_windows.go:204 +0x2b8 fp=0x4022d67380 sp=0x4022d67380 pc=0x7ff62a382af8 internal/filepathlite.VolumeName({0x40020ea7b0, 0x7}) C:/Program Files/Go/src/internal/filepathlite/path.go:267 +0x24 fp=0x4022d673b0 sp=0x4022d67380 pc=0x7ff62a381e24 internal/filepathlite.Split({0x40020ea7b0, 0x7}) C:/Program Files/Go/src/internal/filepathlite/path.go:206 +0x24 fp=0x4022d673d0 sp=0x4022d673b0 pc=0x7ff62a381b04 path/filepath.Split(...) C:/Program Files/Go/src/path/filepath/path.go:120 sigs.k8s.io/kustomize/kyaml/filesys.PathSplit({0x40020ea7b0?, 0x7?}) C:/Users/my_user/go/pkg/mod/sigs.k8s.io/kustomize/[email protected]/filesys/util.go:44 +0x28 fp=0x4022d67450 sp=0x4022d673d0 pc=0x7ff62b8c2968 sigs.k8s.io/kustomize/kyaml/filesys.PathSplit({0x40020ea7b0?, 0x7?}) C:/Users/my_user/go/pkg/mod/sigs.k8s.io/kustomize/[email protected]/filesys/util.go:55 +0x168 fp=0x4022d674d0 sp=0x4022d67450 pc=0x7ff62b8c2aa8 sigs.k8s.io/kustomize/kyaml/filesys.PathSplit({0x40020ea7b0?, 0x7?}) C:/Users/my_user/go/pkg/mod/sigs.k8s.io/kustomize/[email protected]/filesys/util.go:55 +0x168 fp=0x4022d67550 sp=0x4022d674d0 pc=0x7ff62b8c2aa8 sigs.k8s.io/kustomize/kyaml/filesys.PathSplit({0x40020ea7b0?, 0x7?}) C:/Users/my_user/go/pkg/mod/sigs.k8s.io/kustomize/[email protected]/filesys/util.go:55 +0x168 fp=0x4022d675d0 sp=0x4022d67550 pc=0x7ff62b8c2aa8 sigs.k8s.io/kustomize/kyaml/filesys.PathSplit({0x40020ea7b0?, 0x7?}) C:/Users/my_user/go/pkg/mod/sigs.k8s.io/kustomize/[email protected]/filesys/util.go:55 +0x168 fp=0x4022d67650 sp=0x4022d675d0 pc=0x7ff62b8c2aa8 sigs.k8s.io/kustomize/kyaml/filesys.PathSplit({0x40020ea7b0?, 0x7?}) C:/Users/my_user/go/pkg/mod/sigs.k8s.io/kustomize/[email protected]/filesys/util.go:55 +0x168 fp=0x4022d676d0 sp=0x4022d67650 pc=0x7ff62b8c2aa8 sigs.k8s.io/kustomize/kyaml/filesys.PathSplit({0x40020ea7b0?, 0x7?}) C:/Users/my_user/go/pkg/mod/sigs.k8s.io/kustomize/[email protected]/filesys/util.go:55 +0x168 fp=0x4022d67750 sp=0x4022d676d0 pc=0x7ff62b8c2aa8 sigs.k8s.io/kustomize/kyaml/filesys.PathSplit({0x40020ea7b0?, 0x7?}) C:/Users/my_user/go/pkg/mod/sigs.k8s.io/kustomize/[email protected]/filesys/util.go:55 +0x168 fp=0x4022d677d0 sp=0x4022d67750 pc=0x7ff62b8c2aa8 sigs.k8s.io/kustomize/kyaml/filesys.PathSplit({0x40020ea7b0?, 0x7?}) C:/Users/my_user/go/pkg/mod/sigs.k8s.io/kustomize/[email protected]/filesys/util.go:55 +0x168 fp=0x4022d67850 sp=0x4022d677d0 pc=0x7ff62b8c2aa8 sigs.k8s.io/kustomize/kyaml/filesys.PathSplit({0x40020ea7b0?, 0x7?}) C:/Users/my_user/go/pkg/mod/sigs.k8s.io/kustomize/[email protected]/filesys/util.go:55 +0x168 fp=0x4022d678d0 sp=0x4022d67850 pc=0x7ff62b8c2aa8 sigs.k8s.io/kustomize/kyaml/filesys.PathSplit({0x40020ea7b0?, 0x7?}) ...Note:
go version go1.24.5 windows/arm64My proposal
I added a simple check to the existing condition in . This prevents issues when working with
Windowssystems.Why this works
The existing tests already pass with this change, which confirms this change does not impact existing unix support. It is aimed at ensuring consistent behaviour across operating systems without requiring more invasive changes.
Testing notes
The existing tests exclude
Windowsintentionally, and I wasn't sure if adapting them would be straightforward since proper testing needs to be done on aWindowshost where resolves to theWindows-specific rune.Any form of feedback is greatly appreciated.