Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Suggestion for the #defines #81

Open
paulharris opened this issue Jun 27, 2022 · 0 comments
Open

Suggestion for the #defines #81

paulharris opened this issue Jun 27, 2022 · 0 comments

Comments

@paulharris
Copy link

Hi there,

I tweaked the conan recipe for kissfft, it had been missing the extra #defines that the library users needed to include in the compile stage.

I was wondering why library users need to define these themselves?
Instead, could I suggest you auto-generate a header file that has the required defines in there, and #include that in the kiss_fft.h (or other "common" header) ?

You already generate files, such as kissfft-config.cmake.in and kissfft.pc.in
Your generated header could do extra things like double-check any already-defined constants match this library's compiled-in options - to flag potential link errors for library users.

This would help packagers like me, who (for technical reasons) have to duplicate and maintain kissfft's cmake logic into the recipe, like this:

        # got to duplicate the logic from kissfft/CMakeLists.txt
        if self.options.datatype in ["float", "double"]:
            self.cpp_info.components["libkissfft"].defines.append("kiss_fft_scalar={}".format(self.options.datatype))
        elif self.options.datatype == "int16_t":
            self.cpp_info.components["libkissfft"].defines.append("FIXED_POINT=16")
        elif self.options.datatype == "int32_t":
            self.cpp_info.components["libkissfft"].defines.append("FIXED_POINT=32")
        elif self.options.datatype == "simd":
            self.cpp_info.components["libkissfft"].defines.append("USE_SIMD")

        if self.options.use_alloca:
            self.cpp_info.components["libkissfft"].defines.append("KISS_FFT_USE_ALLOCA")

        if self.options.shared:
            self.cpp_info.components["libkissfft"].defines.append("KISS_FFT_SHARED")

This is because in the conan world, only your header and library files get used by the downstream consumer, none of your generated pkgconfig or cmake files are reused... this is so kissfft can be consumed by libraries that use bazel, meson, pkg, automake or cmake... or whatever else in the future.

If you can encode those defines in a header file, then linking to kissfft becomes much simpler and easier to maintain... include files are here, library files are there, no other knowledge is required...

It would also be good if the library was just called "kissfft.a" and not "kissfft-double.a" (would remove some extra logic required in the recipe) ... unless you are building multiple variants that could all be linked at the same time.
In the conan world, you ask for kissfft with a particular option, and you'll get it, no need to name the libraries differently. I'm sure in other worlds it is very handy, so perhaps it could be a cmake option to use the same name, or just leave it as it is now and let the recipe continue to handle the variations.

Best regards,
Paul

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

No branches or pull requests

1 participant