chore(#756): confirm OCCT#1418's regression provenance and stage its Draw test - #758
Conversation
…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
left a comment
There was a problem hiding this comment.
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 | ||
|
|
There was a problem hiding this comment.
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 vdegIf 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" — unhandledSo 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.
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-*.patchis unchanged. This PR touches onlyprovenance 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 everyworkspace offset in
mma2ce1_down by one position, and every other call site in that diff moveswith it. The U-direction
mma2jmx_call is the one exception: it keeps its old offset name insteadof 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, andV7_6_0, 2021-11-01,the first release to carry the commit) shows
V7_5_0with two distinct slots andV7_6_0alreadycollapsed 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=1against the two tags and the commit); this project's ownLibraries/occt-srcdoes 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. Thediscriminating measurement instead reused
Scripts/repro/522-approx-c0-collapse/occt_522_c0_minimal.mmwith the exact
GeomConvert_ApproxSurfaceconstructor callapproxsurfmakes (confirmed by readingGeomliteTest_SurfaceCommands.cxx, including that its 9-argument form leavesPrecisCodeat itsdefault of 1, not the 0 this directory's existing probes used):
Checksum-verified against
Package.swift's pinnedOCCT.xcframework.zipfor the "after" run; the"before" run overrides just
AdvApp2Var_ApproxF2var.o(compiled from the unmodifiedV8_0_1tagsource) 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.
dumpjsonis not a registered Draw command anywhere in the tree.And
Standard_Dump::DumpFieldToNamestrips themyprefix frommyUDeg/myVDegbut does notchange case, so the real
DumpJsonkeys are"UDeg"/"VDeg", not"udeg"/"vdeg". Eitherproblem alone leaves the Tcl variable unset. The staged test instead captures
dump r's existingtextual output through
dlogand reads theDegrees :lineGeomTools_SurfaceSet::PrintSurfacewrites, the same idiom
tests/bugs/modalg_7/bug23942already uses for the same purpose.What changed
Scripts/repro/522-approx-c0-collapse/upstream/tests/bugs/moddata_3/bug1418: the completed Drawtest, 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#1418description, 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: the0019section gains a provenance sentence.CLAUDE.md: the#522Known OCCT Bugs entry gains one inserted sentence (surrounding textunchanged).
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) andprepares artifacts for an external repository (
Open-Cascade-SAS/OCCT) that are not pushed orposted from this PR.
SemVer impact
NONE. No OCCTSwift source, header, or public API changes at all.
Checklist
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".--self-testcase was run once with its subject broken, and thefailure is reported here, see okf/policies/prove-the-test-fails.md.
No gate script was touched, so no
--self-testcase is new. The new Draw test's assertion wasproven to fail on the unpatched mechanism and pass on the patched one via the override-link
measurement above (
udeg=1fails!= 7,udeg=7passes), since the Tcl script itselfcannot be executed here.
docs/CHANGELOG.mdis not in this diff.docs/SEMVER.mdis 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
gsdali/OCCT, nothingwas posted to OCCT#1418, and this PR is not self-merged.
Scripts/patches/0019-*.patchis unchanged and out of scope, per Address the review on OCCT#1418: record the 2021 regression provenance and add the Draw test #756.--self-tests (and the census's and the changelog-audit's,matching what
ci.yml'sgate-scriptsjob runs) pass clean on this branch; none needed changes,since nothing here touches bridge code, headers, or Swift tests.
fix); this PR is neither, and
docs/SEMVER.mdgenuinely is not in the diff, consistent with thepolicy for an ordinary PR.