Skip to content

feat(kissfft): add LLAR formula - #174

Open
MeteorsLiu wants to merge 1 commit into
xgo-dev:mainfrom
MeteorsLiu:issue/72-kissfft
Open

feat(kissfft): add LLAR formula#174
MeteorsLiu wants to merge 1 commit into
xgo-dev:mainfrom
MeteorsLiu:issue/72-kissfft

Conversation

@MeteorsLiu

Copy link
Copy Markdown
Collaborator

Convert the Conan Center kissfft recipe into an LLAR Formula.

  • Module: mborgerding/kissfft
  • fromVer: 131.1.0 (CMakeLists exists from this tag; v130/v131 are Makefile-only)
  • Helper: cmake
  • Keep the generated pkg-config file and rewrite prefix to ${pcfiledir}/../..
  • Consumer: Conan test_package.c

Closes #72

Convert the Conan Center kissfft recipe into an LLAR Formula covering
131.1.0 through 131.2.0. Keep the upstream pkg-config file, rewrite its
prefix to a relocatable ${pcfiledir} form, and verify the Conan
test_package consumer against the published metadata.

@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 mborgerding/kissfft recipe

Solid new recipe that closely follows the established sibling pattern (bdwgc/libatomic_ops, google/crc32c): defaults + filter + cmake + relocatable pkg-config. Metadata (id, fromVer, versions.json, directory layout) is internally consistent, and the single prefix= rewrite is correct here because upstream emits libdir/includedir relative to ${prefix} — so the simpler one-line replace (vs libatomic's three) is appropriate, not an omission. The datatype allowlist, CMake option names, and .pc naming all match upstream 131.1.0.

Security, performance, and documentation-accuracy passes found no issues. Two code-quality notes are inline.

fPIC := slices.contains(target.options["fPIC"], "ON")
openmp := slices.contains(target.options["openmp"], "ON")
useAlloca := slices.contains(target.options["use_alloca"], "ON")
datatype := target.options["datatype"][0]

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.

Multi-value options can silently mis-build. filter iterates over all values per option and only rejects when some value is invalid, so it accepts targets that pass multiple values for one option (e.g. datatype: ["float", "double"] or shared: ["ON", "OFF"]). But onBuild collapses those inconsistently: the booleans (lines 56-59) use slices.contains(..., "ON") (ON wins), while datatype here uses [0] (first value wins, rest discarded). A target requesting two datatypes would pass filter, build only the first, and produce a package that doesn't match the requested options.

Simplest fix: reject multi-value options in filter, e.g. if values.len != 1 { return false } inside the loop, since each of these options is logically single-valued. That also removes the mixed slices.contains vs [0] idiom.

consumer := filepath.join(testDir, "consumer.c")
os.writeFile(consumer, []byte(consumerSource), 0o644)!

pcName := "kissfft-" + target.options["datatype"][0]

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.

Minor: the pcName derivation ("kissfft-" + datatype plus optional -openmp) is duplicated here from onBuild (lines 81-84), and the two copies read the options differently (onBuild uses the local datatype/openmp vars; here it re-reads target.options[...] inline). They can drift if the upstream naming scheme changes. Likely unavoidable across the build/test handler boundary, but worth keeping the two sites in lock-step.

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

1 participant