-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
Don't convert slashes for UNIX paths on Windows hosts #6204
Conversation
Signed-off-by: Joffrey F <[email protected]>
34c37f9
to
9f9122c
Compare
""" Custom path normalizer that handles Compose-specific edge cases like | ||
UNIX paths on Windows hosts and vice-versa. """ | ||
|
||
sysnorm = ntpath.normpath if win_host else os.path.normpath |
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.
Shouldn't this be:
-sysnorm = ntpath.normpath if win_host else os.path.normpath
+sysnorm = ntpath.normpath if win_host else posix.normpath
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.
win_host
will be true only if COMPOSE_FORCE_WINDOWS_HOST
is set. In this case we want to produce NT style paths regardless of the client platform. In all other cases, we want to use the default normalization for the client system, hence the use of os.path.normpath
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.
Do we? If you have a compose on Windows talking to a remote linux engine?
Just to note everything related to path here:
The point is: We shouldn't normalize the paths at all. The engine is clever enough to understand what you meant. Or if we really want to (e.g. to change |
I think you make a good point about not having to normalize paths ; that said, I'd want to do a thorough check first to make sure we're not breaking anything by doing so, and have a follow-up PR for that specifically. For the time being, if this fixes docker/for-win#1829, let's roll with it. |
|
Fixes #5716
Fixes docker/for-win#1829