Handle file paths base on target platform by gabriel-samfira · Pull Request #3908 · moby/buildkit

gabriel-samfira · 2023-05-26T13:48:21Z

This change properly handles paths on different platforms. In short, this change checks the target platform we're building an image for and applies normalization steps to make sure the file paths are valid. This makes buildkit properly handle paths on both *nix systems and on Windows.

Depends on:

Replaces:

[Windows] Handle paths on Windows #3322 (see [Windows] Handle paths on Windows #3322 (comment))

Note: This branch will fail to build until dependencies are merged. Leaving the old branch up, as it contains a longer discussion related to the changes in these 3 PRs.

Here is a test run with all 3 branches merged: https://github.com/gabriel-samfira/buildkit/actions/runs/5091195907

CC: @crazy-max @jedevc

gabriel-samfira · 2023-05-31T21:50:06Z

Rebased, now that the dependencies have been merged. This should now be ready for review.

tonistiigi · 2023-06-01T02:44:27Z

 			mws := mountWithSession(mountable, g)
 			dest := m.Dest
-			if !filepath.IsAbs(filepath.Clean(dest)) {
+			if !system.IsAbs(filepath.Clean(dest), platform) {


This path is coming from LLB. How is it not already normalized?

This function is also called from NewContainer(). I can't remember for sure why I needed this here (first PR was proposed in November), but I can track it down and give you a more detailed answer.

On the other hand, I think that whenever we are dealing with paths that may be meant for a different OS than the one the code is running on, we should use the helper functions in util/system.

gabriel-samfira · 2023-06-26T08:13:52Z

@tonistiigi kind reminder. I would love to unblock the rest of the PRs. The executor work depends on this PR.

tonistiigi · 2023-06-27T05:12:44Z

+	if c.Platform == nil {
+		c.Platform = &ocispecs.Platform{
+			OS:           "linux",
+			Architecture: "amd64",


Is the architecture in here used anywhere? If not then I would prefer if the nil case is handled inside FileOp for OS only and it is clear that this only controls the path conversions.

Removed the Architecture.

What I meant was to handle nil platform inside FileOp. If the logic is only based on OS then we shouldn't control it by passing full platform (or passing an invalid partial platform).

Gotcha. Misread what you meant.

tonistiigi · 2023-06-27T05:20:24Z

 			if d.stage.BaseName == emptyImageName {
 				d.state = llb.Scratch()
 				d.image = emptyImage(platformOpt.targetPlatform)
+				d.platform = &platformOpt.targetPlatform


Looks like this can be nil pointer dereference. In that nil case we should use DefaultSpec

If I am reading the code correctly, the buildPlatforms field defaults to DefaultSpec if it's not set via ConvertOpt and targetPlatform is set to &buildPlatforms[0] if it's nil, before returning.

platformOpt seems to be set at the beginning of this function using buildPlatformOpt() which always seems to return a non-nil instance of the platformOpt type.

Would you prefer I add a nil pointer check here, in case buildPlatformOpt() changes at some point?

Ah, I read it wrong. This is not even a pointer, so it can't be nil.

tonistiigi · 2023-06-27T05:25:12Z

 				a = a.Copy(st, f, dest, opts...)
 			}
 		} else {
+			src, err = system.NormalizePath("/", src, d.platform.OS, false)


I'm still confused why are doing these path transformations twice. If we already are always converting paths to UNIX in Dockerfile before passing to the LLB then why are we still carrying OS value in LLB and doing extra conversions there.

I can remove the calls to NormalizePath() from client/llb and just use the paths as they come in, if you wish.

The current code in the master branch calls normalizePath(). This code is now incorporated into system.NormalizePath(), with additional logic to take into account the input platform.

Would you prefer we do away with this and just use path.Join() in toProtoAction() (example: https://github.com/moby/buildkit/blob/master/client/llb/fileop.go#L340) ?

It's fine with me, either way 😄

Alternatively, in the client I can just hardcode "linux" as the platform sent into NormalizePath() and it would behave just like the current normalizePath() in client/llb, but I am not sure ignoring the input platform (if set) is worth it. In any case, let me know what you prefer and I will make the change.

If you don't need NormalizePath calls inside client/llb to make windows work then better to remove it.

I can revert the change to that file and bring back normalizePath() as it is in the master branch, because that is somewhat still needed. Although that code is now incorporated in system.NormalizePath().

Will change in the morning. I need to see if anything breaks (with a clear head).

I reverted most of the changes to the client. Please take another look.

This change properly handles paths on different platforms. In short, this change checks the target platform we're building an image for and applies normalization steps to make sure the file paths are valid. This makes buildkit properly handle paths on both *nix systems and on Windows. Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>

Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>

* Set default platform for scratch images * Set default platform constraint for FileOps if none is set via ConstraintOpt. * Explicitly set platform ConstraintOpt in dockerfile frontend based on dispatch state platform. Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>

In dockerfile2llb we ensure we use proper path separators before we send them to LLB. In LLB we default to linux/amd64 if no constraints are set by the caller. Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>

Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>

When running on Linux, filepath.ToSlash() is not suitable to convert paths containing Windows path delimiter to Unix delimiters. To enable this, we export system.ToSlash() that takes a platform flag, based on which we use the proper file path separator, regardless of the platform we run on. Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>

tonistiigi · 2023-07-07T02:33:11Z

-	wd := c.Path
-	if !path.IsAbs(c.Path) {
-		wd = path.Join("/", d.image.Config.WorkingDir, wd)
+	wd, err := system.NormalizeWorkdir(d.image.Config.WorkingDir, c.Path, d.platform.OS)


Follow-up: Naming is confusing here NormalizeWorkdir returns OS-specific paths and NormalizePath Unix-only paths. "Normalize" should mean the same thing.

I am terrible at naming things, and can't really think of a better name right now. I did make sure that the doc string for the function does state explicitly that for Windows, we convert path delimiter with \. If it's alright with you, I'd like to revisit the name at a later time. Or if you have any suggestions for a better name, I'll gladly change it.

Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>

gabriel-samfira mentioned this pull request May 26, 2023

[Windows] Handle paths on Windows #3322

Closed

gabriel-samfira force-pushed the handle-platform-file-paths branch from 83cada2 to ac3d451 Compare May 31, 2023 18:47

tonistiigi reviewed Jun 1, 2023

View reviewed changes

gabriel-samfira requested a review from tonistiigi June 7, 2023 21:30

This was referenced Jun 8, 2023

[Windows] Fix local mounter on Windows #3783

Merged

As a Developer I want to be able to build Windows Containers with BuildKit microsoft/Windows-Containers#34

Open

gabriel-samfira force-pushed the handle-platform-file-paths branch 2 times, most recently from ba7f3b9 to 5a4f37f Compare June 15, 2023 23:25

tonistiigi reviewed Jun 27, 2023

View reviewed changes

gabriel-samfira force-pushed the handle-platform-file-paths branch from 5d2cdce to b7985be Compare June 27, 2023 20:14

tonistiigi added this to the v0.12.0 milestone Jun 29, 2023

gabriel-samfira added 10 commits July 3, 2023 08:11

Use current OS as a default

236d00b

Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>

Rename variable, remove superfluous check

7a503ae

Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>

Remove nil pointer check and extra NormalizePath

b29ec0b

Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>

Add nil pointer check in dispatchWorkdir

fbbaa39

Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>

Ensure we use proper path separators

edae0c6

In dockerfile2llb we ensure we use proper path separators before we send them to LLB. In LLB we default to linux/amd64 if no constraints are set by the caller. Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>

Default to linux in client

d425c7c

Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>

Remove Architecture

45b4617

Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>

Revert most changes to client/llb

805b993

Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>

gabriel-samfira force-pushed the handle-platform-file-paths branch from 7022827 to 1045713 Compare July 3, 2023 15:12

gabriel-samfira force-pushed the handle-platform-file-paths branch from 1045713 to 69f2c06 Compare July 3, 2023 15:30

gabriel-samfira requested a review from tonistiigi July 6, 2023 21:23

tonistiigi reviewed Jul 7, 2023

View reviewed changes

Fix various nits

f1657ec

Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>

gabriel-samfira force-pushed the handle-platform-file-paths branch from b829c52 to f1657ec Compare July 10, 2023 19:58

tonistiigi approved these changes Jul 10, 2023

View reviewed changes

tonistiigi merged commit 595bfa2 into moby:master Jul 10, 2023

crazy-max added the area/windows label Oct 9, 2023

Conversation

gabriel-samfira commented May 26, 2023

Uh oh!

gabriel-samfira commented May 31, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gabriel-samfira commented Jun 26, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tonistiigi Jun 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gabriel-samfira Jun 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tonistiigi Jun 27, 2023 •

edited

Loading

gabriel-samfira Jun 27, 2023 •

edited

Loading