Repository navigation
fix(macos): resolve deployment and SDK versions independently - #1085
hiyouwhatsapp wants to merge 9 commits into
Conversation
Resolve independent minimum and SDK versions, infer SDK versions from named bundles, and reject invalid or incompatible platform values before link command execution Signed-off-by: hiyouwhatsapp <89997383+hiyouwhatsapp@users.noreply.github.com>
Support explicit overrides, environment fallbacks and versioned SDK roots while rendering both values in link plans and target-spec JSON Validate incompatible versions and reject macOS-only flags on other targets before emitting artifacts Signed-off-by: hiyouwhatsapp <89997383+hiyouwhatsapp@users.noreply.github.com>
…ence Check independent platform-version arguments for x86_64 and arm64, validate overrides and failures in CLI dry-run and target specs, and document cross-link selection Signed-off-by: hiyouwhatsapp <89997383+hiyouwhatsapp@users.noreply.github.com>
Keep the platform-version resolver and backend error handling consistently formatted Signed-off-by: hiyouwhatsapp <89997383+hiyouwhatsapp@users.noreply.github.com>
LunaStev
left a comment
There was a problem hiding this comment.
Reviewed exact head 6545b17. The three new CLI tests and four focused LLVM tests passed, but an untested selected-SDK case is incorrect.
P2 — a sysroot alias can select the host SDK version and bypass compatibility rejection
sdk_version_from_path only examines the supplied path's final lexical component. If --sysroot or SDKROOT points through a symlink/unversioned alias, macos_versions_for falls back to xcrun --sdk macosx --show-sdk-version, which describes the default host SDK instead of that selected root.
I ran the actual PR-built CLI on Linux with controlled xcrun discovery responses, for both Darwin triples:
| Selected path | Resolved SDK at deployment 11.0 | Result at deployment 15.0 |
|---|---|---|
| MacOSX14.4.sdk | 14.4 | rejected, exit 2 |
| Pinned.sdk -> MacOSX14.4.sdk | 15.0 from host discovery | accepted, exit 0 |
Both paths identify the same SDK. The alias therefore changes the reported SDK and permits the very older-SDK/deployment mismatch this PR intends to reject. This is a CLI/discovery reproduction, not a native Darwin link claim.
Minimal controlled reproduction after building this PR with the pinned toolchain:
from pathlib import Path
import os
import subprocess
import tempfile
compiler = Path("target/debug/wavec").resolve()
with tempfile.TemporaryDirectory(prefix="wave-sdk-review-") as temporary:
root = Path(temporary)
sdk = root / "MacOSX14.4.sdk"
sdk.mkdir()
(sdk / "SDKSettings.json").write_text('{"Version":"14.4"}\n')
alias = root / "Pinned.sdk"
alias.symlink_to(sdk.name, target_is_directory=True)
tools = root / "bin"
tools.mkdir()
xcrun = tools / "xcrun"
xcrun.write_text(
'#!/bin/sh\n'
'case "$*" in\n'
' "--sdk macosx --show-sdk-version") printf "15.0\\n" ;;\n'
' *) exit 1 ;;\n'
'esac\n'
)
xcrun.chmod(0o755)
environment = os.environ.copy()
for name in ("SDKROOT", "MACOSX_DEPLOYMENT_TARGET", "MACOSX_SDK_VERSION"):
environment.pop(name, None)
environment["PATH"] = str(tools) + os.pathsep + environment.get("PATH", "")
for triple in ("x86_64-apple-darwin", "aarch64-apple-darwin"):
for selected in (sdk, alias):
result = subprocess.run([
str(compiler), "print", "target-spec", "--format=json",
"--error-format=json", "--target=" + triple,
"--sysroot=" + str(selected),
"--macos-deployment-target=15.0",
], env=environment, capture_output=True, text=True, timeout=15)
print(triple, selected.name, result.returncode,
result.stdout.strip(), result.stderr.strip())Resolve the version from the selected SDK, including aliases or its metadata, or query discovery for that specific SDK. Preserve explicit override precedence. Add alias/unversioned-root tests where the selected SDK and host-default SDK differ, and verify both diagnostics and emitted linker arguments use the selected version.
Keep docs/ local
The maintainer's documentation-location decision keeps Wave's root docs/ local-only and publishes project documentation in wavefnd/wave-platform. Remove docs/macos-linking.md from this Wave PR and place any public documentation in the documentation repository.
Formatting and validation
The touched Rust files also fail cargo +1.89.0 fmt --all -- --check, including macos.rs, backend.rs, cli.rs and the new integration test. Format the touched files, rerun the existing seven focused tests and the selected-vs-host SDK regressions, and obtain successful required checks before merge. Keep the deployment minimum and other target/linker contracts unchanged.
LunaStev
left a comment
There was a problem hiding this comment.
Re-reviewed updated head d1f8bfa.
The original alias/host-default mismatch is fixed for the reported case, and the tracked docs/ file has been removed. Four CLI tests and four focused LLVM tests pass. Direct, aliased and ordinary JSON-metadata SDK14.4 controls now reject deployment15.0 on both Darwin triples.
P2 — parse the SDK metadata field structurally
The new sdk_metadata_version uses split_once on the first text occurrence of "Version", then searches forward for a colon and quoted text. It does not validate the JSON document or select the top-level field. As a result it can use an unrelated nested value or even a string value named Version as the SDK version.
Executed against the actual PR-built CLI on Linux, for both x86_64-apple-darwin and aarch64-apple-darwin, with an unversioned Selected.sdk directory:
| SDKSettings.json | Observed result for deployment15.0 |
|---|---|
| {"Version":"14.4"} | usage error, exit2 (correct control) |
| {"Nested":{"Version":"15.0"},"Version":"14.4"} | accepted, exit0, SDK15.0 |
| {"Description":"Version","Other":"15.0","Version":"14.4"} | accepted, exit0, SDK15.0 |
| {"Nested":{"Version":"15.0"}} | accepted despite missing root Version |
| {"Version":"15.0" (unterminated JSON) | accepted despite malformed input |
The first two accepted well-formed cases still declare the SDK's root Version as14.4, so the new parser bypasses the older-SDK compatibility rejection. This reproduces without an installed Apple SDK or host xcrun; it is metadata/CLI validation, not native Darwin linking.
Minimal reproduction after building this PR with the pinned toolchain:
from pathlib import Path
import os
import subprocess
import tempfile
compiler = Path("target/debug/wavec").resolve()
environment = os.environ.copy()
for name in ("SDKROOT", "MACOSX_DEPLOYMENT_TARGET", "MACOSX_SDK_VERSION"):
environment.pop(name, None)
cases = {
"control": '{"Version":"14.4"}',
"nested": '{"Nested":{"Version":"15.0"},"Version":"14.4"}',
"value_collision": '{"Description":"Version","Other":"15.0","Version":"14.4"}',
"missing_root": '{"Nested":{"Version":"15.0"}}',
"malformed": '{"Version":"15.0"',
}
with tempfile.TemporaryDirectory(prefix="wave-sdk-metadata-") as temporary:
sdk = Path(temporary) / "Selected.sdk"
sdk.mkdir()
for name, metadata in cases.items():
(sdk / "SDKSettings.json").write_text(metadata, encoding="utf-8")
for target in ("x86_64-apple-darwin", "aarch64-apple-darwin"):
result = subprocess.run([
str(compiler), "print", "target-spec", "--format=json",
"--error-format=json", "--target=" + target,
"--sysroot=" + str(sdk),
"--macos-deployment-target=15.0",
], env=environment, capture_output=True, text=True, timeout=15)
print(name, target, result.returncode,
result.stdout.strip(), result.stderr.strip())Read the selected SDK's top-level version through a structured parser that validates the complete input, using the existing project JSON facilities. Missing, malformed or wrong-typed SDK version metadata must not be replaced by an unrelated later/nested field. Preserve the selected-root/explicit-override rules. Apply the same key/value ownership principle to the plist fallback rather than selecting an arbitrary subsequent string.
Add regressions for the controls above, verifying both source diagnostics and linker-plan versions. Retain the alias and unversioned-root checks already added.
Formatting remains
cargo +1.89.0 fmt --all -- --check still exits1, with differences in backend.rs, macos.rs, cli.rs and macos_link_versions.rs. The exact-head CI 41822853472 also fails that command. Generate the touched-file output with the pinned formatter and repository configuration, then rerun the complete check.
Separate failing check
The FreeBSD package job additionally reports a failed assertion error.message().contains("started") in src/std/process.rs:254, test timeout_kills_descendants_holding_output_pipes_and_releases_budget. That test is outside this patch's modified files; I have not established that this macOS change caused it. It needs separate triage, while final required checks must still succeed. Do not weaken the timeout assertion to conceal the failure.
|
Remaining changes before merge — reviewed head The original SDK alias issue and tracked docs/ file are addressed. Please fix the remaining SDK metadata parsing error and formatting:
{"Nested":{"Version":"15.0"},"Version":"14.4"}The actual CLI then accepts deployment 15.0 on both Darwin triples although the root SDK version is 14.4. It also accepts a Version-valued string collision, missing root Version and malformed JSON. Use the existing structured JSON facilities, preserve field ownership for the plist fallback, and retain selected-SDK/explicit-override precedence.
cargo +1.89.0 fmt --all
cargo +1.89.0 fmt --all -- --check
cargo +1.89.0 test --locked --jobs 2 -p wavec --test macos_link_versions
cargo +1.89.0 test --locked --jobs 2 -p llvm --lib macosKeep the formatting diff scoped to this PR. Separately, the inspected FreeBSD package job 126550839529 failed an existing process-timeout test's started-marker assertion. That test is outside these modified files; causation by this patch is not established. It needs independent triage while final required checks still need to succeed. Detailed findings and reproduction: review. |
LunaStev
left a comment
There was a problem hiding this comment.
Reviewed head c44d107e79690b3275b18976948f2e33265aaf28.
The previous SDK metadata request is addressed. JSON is parsed through the existing WSON parser and selects the root string Version; plist data is parsed structurally and owns the root dictionary key. The nested Version, string-value collision, missing root, malformed JSON, wrong type and duplicate JSON-key cases are covered. Selected SDK aliases and explicit-override behavior remain preserved, and root docs/ stays out of the patch.
Local Rust 1.89.0 / LLVM 21 checks passed: full Cargo formatting, all 6 focused LLVM/macOS unit tests, and all 5 CLI integration tests. The integration tests exercise both Darwin triples and linker plans, including rejection of the earlier metadata bypass. This is Linux-hosted CLI/planning validation, not native Darwin execution; the exact-head CI macOS lanes also succeeded.
Code review approved. CI 42059038724 still has 49 successes and one failed Linux/riscv64 cases shard. Job 127370127920 times out after 30 minutes in prerequisite installation, while Ubuntu mirror downloads for the cross compiler repeatedly stall/retry. It does not reach compiler or case execution. The earlier FreeBSD marker failure is absent from this run; the FreeBSD package job succeeds.
Keep the PR open until its remaining required check succeeds normally. Do not weaken the timeout, package verification or required-check policy to obtain a green result. No CI rerun/watch was started during this review.
Fixes #415
The Darwin CLI linker plan reused one version for minimum deployment and SDK, while the lower LLVM linker still hard-coded both to 11.0
I added a shared version resolver and wired both the compiler driver and LLVM linker to independent validated deployment and SDK values
Added
--macos-deployment-targetand--macos-sdk-versionoptions with documented environment, SDK root and default precedenceThe resolved values now appear in human and JSON dry-run plans and target-spec output, with malformed versions and SDKs older than the deployment minimum rejected before linking
Added regression tests for x86_64 and AArch64 linker arguments, explicit overrides, SDK bundle detection, precedence and invalid configurations
Documented cross-link behavior and kept the default deployment minimum at 11.0
Haven't run the Rust test suite locally, CI still needs to verify the change