feat(tinymidi): add LLAR formula - #153
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: add krgn/tinymidi formula
Overall a well-structured formula that follows the shape of its siblings (cglm / json-c / zlib): it injects a CMakeLists.txt for an upstream project that ships only a raw Makefile, pins an immutable commit SHA (stronger than a movable tag), and its filter is actually more thorough than the cglm sibling. The commit pin 3162cf8… and the cci.20130325 reference both match Conan Center's recipe.
The findings below are mostly minor/polish. The most substantive one is that onTest is effectively a compile-and-link smoke test — its runtime call's result is discarded — so it does not verify the library functions.
| const consumerSource = `#include <rawmidi.h> | ||
|
|
||
| int main(void) { | ||
| rawmidi_hw_print_info("/dev/midi"); | ||
| return 0; | ||
| } | ||
| ` |
There was a problem hiding this comment.
[P2] onTest does not verify the library — device-call result is discarded
The consumer calls rawmidi_hw_print_info("/dev/midi") but ignores its return value, and main returns 0 unconditionally. Upstream, rawmidi_hw_print_info returns -1 when it cannot open the device, which is the normal case in a CI/container sandbox where /dev/midi does not exist. As a result onTest only proves that the headers/library compile and link — the run is a no-op that always "passes."
By contrast the cglm and json-c consumers call pure library functions that produce a deterministic, host-independent result. Consider either exercising a tinymidi API that does not require a hardware device, or, if rawmidi_hw_print_info is the only reasonable entry point, treating compile+link as the real assertion and documenting that the run is best-effort.
| for value in target.require["os"] { | ||
| if value != "linux" && value != "freebsd" { | ||
| return false | ||
| } | ||
| } |
There was a problem hiding this comment.
[P2] freebsd accepted by filter, but code path is Linux/ALSA-specific
The filter accepts both linux and freebsd, but tinymidi's rawmidi.c uses Linux kernel UAPI ioctls (SNDRV_RAWMIDI_IOCTL_*) and the /dev/midi device path — Linux ALSA constructs. A FreeBSD build would likely fail to compile/link. Please confirm upstream actually builds on FreeBSD (the Conan recipe this mirrors is the source of truth); otherwise drop freebsd.
Either way, add a one-line comment explaining why the OS set is restricted — this is a load-bearing constraint (Conan's recipe validates "Only Linux and FreeBSD are supported") and, unlike the sibling formulas, it currently has no rationale.
| { | ||
| "path": "krgn/tinymidi", | ||
| "deps": {} | ||
| } |
There was a problem hiding this comment.
[P3] versions.json uses 2-space indentation; siblings use tabs
All three sibling versions.json files (recp/cglm, json-c/json-c, madler/zlib) use tab indentation; this file uses 2 spaces. Convert to tabs for repo consistency.
| } | ||
| ` | ||
|
|
||
| // Conan Center cci.20130325 pins krgn/tinymidi to this untagged commit. |
There was a problem hiding this comment.
[P3] Pin comment sits on id, not on the line that encodes the pin
This comment documents the commit/version pin, but it is attached to the id line rather than the fromVer "3162cf8…" line (line 19), which is what actually encodes the pinned commit. Consider moving it above fromVer. Reflecting the human-readable date the cci.20130325 label carries would also help, since the raw hash does not convey it.
| c.defineBool "BUILD_SHARED_LIBS", shared | ||
| c.defineBool "CMAKE_POSITION_INDEPENDENT_CODE", fPIC |
There was a problem hiding this comment.
[P3] fPIC / shared diverge from Conan contract without a note
Conan's recipe removes fPIC when shared=True (self.options.rm_safe("fPIC")), because position-independent code is implied/meaningless for a shared build. This formula passes BUILD_SHARED_LIBS and CMAKE_POSITION_INDEPENDENT_CODE independently, so shared=ON, fPIC=OFF is silently allowed — a divergence from the recipe it claims to mirror. A one-line comment noting that fPIC is irrelevant when shared=ON (as Conan enforces) would prevent confusion. The sibling json-c documents exactly this kind of shared/fPIC rationale.
| os.setenv("LD_LIBRARY_PATH", filepath.join(installDir, "lib"))! | ||
| os.setenv("DYLD_LIBRARY_PATH", filepath.join(installDir, "lib"))! |
There was a problem hiding this comment.
[P3] DYLD_LIBRARY_PATH is dead code; env vars mutated without restore
DYLD_LIBRARY_PATH is macOS-specific, but the filter restricts the OS to linux/freebsd, so this line never has any effect — consider removing it for clarity. Additionally, both env vars are set on the process and never restored; if the runner reuses the process across formulas/targets, a leaked LD_LIBRARY_PATH could influence a later step's dynamic loading. Scoping these to the child exec (per-command env) or restoring the prior values afterward would be safer. Values are derived from the trusted installDir, so there is no injection risk — this is purely about cleanliness/isolation.
1 participant
Adds a Formula for the Conan Center tinymidi recipe.
krgn/tinymidicci.201303253162cf8faff04e26a8daa846618b90326f71b9d5Local tests were intentionally not run per request.