Skip to content

feat(cqrlib): add CQRlib formula - #152

Open
MeteorsLiu wants to merge 3 commits into
mainfrom
issue/86-cqrlib
Open

feat(cqrlib): add CQRlib formula#152
MeteorsLiu wants to merge 3 commits into
mainfrom
issue/86-cqrlib

Conversation

@MeteorsLiu

Copy link
Copy Markdown
Collaborator

Summary

  • add the yayahjb/cqrlib Formula from the Conan Center recipe
  • build and install CQRlib with CMake, including shared/fPIC options and LGPL license
  • verify C and C++ consumer programs against the installed library

Closes #86

Validation

  • git diff --check
  • Local Formula/CI tests 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 yayahjb/cqrlib recipe

Solid recipe that closely follows the recp/cglm pattern (shared/fPIC options, whitelist filter, onBuild/onTest, conditional -lm). The consumer sources are valid against the CQRlib-1.0.4 API, onTest correctly derives paths from the install dir so it works on a cache hit, and the fromVer comment is accurate. Two items worth addressing plus a couple of minor notes below.

Checked and fine:

  • filter (lines 35–47): correct whitelist — rejects only unknown option names / non-ON/OFF values; no premature return false.
  • -lm gating to linux/freebsd is consistent with the CMakeLists' own conditional find_library(M_LIB m).
  • versions.json matches the shape of the other recipes.

Minor (non-blocking):

  • Relative to recp/cglm and json-c, this recipe is more complex (it injects its own CMakeLists.txt, supports two options, builds C and C++ consumers) yet carries almost no explanatory comments. Consider documenting why a repo-local CMakeLists.txt replaces the upstream build, and why the defaults (shared=OFF, fPIC=ON) were chosen — matching the comment density the sibling recipes establish.
  • WINDOWS_EXPORT_ALL_SYMBOLS (CMakeLists:6) only affects DLL builds, so it is a no-op for the default shared=OFF config; a one-line comment noting it applies only to the shared Windows build would help.

lastErr!

if slices.contains(target.options["shared"], "ON") {
os.setenv("LD_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.

[P2] LD_LIBRARY_PATH/DYLD_LIBRARY_PATH overwritten instead of prepended

LD_LIBRARY_PATH and DYLD_LIBRARY_PATH are set to a single path, discarding any inherited value — unlike the Windows branch just below (line 123) which correctly reads and prepends to PATH. If the test binary depends on other shared libraries reachable only via an inherited loader path, the test could fail spuriously. Recommend reading the existing value and prepending, e.g. join installDir/lib with os.getenv("LD_LIBRARY_PATH") using :, for parity with the PATH handling. (Minor; the written value is trusted here.)

@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 (follow-up): lib/lib64 path finding

Adding the one finding that was dropped from my prior review due to a title-length limit — it's the most impactful item.

include(GNUInstallDirs)
install(TARGETS CQRlib
RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR}
LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR}

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.

[P1] Hardcoded lib breaks where GNUInstallDirs uses lib64

The bundled CMakeLists.txt installs the library to ${CMAKE_INSTALL_LIBDIR} via GNUInstallDirs. On many 64-bit distros (Fedora, RHEL, openSUSE) CMAKE_INSTALL_LIBDIR resolves to lib64, but the recipe hardcodes filepath.join(installDir, "lib") for the metadata -L flag, both consumer -L flags (lines 72, 92, 108), and the LD_LIBRARY_PATH/DYLD_LIBRARY_PATH exports (lines 119–120). On such a platform the install lands in lib64 while all flags point at lib, so the emitted metadata is wrong and the consumer link/run fails.

Recommend mirroring the json-c recipe, which pins the layout with c.define "CMAKE_INSTALL_LIBDIR", "lib" so the hardcoded lib assumption holds across platforms.

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

1 participant