Skip to content

feat(libipt): add LLAR formula - #149

Open
MeteorsLiu wants to merge 1 commit into
xgo-dev:mainfrom
MeteorsLiu:issue/89-libipt
Open

feat(libipt): add LLAR formula#149
MeteorsLiu wants to merge 1 commit into
xgo-dev:mainfrom
MeteorsLiu:issue/89-libipt

Conversation

@MeteorsLiu

Copy link
Copy Markdown
Collaborator

Closes #89

  • Adds an intel/libipt Formula using the upstream CMake build and consumer check.
  • Supports Conan recipe shared and fPIC options, installs LICENSE, and publishes -lipt metadata.
  • Uses v2.0.1 as the lowest compatible tag because earlier releases hard-code a shared target.

Tests were not run per request.

@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: add intel/libipt Formula (v2.0.1)

The recipe is well-structured and follows the LLAR Formula contract: id/fromVer, defaults, filter, onBuild, onTest. versions.json matches the sibling format. onTest correctly re-derives paths from installDir (ctx.outputDir) so it survives a cache hit where onBuild is skipped. Nice.

I checked several things that turned out to be fine, noting them so they aren't re-raised:

  • -lipt is correct. Upstream libipt/CMakeLists.txt names the target libipt with PREFIX "", so the installed artifact is libipt.so/libipt.a. -lipt makes the linker search libipt.* → matches. (-llibipt would search liblibipt.* and fail.)
  • The lowercase filename libipt_llar.gox is correct. The repo's write-formula skill mandates a lowercase filename stem ("Do not capitalize it"). The capitalized siblings (Cglm_llar.gox, etc.) predate/diverge from that rule.
  • No pkg-config path is expected. libipt v2.0.1 installs no .pc file, so hand-built -I/-L/-l flags are the correct choice here.

One low-severity robustness note is inline. A couple of optional polish items:

  • Comments (optional): Unlike the cglm/json-c siblings, this recipe has no explanatory comments (build contract, shared/fPICBUILD_SHARED_LIBS/CMAKE_POSITION_INDEPENDENT_CODE, ON/OFF values). A short header would aid maintenance, though zlib is also comment-free so this isn't universal.
  • consumer.cpp compiled with c++: The consumer uses only the C API; compiling it as C++ works but unnecessarily requires a C++ toolchain. cc + .c would match the cglm sibling.

No blocking issues found.


if slices.contains(target.options["shared"], "ON") {
os.setenv("LD_LIBRARY_PATH", filepath.join(installDir, "lib"))!
os.setenv("DYLD_LIBRARY_PATH", filepath.join(installDir, "lib"))!

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.

os.setenv overwrites LD_LIBRARY_PATH/DYLD_LIBRARY_PATH rather than prepending to any existing value. If the test environment already relies on these paths (e.g. for the C++ runtime or another dependency), overwriting them could break loader resolution for the consumer binary. Prepending the install lib dir to the existing value is the safer idiom, e.g. os.getenv("LD_LIBRARY_PATH") joined with :. Low severity, but worth considering.

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 libipt recipe to LLAR

1 participant