Refactor tmpfsMounts() to process tmpfs path correctly - #2103
Conversation
jglogan
left a comment
jglogan
left a comment
There was a problem hiding this comment.
@LevelVoid Don't forget the absolute path test, see comment.
Also, you'll need to configure your account for verified commit signatures or we can't merge, see: https://docs.github.com/en/authentication/managing-commit-signature-verification/about-commit-signature-verification#about-commit-signature-verification.
Thank you!
| throw ContainerizationError(.invalidArgument, message: "mount destination cannot be empty") | ||
| } | ||
|
|
||
| let normalizedDest = FilePath(destination).lexicallyNormalized().string |
There was a problem hiding this comment.
compute the filepath first and guard to ensure that isAbsolute is true.
Then do the normalization and continue.
Add a test to ensure non-absolute destinations fail.
There was a problem hiding this comment.
compute the filepath first and guard to ensure that
isAbsoluteis true. Then do the normalization and continue.Add a test to ensure non-absolute destinations fail.
Got it, i will do that and push after configuring my account.
|
@LevelVoid Still seeing some formatting issues: ❌ The following files require formatting or license header updates:
Tests/ContainerAPIClientTests/ParserTest.swift |
@jglogan I fixed the issues causing failure, and it's ready to review. |
jglogan
left a comment
jglogan
left a comment
There was a problem hiding this comment.
swift format is quite particular, two spaces
|
@LevelVoid I touched up the formatting. That means I'll need to have someone else approve the PR tomorrow to get it merged, but everything looks good to go. Thank you for fixing this! |
Code Coverage
|
|
Thanks! Appreciate you taking the time to review it. |
Part of #130. ## What apple/container 1.3.0 ([apple/container#2103](apple/container#2103)) rewrote `Parser.tmpfsMounts` to split a `dest:opts` tmpfs entry into a clean destination + options list. Before the fix the entire `/scratch:rw` string became the mount destination. That parser is client-side code Berthly links and calls in-process via `Utility.containerConfigFromFlags`, so the pin bump in #133 already picks up the fix. The Run sheet passes tmpfs entries **verbatim** (`strings(from:)` only trims whitespace) — no app-code change needed. This PR adds the E2E proof: - **`testRunOptionsReachDaemon`** — tmpfs input changed to `/scratch:rw`; asserts no mount destination in `container inspect` carries a colon. - **`testRunOptionsTakeEffectViaExec`** — enters `/scratch:rw`, and since that journey already boots with a **read-only root**, execs in to assert `/proc/mounts` shows `tmpfs` mounted at the clean `/scratch` and `touch /scratch/ok` succeeds (a real, writable, independent mount). ## Test plan - [x] `xcodebuild build-for-testing` (all targets) — succeeds - [x] `swiftlint lint --strict` — 0 violations - [ ] **`scripts/e2e.sh RunContainerJourneyTests/testRunOptionsTakeEffectViaExec` — NOT completed.** Pulling `ghcr.io/apple/containerization/vminit:0.41.0` (needed to boot any 1.3.0 container) fails reproducibly on the test machine's network with `StreamClosed … HTTP2ErrorCode ProtocolError` at ~20 MB of 66 MB, three attempts, same byte offset each time. The first E2E run got as far as filling the sheet and submitting; it only failed waiting for the VM to boot. Needs a run on a network with a clean path to ghcr.io before #130 closes — keeping #130 open for that.
3 participants
Type of Change
Motivation and Context
Fixes #2094 : tmpfs works as stated in issue with tests added
Testing