Skip to content

monitor: resolve paths arguments in client #1581

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Mar 2, 2023
Merged

monitor: resolve paths arguments in client #1581

merged 2 commits into from
Mar 2, 2023

Conversation

ktock
Copy link
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
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
Open
@jedevc
Copy link
Collaborator

jedevc commented Feb 24, 2023

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

@ktock
Copy link
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
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
Collaborator

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
Collaborator Author

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
Collaborator

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
Collaborator Author

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.

@ktock
monitor: resolve paths arguments in client
17105bf 
Signed-off-by: Kohei Tokunaga <ktokunaga.mail@gmail.com>
@jedevc
monitor: resolve oci layout path in client
b5ea989 
Signed-off-by: Justin Chadwell <me@jedevc.com>
@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