Skip to content

formula: add AcademySoftwareFoundation/openapv - #213

Open
MeteorsLiu wants to merge 1 commit into
xgo-dev:mainfrom
MeteorsLiu:issue/42-openapv
Open

formula: add AcademySoftwareFoundation/openapv#213
MeteorsLiu wants to merge 1 commit into
xgo-dev:mainfrom
MeteorsLiu:issue/42-openapv

Conversation

@MeteorsLiu

Copy link
Copy Markdown
Collaborator

Translate the Conan Center openapv recipe to an LLAR formula.

Closes #42

@fennoai fennoai Bot 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.

Review: openapv build recipe

This adds a well-structured llarhub build recipe for AcademySoftwareFoundation/openapv plus its versions.json. It closely follows the established strukturag/libde265 convention (cmake + license install + .pc relocation + pkg-config consumer test).

Verified correct:

  • filter (L31-43) — correctly rejects any option name other than shared/fPIC and validates each value is ON/OFF. Stricter than libde265, and sound.
  • .pc path handling — headers install to include/oapv and the static archive to lib/oapv, so includedir (L77), the static -L$${libdir}/oapv (L87), the shared -L$${libdir} (L85), and static-only -DOAPV_STATIC_DEFINE (L80-82) all match the upstream install layout.
  • 0o755/0o644 octal literals are accepted and used across sibling recipes.
  • Security/perf: embedded consumer source is a static constant (no injection), env vars scoped to the shared branch, cc!/exec! use arg-vector invocation, single-pass .pc rewrite. No concerns.

Minor / optional polish (non-blocking):

  • The .pc rewrite comment says "Keep its verified flags" but the code fully rewrites Cflags:/Libs: with hardcoded values (inline note).
  • -lm -lpthread is appended to the public Libs: for both shared and static builds; upstream keeps these in Libs.private (static-only). Harmless on shared, but could be scoped to the static branch (inline note).
  • target.options["shared"][0] indexing (L47-48, L117) is safe given defaults + filter, but most sibling recipes prefer the more defensive slices.contains(target.options["shared"], "ON") idiom.
  • -I$${prefix}/include in Cflags (L79) is redundant since $${includedir} already resolves under it.

No critical or important issues — the recipe looks correct and ready.

os.writeFile(filepath.join(licenseDir, "LICENSE"), os.readFile(filepath.join(ctx.SourceDir, "LICENSE"))!, 0o644)!

// Upstream installs oapv.pc with CMAKE_INSTALL_PREFIX-expanded paths.
// Keep its verified flags while making the installed metadata relocatable.

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.

The comment says "Keep its verified flags," but the loop below actually rewrites Cflags: and Libs: entirely with hardcoded relocatable values rather than preserving the upstream flags. Consider rewording to something like "Rewrite the paths and flags to be relocatable" so the comment matches the behavior.

libs = "-L$${libdir}/oapv -loapv"
}
if slices.contains(target.require["os"], "linux") {
libs += " -lm -lpthread"

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.

-lm -lpthread is appended to the public Libs: for both shared and static builds. These are transitive runtime deps that upstream keeps in Libs.private (needed only for static linking). Harmless for the shared case (the .so already carries them), but if you want to mirror upstream semantics you could scope this append inside the if !shared branch. Non-blocking.

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.

Translate Conan Center openapv recipe to LLAR

1 participant