Skip to content

monitor: resolve paths arguments in client#1581

Merged
jedevc merged 2 commits intodocker:masterfrom
ktock:resolvepath
Mar 2, 2023
Merged

monitor: resolve paths arguments in client#1581
jedevc merged 2 commits intodocker:masterfrom
ktock:resolvepath

Conversation

@ktock
Copy link
Copy Markdown
Collaborator

@ktock ktock commented Feb 1, 2023

Following up for #1296 (comment)

Quoted from @jedevc 's comment (#1296 (comment)):

BUILDX_EXPERIMENTAL=1 buildx build . --builder=dev --invoke=sh
INFO: connecting to buildx server
[+] Building 0.0s (2/2) FINISHED                                                                                                                                  
 => [internal] load build definition from Dockerfile                                                                                                         0.0s
 => => transferring dockerfile: 2B                                                                                                                           0.0s
 => [internal] load .dockerignore                                                                                                                            0.0s
 => => transferring context: 2B                                                                                                                              0.0s
ERROR: failed to build: rpc error: code = Unknown desc = failed to solve: failed to read dockerfile: open /tmp/buildkit-mount3665150422/Dockerfile: no such file or directory

This commit allows using relative paths on buildx build arguments by fixing the controller client to resolve paths on the client side.

@jedevc
Copy link
Copy Markdown
Collaborator

jedevc commented Feb 1, 2023

Hm, I'm fairly sure we should look for a better way to go about this 🤔

As is, this looks really tricky to maintain over time, and I think it's best to avoid modifying the user-provided options wherever possible. I think we need to push down the file resolution to the components that actually open the files, though those are scattered in BuildKit's client code as well as in Buildx.

For example, in Buildkit, we need to handle it here:

buildx/vendor/github.com/moby/buildkit/client/solve.go

Lines 193 to 201 in 6506166

if err := os.MkdirAll(ex.OutputDir, 0755); err != nil {
return nil, err
}
cs, err := contentlocal.NewStore(ex.OutputDir)
if err != nil {
return nil, err
}
contentStores["export"] = cs
storesToUpdate = append(storesToUpdate, ex.OutputDir)

To get the information of the right path through - I think we need to add a current working directory field to the SolveOpt. Then that gives us the ability to handle the paths used in the cache importers/exporters, as well as the dockerfile and contexts.

For things like the metadata file, and secrets, we need to handle those buildx-side - so we'd need to add the same working directory property to the controllerapi.BuildOptions.

I think this would work better as a long-term approach? The buildkit-side patches shouldn't be too complex, and should just affect the client, so we wouldn't need new capabilities or anything.

@tonistiigi are we missing an easier option here?

@jedevc jedevc mentioned this pull request Feb 9, 2023
Merged
@jedevc jedevc mentioned this pull request Feb 9, 2023
Closed
@jedevc
Copy link
Copy Markdown
Collaborator

jedevc commented Feb 24, 2023

With #1614 merged, @ktock, could you rebase this with those most recent changes?

@ktock
Copy link
Copy Markdown
Collaborator Author

ktock commented Feb 24, 2023

@jedevc Thank you for working on #1614. Rebased this PR on that commit.

jedevc
jedevc reviewed Mar 2, 2023
View reviewed changes
Comment thread commands/build.go Outdated
// and replaces them to absolute paths.
func resolvePaths(options *controllerapi.BuildOptions) (_ *controllerapi.BuildOptions, err error) {
if options.ContextPath != "-" {
options.ContextPath, err = absIfLocal(options.ContextPath)
Copy link
Copy Markdown
Collaborator

@jedevc jedevc Mar 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if we should do os.Stat here as part of absIfLocal? I think we should just do filepath.Abs, and similarly below.

We know at this point it's definitely a path, and if the file doesn't exist, we should let the controller throw the error (which may or may not be a file not found error, we shouldn't assume the implementation at this point).

Copy link
Copy Markdown
Collaborator Author

@ktock ktock Mar 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the comment. Yes, we don't need os.Stat here because the options are already strongly typed. Fixed the patch.

Copy link
Copy Markdown
Collaborator

@jedevc jedevc Mar 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just pushed a commit on top to handle the new oci-layout named context (which can also be a path). Does that seem good to you? If so, I think this is ready-to-go 🎉

Copy link
Copy Markdown
Collaborator Author

@ktock ktock Mar 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the following-up! LGTM.

@jedevc jedevc merged commit 2a83723 into docker:master Mar 2, 2023
@ktock ktock deleted the resolvepath branch March 2, 2023 11:59
@jedevc jedevc mentioned this pull request Mar 28, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jedevc jedevc jedevc approved these changes

Assignees

No one assigned

Labels

area/debug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ktock @jedevc