-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Use filepath.Base instead of path.Base #1039
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
Conversation
This ensures Windows paths are handled correctly as explained in the path package documentation. Signed-off-by: Mathieu Champlon <[email protected]>
| } | ||
|
|
||
| fileName := path.Base(config.File) | ||
| fileName := filepath.Base(config.File) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wondering; if someone runs in a Bash session on Windows, and/or uses /c/some/path notation (I think that was supported as well in some shells), would this fail?
Should we uses filepath.FromSlash() first (to normalise the input), then filepath.Base?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works fine, this is what the filepath package takes care of, I believe.
I run everything from Powershell and MinGW64 indifferently and filepath has never failed me.
Given
package main
import (
"fmt"
"path/filepath"
)
func base(p string) {
fmt.Println(filepath.Base(p), p)
}
func main() {
base("/usr/local/bla")
base(`\usr\local\bla`)
base(`C:\usr\local\bla`)
base("C:/usr/local/bla")
base(`bla`)
}
Mingw64, Powershell and Cmd all print the same
bla /usr/local/bla
bla \usr\local\bla
bla C:\usr\local\bla
bla C:/usr/local/bla
bla bla
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Except, most of them should've printed bla?
If you use, for example, C:/Users/me/somefile, does it work, and is the config read from that file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They all printed bla (first column).
Yes C:/Users/me/somefile yields somefile.
What config are you referring to?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh lol, I somehow overlooked the first column, and thought only the last example actually returned bla
What config are you referring to?
sorry, I was typing from my phone so a bit short; Ignore that part, as it's not relevant now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think filepath.FromSlash my be needed here, because filepath.Base is looking for os.PathSeparator. For instance, what happens if the path is /usr/local/bla/?
But I don't have Windows lying around to test this. There should likely be some tests for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thaJeztah pointed out that it's looking at os.IsPathSeparator which on Windows accepts both \\ and / as a path separator.
thaJeztah
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
cpuguy83
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
- What I did
I fixed some path handling code in the Kubernetes
docker stackintegration.This might be related to #966 in some way, however even with the changes contained in that PR the bug fixed here is still happening.
- How I did it
Changed a couple
path.Abstofilepath.Abs.- How to verify it
With Docker on Windows with Kubernetes activated, apply the following change
as this won't work with Kubernetes (the test data come from the e2e tests running against Swarm).
Then without this PR
whereas with this PR
- Description for the changelog
Not sure if this is worth mentioning as this could be sneaked into https://github.com/docker/cli/pull/966…
- A picture of a cute animal (not mandatory but encouraged)