Skip to content

fix(bindings): close the review findings that followed PR #2937 - #2970

Merged
zancas merged 6 commits into
devfrom
fix/pr2937-review-followups
Oct 16, 2026
Merged

zancas merged 6 commits into
devfrom
fix/pr2937-review-followups

Conversation

@zancas

@zancas zancas commented Oct 15, 2026

Copy link
Copy Markdown
Member

Problem

A review of PR #2937 "ci(bindings): run the Kotlin and Swift wire-contract tests on the shipped Binding Sets" found ten items. PR #2968 "fix(workbench): hide every symbol but the UniFFI exports in the iOS static libraries" addresses none of them. Two items closed on their own: the iOS job has run green on a Mac, and the glossary thread is resolved. This PR closes the other eight.

Change

  1. build-binding-layer ios takes no --out flag. The plan writes the one directory that the shipped Package.swift reads. An explicit --out on ios is refused. The lexical path comparison that guarded the flag is deleted with its test. The iOS workflow still hands that directory to its upload step through the out input.
  2. The Swift contract step streams the standard output of xcodebuild line by line. A stalled simulator now shows its last line before the job timeout.
  3. The Swift suite imports ZingoBindings without @testable. Every converter it reaches is public.
  4. The Gradle build refuses to configure when the Kotlin contract-tests directory or the golden directory is missing. It counts the @Test methods under the contract-tests directory. Every unit-test task must execute exactly that many tests. A NO-SOURCE skip can no longer pass.
  5. The workbench pins SWIFT_CONTRACT_TEST_COUNT to the XCTest methods of the suite, not to the func test substring.
  6. binding_layer.rs imports std::path once and spells path::Component at each use.
  7. The SWIFT_CONTRACT_TESTS_MANIFEST constant is gone. It duplicated the contract-tests directory.

IosSimulator.name stays. The step prints it in its "testing on" line.

Verification

cargo fmt, cargo check --all-targets and cargo clippy --all-targets -- -D warnings pass on the workbench crate. The workbench unit tests did not run here. No Mac and no Gradle ran here. The per-PR Android and iOS jobs are the proof.

🤖 Generated with Claude Code

zancas and others added 3 commits October 14, 2026 17:32
The iOS plan writes the one directory the shipped Package.swift reads, so
`build-binding-layer ios` takes no `--out` flag and refuses one. This
deletes the lexical path comparison that guarded the flag, which compared
two spellings of one directory, together with its test. The workflow
still hands the directory to its upload step through the `out` input.

The Swift contract step streams the standard output of xcodebuild line by
line instead of printing it after exit, so a stalled simulator shows its
last line before the job timeout kills it. The suite imports
ZingoBindings without `@testable`, because every converter it reaches is
public.

The Gradle build refuses to configure when the Kotlin contract-tests or
golden directory is missing. It counts the `@Test` methods under the
contract-tests directory and requires every unit-test task to execute
exactly that many tests, so a NO-SOURCE skip can no longer pass.

The workbench pins the Swift test count by counting XCTest methods rather
than the `func test` substring, imports `std::path` once, and drops the
manifest constant that duplicated the contract-tests directory.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
zancas and others added 3 commits October 15, 2026 16:23
The streaming step read lines through `BufRead::lines`, and its UTF-8
error closure was an exact duplicate of the one in `capture_command`,
which the dupes gate rejects. The step now reads raw lines with
`read_until` and converts each one lossily, as the buffered version did
before it, so the only error path left is a read failure and the gate
reports no exact group and the same 34 near groups as `dev`.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ollowups

# Conflicts:
#	bindings/android/build.gradle.kts
#	tools/workbench/src/binding_layer.rs

@juanky201271 juanky201271 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Android and iOS green on 42dcce9; the iOS log shows Executed 3 tests and the Gradle gate ran under testReleaseUnitTest. One optional nit inline.

Comment thread bindings/android/build.gradle.kts
@zancas
zancas merged commit 3ac96c4 into dev Oct 16, 2026
42 checks passed
@zancas
zancas deleted the fix/pr2937-review-followups branch October 16, 2026 02:43
zancas added a commit that referenced this pull request Oct 16, 2026
An issue_comment run of the publish workflow takes its YAML from the
default branch's head, while the build workflows it calls check out the
manifest entry's commit and run that commit's builder. The two agree
only when they are one commit. PR #2991's `/publish` built entry commit
7352787 under the head 3ac96c4, whose iOS workflow had dropped the
`--out` flag in PR #2970 while the older builder still required it, so
the iOS build failed and the publish job refused.

binding-manifest gains `--publish-at <ref>`. With `--check` it refuses an
entry that awaits `/publish` and names any commit but that one, naming
the head to move the entry to. With `--newest` it refuses the same way.
The manifest check passes the base branch's head on every pull request,
so the pull request learns the required re-point before anyone comments.
The publish request job passes github.sha, then checks out the entry's
commit before the naming step, so the names, the flags, and the build
all come from the one commit the run's own workflow belongs to.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants