Repository navigation
feat(docker): refuse --docker for macOS targets (cli#471 X3) - #503
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Warning Review limit reachedNext included review available in 49 minutes. View limit detailsLimit details: You’ve used the included review currently available. Your 105 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (7)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (7)
Included review availability: This review used your included allowance. 0 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour. 📝 WalkthroughWalkthroughDocker builds now refuse macOS targets, whether the target is explicit or inferred from a Mac host. The Docker build script no longer fetches or injects macOS framework paths. Linux and Windows Docker targets remain supported. ChangesDocker macOS target handling
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to The macOS Docker-build restriction appears consistent with the updated build path, tests, and guidance. No issue identified here needs resolution before merge. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
User coderabbitai[bot] does not have write permissions |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 23877aad92
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| std.debug.print("labelle {s}: --target requires a value (e.g. --target=x86_64-windows)\n", .{cmd_name}); | ||
| return null; | ||
| } | ||
| if (dockerTargetRefused(val)) { |
There was a problem hiding this comment.
This validates only explicit --target values and ignores whether Docker is enabled. On a macOS host, labelle build --docker without --target passes parsing, then docker.runBuild falls back to the *-macos host_target (docker.zig:203-205) and attempts the now-unsupported build after the framework setup was deleted. Conversely, --target=x86_64-macos is rejected even without --docker, although the pipeline otherwise treats --target without Docker as a no-op. Perform this check after parsing, conditional on docker_build, against the explicit target or the implicit host target.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 30412061cf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| dir_set = true; | ||
| } | ||
| } | ||
| if (refuseDockerBuild(cmd_name, docker_build, docker_target)) return null; |
There was a problem hiding this comment.
On macOS, labelle generate --docker now fails parsing here because an omitted target is treated as the host macOS target. However, generate returns immediately after the host-side generation stage (pipeline.zig:395) and never calls docker.runBuild; the provider-bypass logic also explicitly treats generate --docker as a supported path. Limit this refusal to commands that actually reach the container build so generation remains usable on Macs.
Useful? React with 👍 / 👎.
| var parts = std.mem.splitScalar(u8, t, '-'); | ||
| _ = parts.next() orelse return false; // arch | ||
| const os = parts.next() orelse return false; | ||
| return std.mem.eql(u8, os, "macos"); |
There was a problem hiding this comment.
When a valid Zig target query includes an OS version, such as aarch64-macos.13.0, the extracted component is macos.13.0, so this equality check lets it through. docker.runBuild then passes that target to Zig after this commit has removed the framework setup, reaching precisely the unsupported macOS link path this guard is intended to reject. Parse the target query or compare the OS tag before its version suffix.
Useful? React with 👍 / 👎.
| \\ has, and a target without its provider fails naming the package to add | ||
| \\ (looked up in the provider registry; `LABELLE_OFFLINE=1` skips the lookup). | ||
| \\ `--docker` builds the `desktop` target only; a provider target refuses it. | ||
| \\ `--target` cross-compiles for Linux or Windows; a macOS target is refused. |
There was a problem hiding this comment.
This new restriction conflicts with docs/migrating-to-3.0.md:137-138, which explicitly says that labelle build --docker [--target=<triple>] for desktop is unchanged. Because the help immediately directs upgrading users to that guide, users of the removed macOS Docker path receive contradictory migration guidance and no explanation of the required native-build replacement. Update the guide alongside this help text.
Useful? React with 👍 / 👎.
3041206 to
46dbf47
Compare
Per the RFC's D6, the container build no longer cross-compiles for macOS. It could only link by cloning a third-party copy of Apple's frameworks and patching the generated build.zig. `--docker --target=<arch>-macos` now fails at argument parsing with a one-line message. Linux and Windows cross builds are unchanged. Deletes the fetch-and-patch snippet, which takes cli/docker.zig off the agnostic guard's allowlist (17 entries).
…ocker Checking only an explicit --target missed a bare `--docker` on a Mac, which falls back to the host's macOS target. It also refused `--target=<arch>-macos` without --docker, where --target is only a warned no-op. Check once after parsing: with --docker, against the explicit target or else the host's.
… 3.0 guide - `generate --docker` generates on the host and never reaches the container build, so it is no longer refused on a Mac. - An OS version suffix (`aarch64-macos.13.0`) no longer hides a macOS target. - docs/migrating-to-3.0.md said `build --docker` for desktop was unchanged. It now says macOS targets are refused and why. - The value-flag test for `build --scene intro --docker` names an explicit Linux target, so it passes on a Mac host too.
46dbf47 to
8fb89fd
Compare
2 participants
Part of #471, item X3 (decision D6: refuse the
--dockermacOS cross build).What
--docker --target=<arch>-macos[-abi]now fails at argument parsing (buildandrun) with:--docker does not cross-compile for macOS (--target=…); build on a Mac, or pick a Linux or Windows target.Corendos/xcode-frameworksand patched the generatedbuild.zig.cli/docker.zigleaves the agnostic guard's allowlist: 18 → 17 entries.Tests
dockerTargetRefused, plus an args test: macOS is refused,x86_64-windowspasses through.zig build testpasses 13/13 steps (1134 tests).It conflicts trivially with #502 (X2) on the allowlist's history comment and count. Whichever merges second rebases.
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.