Add path handling functions#3907
Conversation
| if platform == "" { | ||
| platform = runtime.GOOS | ||
| } | ||
|
|
||
| if platform != "windows" { | ||
| return path, nil | ||
| } |
There was a problem hiding this comment.
I think the previous impl was better with CheckSystemDriveAndRemoveDriveLetter(path string) in path_unix.go and path_windows.go files. Can we keep that?
There was a problem hiding this comment.
Windows hosts can theoretically build Linux images, as it also has support for LCOW (Linux Containers on Windows). We should also be able to build Windows images on Linux. It's a good idea to use the target platform as an indicator of how to handle paths (as well as other things).
In short, the platform we're running on is not necessarily the platform we're building for.
@tonistiigi expressed a similar concern here: #3322 (comment)
There was a problem hiding this comment.
There was a similar discussion in containerd. Which is why, in that instance, we ended up using the image.ImageSpec.OS as an indicator of how to handle the volumes when copying existing contents.
tonistiigi
left a comment
tonistiigi
left a comment
There was a problem hiding this comment.
Why are the tests OS specific? I thought the functions work the same independently from the current platform based on the input platform.
gabriel-samfira
left a comment
gabriel-samfira
left a comment
There was a problem hiding this comment.
Why are the tests OS specific? I thought the functions work the same independently from the current platform based on the input platform.
Good point. I will move them to a non OS specific file. Thanks for the review!
There are several places throughout the code where we repeat the same normalization patterns for paths. This change adds a couple of helper functions and makes CheckSystemDriveAndRemoveDriveLetter() on all platforms. Additionally, all functions accept a platform flag that indicates the target OS we're building the image for. Decissions are made based on this flag instead of checking the platform on which buildkit is running. Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>
gabriel-samfira
force-pushed
the
add-path-handling-functions
branch
from
7b2272b to
d6124fc
Compare
|
@tonistiigi changes made. PTAL. |
| // - optionally keep the trailing slashes on paths | ||
| // - paths are returned using forward slashes | ||
| func NormalizePath(parent, newPath, inputOS string, keepSlash bool) (string, error) { | ||
| if inputOS == "" { |
There was a problem hiding this comment.
Do we need to make os optional? Can't the caller just always pass it?
There was a problem hiding this comment.
I can have the functions err out if it's empty if you prefer. The caller can of course, always pass it, even if they just pass the value of runtime.GOOS.
There was a problem hiding this comment.
Was not sure what the preference was 😄 .
There was a problem hiding this comment.
I'm also fine with just having unix as default like the toSlash etc do. Just having the behavior be different based on runtime.GOOS looks like something that can easily break in the future without we realizing it.
Make sure the behavior is consistent (eg. IsAbs is different atm).
There was a problem hiding this comment.
Fair enough. I set linux as the default.
gabriel-samfira
force-pushed
the
add-path-handling-functions
branch
from
d71c0ed to
f22032f
Compare
Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>
gabriel-samfira
force-pushed
the
add-path-handling-functions
branch
from
f22032f to
feeb3ff
Compare
3 participants
There are several places throughout the code where we repeat the same normalization patterns for paths. This change adds a couple of helper functions and makes CheckSystemDriveAndRemoveDriveLetter() on all platforms.
Additionally, all functions accept a platform flag that indicates the target OS we're building the image for. Decisions are made based on this flag instead of checking the platform on which buildkit is running.
This change was split from #3322. The functions added here are not yet called anywhere in the code. A subsequent PR which will depend on this one will be proposed soon with the bits that make use of these functions.