-
Notifications
You must be signed in to change notification settings - Fork 581
Fix relative path resolution in entrypoint #987
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?
Fix relative path resolution in entrypoint #987
Conversation
jglogan
left a comment
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.
@ParkSeongGeun Thanks! It's a good start but a few things:
- Please rebase against main to resolve conflicts from the ContainerClient refactor.
- Extract your resolution code into a private function.
- Add handling for just
commandwith no slashes at all (see comment with example that fails in this branch with container but works with the Docker CLI in Colima).
| if !arguments.isEmpty { | ||
| result.append(contentsOf: arguments) | ||
| } else { | ||
| if let cmd = config?.cmd, !hasEntrypointOverride, !cmd.isEmpty { | ||
| result.append(contentsOf: cmd) | ||
| if let first = cmd.first, !first.hasPrefix("/") { |
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.
Looks like we're repeating the same code in three places - extract method?
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.
@jglogan This is my mistake. I've refactored the path resolution logic into a private helper function called resolveExecutablePath
| .standardized | ||
| .path | ||
| result = [resolved] | ||
| } |
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 doesn't cover all cases. Please add handling for bare commands:
% container run --rm -w / --entrypoint ls alpine
Error: internalError: "failed to find target executable /ls"
% docker run --rm -w / --entrypoint ls alpine
bin
dev
etcThere 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.
@jglogan Thank you for catching this case.
I've updated the logic to ensure that bare commands (those without any slashes) are not resolved against the working directory.
4081f4c to
2f49058
Compare
Resolve relative paths (e.g., ./rustc) in --entrypoint flag by combining them with the working directory. This enables commands like: container run -w /usr/local/cargo/bin --entrypoint ./rustc rust:latest Fixes apple#962
2f49058 to
9601f9d
Compare
|
@jglogan Thanks for the thorough review I've rebased the branch on the latest main and addressed all the feedback.
|
|
@ParkSeongGeun I think this works, but I'm curious as to the root cause for this error in your original failure case: % container run -it --rm -w /bin --entrypoint ./uname alpine:latest
Error: internalError: "failed to find target executable ./uname"We might be able to address this down in containerization instead, such that we don't need to modify the path in the Bundle that gets generated. I feel that that is the cleaner approach. Let me know if you're interested in looking at that. The bit of vmexec where the failure occurs is https://github.com/apple/containerization/blob/main/vminitd/Sources/vmexec/vmexec.swift#L74. It might be as simple as pointing an AI assistant at a context of container, containerization, a failing command example, and this line of code and letting it crank out an answer for you. Or, troubleshoot it the old school way! In either case see https://github.com/apple/container/blob/main/BUILDING.md for how to build |
|
@jglogan I agree that fixing this problem in I'll set up the local environment settings and wait for your guidance in #1030 on how to use a custom init filesystem. It's interesting to dive deeper into the root cause. Thanks for your detailed explanation! |
|
@ParkSeongGeun The missing instructions were in the PR. I just merged them into BUILDING.md. TL;DR:
When you build container in step 4, the build runs The system property change tells container to use that local init filesystem image instead of the release image hosted on GHCR. Don't forget to unedit the package and clear the system property to get back to using the normal containerization dependency and init fs. |
Type of Change
Motivation and Context
resolveExecutablePathprivate helper to eliminate code duplication.Testing
Verification Scenarios
Verified the fix on macOS 26.0 (Target: arm64-apple-macosx26.0) with the following integration tests
Bare command:
container run --rm -w / --entrypoint ls alpineRelative path with
./:container run --rm --workdir /bin --entrypoint ./ls alpine/bin/lsand executed successfully.Relative path without prefix:
container run --rm --entrypoint bin/ls alpineAbsolute path (Regression):
container run --rm --entrypoint /bin/ls alpinePath Normalization: Verified that complex paths like
../bin/lsare standardized correctly viaURL.standardized.Unit Tests
Ran
swift test --filter ProcessEntrypointto confirm all logic cases inParserTest.swiftpass.