feat(poly2tri): add LLAR formula - #157
Conversation
f462b16 to
9babaa0
Compare
9babaa0 to
1666cc4
Compare
There was a problem hiding this comment.
Review: add poly2tri LLAR formula + zlib API migration
Reviewed the full diff (4 files) with quality, performance, security, and documentation passes. The formula cleanly adopts the newer single-arg API (onBuild ctx =>, ctx.outputDir property, ctx.setMetadata) and the new defaults/filter/onTest/target DSL, and adds a real compile-and-link consumer test — nice. No security issues found (all paths derive from trusted ctx; the pinned 40-char commit SHA is the correct immutable pin, and upstream ships a top-level LICENSE so line 62 won't panic).
The main thing to verify before merge is the source/header path layout — see the inline comment on CMakeLists.txt. Upstream nests all sources and headers under a top-level poly2tri/ subdirectory, but the generated CMakeLists references them relative to the checkout root.
Other, lower-priority items (no reliable single inline line):
- SKILL.md is now stale. This PR migrates
Zlib_llar.goxto the new API too, but.claude/skills/write-formula/SKILL.mdstill documents the old three-argonBuild (ctx, proj, out) =>signature,ctx.outputDir()returning(string, error), and theout.setMetadata/out.addErr/BuildResultmodel — all of which no longer apply. The newdefaults,filter,onTest,target.options,target.require, andctx.Projare undocumented. Consider updating the skill doc (or a follow-up) so contributors don't write formulas against the removed API. - Filename casing inconsistency. The sibling formula is
Zlib_llar.gox(capital Z) while this one ispoly2tri_llar.gox(lowercase). SKILL.md's layout uses lowercase{name}. Pre-existing, but worth normalizing since this PR already touches the zlib file. - Build optimization. No
CMAKE_BUILD_TYPEis set, solibpoly2triis built without optimization. For a triangulation library consumers run in hot paths, considerc.buildType "Release".
Note on error handling: the !-panic style used throughout onBuild/onTest is fine — cmake.Configure/Build/Install panic internally on failure in the current llar API, so the command-form c.configure/c.build/c.install calls do surface errors rather than swallow them.
| ${POLY2TRI_SRC_DIR}/sweep/cdt.cc | ||
| ${POLY2TRI_SRC_DIR}/sweep/sweep.cc | ||
| ${POLY2TRI_SRC_DIR}/sweep/sweep_context.cc | ||
| ) |
There was a problem hiding this comment.
Verify the source/header paths against upstream's layout. At this pinned commit, greenm01/poly2tri keeps all sources and headers under a top-level poly2tri/ subdirectory (poly2tri/common/shapes.cc, poly2tri/sweep/cdt.cc, poly2tri/poly2tri.h, ...). POLY2TRI_SRC_DIR is set to ctx.SourceDir (the checkout root) in the formula, so ${POLY2TRI_SRC_DIR}/common/shapes.cc resolves to <root>/common/shapes.cc, which doesn't exist — the paths look like they're missing the poly2tri/ prefix (e.g. ${POLY2TRI_SRC_DIR}/poly2tri/common/shapes.cc).
Same concern for the header install below: install(DIRECTORY ${POLY2TRI_SRC_DIR} ...) (no trailing slash) installs the checkout-root directory itself under include/, producing something like include/<checkout-name>/poly2tri/poly2tri.h, whereas the consumer test does #include <poly2tri/poly2tri.h> with only -Iinclude. Please confirm onTest actually passes; if it does, ctx.SourceDir must already point at the poly2tri/ subdir — otherwise both the source list and the include layout need the poly2tri/ prefix.
| c.defineBool "BUILD_SHARED_LIBS", shared | ||
| c.defineBool "CMAKE_POSITION_INDEPENDENT_CODE", fPIC | ||
| c.configure | ||
| c.build |
There was a problem hiding this comment.
cmake Build doesn't add --parallel/-j by default (confirmed in llar's x/cmake), and no generator is set so it falls back to Make. The 5 independent translation units compile serially. Consider c.build "--parallel" for a faster build on multi-core runners.
| onBuild ctx => { | ||
| installDir := ctx.outputDir | ||
|
|
||
| cmakeLists := ctx.Proj.readFile("88de49021b6d9bef6faa1bc94ceb3fbd85c3c204/CMakeLists.txt")! |
There was a problem hiding this comment.
Minor: the commit SHA 88de49... is hardcoded in this readFile path and also in fromVer (line 22) / the directory name. If the pin ever changes, this path must be updated in lockstep. Consider deriving it from the version to avoid drift.
| "-L" + filepath.join(installDir, "lib"), | ||
| "-lpoly2tri", | ||
| } | ||
| if slices.contains(target.require["os"], "linux") || slices.contains(target.require["os"], "freebsd") { |
There was a problem hiding this comment.
Minor: the -lm gating (slices.contains(target.require["os"], "linux") || ... "freebsd") and the base link flags are duplicated almost verbatim in onTest (lines 92-94). Extracting a small helper would keep the two paths in sync if the flag set changes.
Translate the Conan Center poly2tri recipe from snapshot ffe30df101afd4dc95aac2f14b25bf345e64d7be into an LLAR Formula.
Closes #81