Skip to content

chore(#756): confirm OCCT#1418's regression provenance and stage its Draw test - #758

Merged
gsdali merged 1 commit into
refactor/381-pass1bfrom
chore/756-occt1418-review
Aug 7, 2026
Merged

chore(#756): confirm OCCT#1418's regression provenance and stage its Draw test#758
gsdali merged 1 commit into
refactor/381-pass1bfrom
chore/756-occt1418-review

Conversation

@gsdali

@gsdali gsdali commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator

What & why

Maintainer gkv311 reviewed our upstream filing of patch 0019
(OCCT#1418) and asked for two things: confirm
that the defect is a regression introduced by a specific 2021 commit (including its 7.5.x behaviour,
by measurement, not assumption), and finish the Draw regression test he sketched with the assertion
block commented out. This PR does both, independently confirming rather than restating his review,
and prepares (without pushing or commenting) the revised PR description, the reply, and the test.

The one-character fix in Scripts/patches/0019-*.patch is unchanged. This PR touches only
provenance documentation and staged upstream artifacts.

Closes #756

What was found

Provenance, confirmed at the release-tag level, not just the commit diff.
3016a390713d2e893f4bfa797882b9f0266840e1 (2021, a UBSan coding-rules cleanup) renumbers every
workspace offset in mma2ce1_ down by one position, and every other call site in that diff moves
with it. The U-direction mma2jmx_ call is the one exception: it keeps its old offset name instead
of moving, so both the U and V calls end up writing to the slot the V call already owns. Reading the
two release tags either side of the commit directly (V7_5_0, 2020-11-02, and V7_6_0, 2021-11-01,
the first release to carry the commit) shows V7_5_0 with two distinct slots and V7_6_0 already
collapsed to one, so OCCT 7.5.x computed this correctly and the regression is confirmed at the
shipped 7.6.0 release. Verified against a disposable shallow clone of Open-Cascade-SAS/OCCT
(git fetch --depth=1 against the two tags and the commit); this project's own Libraries/occt-src
does not exist in this isolated worktree and was not touched.

The reviewer's guessed degree numbers (7/7) are correct, measured rather than assumed. This
project only builds a static library, not DRAWEXE, so the Draw script itself was not run. The
discriminating measurement instead reused Scripts/repro/522-approx-c0-collapse/occt_522_c0_minimal.mm
with the exact GeomConvert_ApproxSurface constructor call approxsurf makes (confirmed by reading
GeomliteTest_SurfaceCommands.cxx, including that its 9-argument form leaves PrecisCode at its
default of 1, not the 0 this directory's existing probes used):

before (stock, override-linked against the pinned V8_0_1 source):  udeg=1  vdeg=7
after  (the pinned v2.0.0-kernel.1 release, patch 0019 applied):   udeg=7  vdeg=7

Checksum-verified against Package.swift's pinned OCCT.xcframework.zip for the "after" run; the
"before" run overrides just AdvApp2Var_ApproxF2var.o (compiled from the unmodified V8_0_1 tag
source) ahead of that same archive, isolating the one line the patch changes.

The reviewer's commented-out assertion block does not run as written, for two independent reasons
unrelated to the degree values.
dumpjson is not a registered Draw command anywhere in the tree.
And Standard_Dump::DumpFieldToName strips the my prefix from myUDeg/myVDeg but does not
change case, so the real DumpJson keys are "UDeg"/"VDeg", not "udeg"/"vdeg". Either
problem alone leaves the Tcl variable unset. The staged test instead captures dump r's existing
textual output through dlog and reads the Degrees : line GeomTools_SurfaceSet::PrintSurface
writes, the same idiom tests/bugs/modalg_7/bug23942 already uses for the same purpose.

What changed

  • Scripts/repro/522-approx-c0-collapse/upstream/tests/bugs/moddata_3/bug1418: the completed Draw
    test, staged where it would live upstream (not added to Libraries/occt-src).
  • Scripts/repro/522-approx-c0-collapse/upstream/pr-1418-description.md: the rewritten OCCT#1418
    description, leading with the missed decrement per the review.
  • Scripts/repro/522-approx-c0-collapse/upstream/reply-to-gkv311.md: the drafted reply.
  • Scripts/repro/522-approx-c0-collapse/README.md: new "Upstream provenance (Address the review on OCCT#1418: record the 2021 regression provenance and add the Draw test #756)" section.
  • Scripts/patches/README.md: the 0019 section gains a provenance sentence.
  • CLAUDE.md: the #522 Known OCCT Bugs entry gains one inserted sentence (surrounding text
    unchanged).

CHANGELOG entry

None. This PR does not change any OCCTSwift public API, bridge behavior, or shipped artifact. It
records provenance for an already-carried kernel patch (0019, shipped since the patch landed) and
prepares artifacts for an external repository (Open-Cascade-SAS/OCCT) that are not pushed or
posted from this PR.

SemVer impact

NONE. No OCCTSwift source, header, or public API changes at all.

Checklist

  • New or changed behavior is covered by a unit test in the same PR (not just manual
    verification), see SecondMouseAU/OCCTReconstruct#397
    for the ecosystem-wide test-coverage standard this is piloting.
    This PR changes no OCCTSwift behavior; the "test" here is the staged upstream OCCT Draw
    case, which cannot run in this repo's CI (no DRAWEXE). See "Notes for the reviewer".
  • Every new test and every new --self-test case was run once with its subject broken, and the
    failure is reported here, see okf/policies/prove-the-test-fails.md.
    No gate script was touched, so no --self-test case is new. The new Draw test's assertion was
    proven to fail on the unpatched mechanism and pass on the patched one via the override-link
    measurement above (udeg=1 fails != 7, udeg=7 passes), since the Tcl script itself
    cannot be executed here.
  • The CHANGELOG entry above is complete, and docs/CHANGELOG.md is not in this diff.
  • The SemVer impact above is stated, and docs/SEMVER.md is not in this diff.
    It is assessed at release on main, not per PR.
    Tick this for a release commit or a PR that fixes the CHANGELOG itself too: those are the
    policy's two exceptions and the file is expected in their diff. Say which one applies in
    "Notes for the reviewer".

Notes for the reviewer

  • Neither hard constraint from the issue was crossed: nothing was pushed to gsdali/OCCT, nothing
    was posted to OCCT#1418, and this PR is not self-merged.
  • The one-character fix in Scripts/patches/0019-*.patch is unchanged and out of scope, per Address the review on OCCT#1418: record the 2021 regression provenance and add the Draw test #756.
  • All five gate scripts plus their four --self-tests (and the census's and the changelog-audit's,
    matching what ci.yml's gate-scripts job runs) pass clean on this branch; none needed changes,
    since nothing here touches bridge code, headers, or Swift tests.
  • The "SemVer impact" checkbox item's own wording is about the exceptions (release commit / CHANGELOG
    fix); this PR is neither, and docs/SEMVER.md genuinely is not in the diff, consistent with the
    policy for an ordinary PR.

…Draw test

Independently confirms gkv311's review on OCCT#1418 (3016a390 missed one of two
adjacent offset decrements) at the release-tag level (V7_5_0 correct, V7_6_0
already broken), and measures rather than assumes his script's guessed 7/7
degree numbers via override-link against the pinned kernel. Stages the
completed Draw test, the rewritten PR description, and the drafted reply,
without touching the one-character fix or Libraries/occt-src.

@secondmouseAU-bot secondmouseAU-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Automated review (medium effort): 1 finding on the staged upstream Draw test — see inline comment. No issues found elsewhere in this diff (provenance docs, patch README updates, prepared-but-unpushed upstream artifacts).

dlog reset
dlog off
decho on

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Unverified/unguarded regex against dump r output — the PR body says this script was never actually run against DRAWEXE.

regexp {Degrees :([0-9]+) +([0-9]+)} ${info} full udeg vdeg

If GeomTools_SurfaceSet::PrintSurface's real label/spacing doesn't match this assumed "Degrees :" format exactly, regexp returns 0 and udeg/vdeg are never bound. The very next line then Tcl-errors instead of failing cleanly:

puts "udeg = ${udeg}"   // "no such variable" — unhandled

So instead of the intended "Error : wrong U degree" diagnostic, the test would abort ungracefully. Worth confirming the exact dump/PrintSurface text (or running it once against a real DRAWEXE build) before this is forwarded upstream to OCCT's tests/ tree.

@gsdali
gsdali merged commit 7c8b694 into refactor/381-pass1b Aug 7, 2026
5 checks passed
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