-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Add local build API for direct filesystem builds on MacOS and Windows #27059
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
base: main
Are you sure you want to change the base?
Conversation
9e81b73
to
46d2fc6
Compare
[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore. |
cf65a4c
to
25cc198
Compare
f4c1c04
to
f19d0f3
Compare
code LGTM ... if you get asked to fix things by others, adding a little commentary inside some of those functions might help the next person. again only if you have to repush and have time. |
internal/localapi/utils.go
Outdated
result[path] = mapping.RemotePath | ||
} | ||
|
||
logrus.Debugf("Translation map: %#v\n", result) |
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 need this ? This message has no context. Translation Map for what.
func validateLocalPath(path string) error { | ||
if !filepath.IsAbs(path) { | ||
return fmt.Errorf("path %q is not absolute", path) | ||
} | ||
|
||
if err := fileutils.Exists(path); err != nil { | ||
return err | ||
} | ||
return nil | ||
} | ||
|
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.
I think we have this function elsewhere as well, could we do a quick search before adding this again ?
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.
I didn't find a similar function, but I moved it to the local API to be provided to others in the future.
return nil, utils.GetInternalServerError(fmt.Errorf("additionalbuildcontexts must be in name=value format: %q", url)) | ||
} | ||
|
||
logrus.Debugf("name: %q, context: %q", name, value) |
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.
This message needs more context
pkg/bindings/images/build.go
Outdated
if isStdIn { | ||
stdinFile, err := tempManager.CreateTempFileFromReader(contextDir, "podman-build-stdin-*", os.Stdin) | ||
if err != nil { | ||
return nil, fmt.Errorf("processing stdin: %w", err) |
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.
return nil, fmt.Errorf("processing stdin: %w", err) | |
return nil, fmt.Errorf("processing containerfile from stdin: %w", err) |
f19d0f3
to
aa36c82
Compare
} | ||
|
||
for _, c := range containerFiles { | ||
// Don not add path to containerfile if it is a URL |
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.
// Don not add path to containerfile if it is a URL | |
// Do not add path to containerfile if it is a URL |
Nit
pkg/bindings/images/build.go
Outdated
remoteContainerfile, ok = translationLocalAPIMap[containerfile] | ||
if !ok { | ||
if !isInContextDir { | ||
return nil, fmt.Errorf("containerfile %q not found in translation map", containerfile) |
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.
return nil, fmt.Errorf("containerfile %q not found in translation map", containerfile) | |
return nil, fmt.Errorf("Containerfile %q not found in translation map", containerfile) |
I'll just add this one and it's a nit, but a big nit. Customer facing verbaiage should list this with a capital C
. I'll just comment here, but there are other intances.
LGTM |
aa36c82
to
5a5d58c
Compare
… (only WSL) Adds /libpod/local/build endpoint, client bindings, and path translation utilities to enable container builds from mounted directories to podman machine without tar uploads. This optimization significantly speeds up build operations when working with remote Podman machines by eliminating redundant file transfers for already-accessible files. Fixes: https://issues.redhat.com/browse/RUN-3249 Signed-off-by: Jan Rodák <[email protected]>
Replace single provider.Get() with provider.GetAll() loop to search across all available machine providers when finding machine by port. Signed-off-by: Jan Rodák <[email protected]>
5a5d58c
to
092aaeb
Compare
I fixed the nits and rebased on the latest |
LGTM |
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.
LGTM
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: flouthoc, Honny1 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Happy Green test buttons! |
Adds
/libpod/local/build
endpoint, client bindings, and path translation utilities to enable container builds from mounted directories to podman machine without tar uploads.This optimization significantly speeds up build operations when working with remote Podman machines by eliminating redundant file transfers for already-accessible files.
This feature is non-functional on Windows when using the Hyper-V provider. The issue stems from the 9p filesystem protocol, which fails to correctly translate file attributes between the host and the virtual machine. This incompatibility causes certain container build operations to fail. As a result, this functionality has been disabled for Windows environments that utilize Hyper-V.
Fixes: https://issues.redhat.com/browse/RUN-3249
Fixes Build part of #26321
Benchmark
podman build -t test ./test-context
Benchmark Results:
Mac OS
Providers
applehv
libkrun
Windows
Providers
WSL
Hyper-V
Does this PR introduce a user-facing change?