Fix and improve jemalloc releated cmake

[ur4t]

1. Fix JEMALLOC_PREFIX when --with-jemalloc-prefix= is absent.
2. Use pkg-config to find jemalloc if available.

PS: Actually jemalloc releated cmake can be further simplified, current modifications are minimized to keep consistent with the existing approach.

Releated CMake documentation: https://cmake.org/cmake/help/latest/module/FindPkgConfig.html#command:pkg_check_modules

[ygoldfeld]

Thank you! I was on paternity leave - hence the delayed response - sorry about that. I need a few days to orient myself, but I shall surely review and accept this soon. And, it won't take 4 months next time... so hopefully people don't get the wrong idea.

Furthermore - someone in my org found the issue with empty prefix... and I was just coming here to fix that up and so on. You already did though which is quite delightful.

[ygoldfeld]

There are a few related things going on possibly outside the issues you were trying to (and succeeding to) fix/improve, so I might make a new PR after looking into everything. I omit details here to avoid extra confusion. Anyway I shall look into that soon but probably not right this second.

Even before then, let me put some comments/questions here, before I forget. Your answers (if any) will serve to at least educate me. (Undoubtedly I am a CMake noob; in fact I learned it, to the extent that I did, for this project.)

--

• This looks delightful -- but shouldn't this be within if(NOT JEMALLOC_LIB)? To the extent that this is better than find_library() and all that, perhaps it should be attempted a bit below, and if it fails then go on to my find_library() stuff?
• I am also curious; and again this is most likely just for my own education; what moved you to make this change? Was it a general desire to improve this, or did you observe (or know of a use case where one would observe) the find_library() stuff failing, where pkg-config would have succeeded? If it's only a general desire to improve this, that doesn't mean we shouldn't; I just want to know the reason is all.

[ur4t]

Shouldn't this be within if(NOT JEMALLOC_LIB)?

Agree. This shoubld be within if(NOT JEMALLOC_LIB).

Personally I perfer the following logic:

1. Use user specified elements (but via other variable names, explanation later).
2. If available, use find_package() to find corresponding elements.
3. Else, use find_package(PkgConfig) and the following pkg_check_modules() (which drives pkgconf or old pkg-config) to find corresponding elements.
4. (Not recommended) manually call find_library() and find_path() and set corresponding elements.

Usually find_package() is the approach recommended by CMake, but regrettably jemalloc does not provided files used by find_package(). jemalloc.pc is provided and much more cleaner than using find_library() and find_path() directly.

[ygoldfeld]

Should these lines set them as CACHE variables? I ask, because find_library() and find_path() do so (unless one sets NO_CACHE, which I did not).

[ur4t]

PS: Actually jemalloc releated cmake can be further simplified, current modifications are minimized to keep consistent with the existing approach.

I am not sure, they are just aliases to avoid more modifications.

Personally I recommend use JEMALLOC_LIBRARIES and JEMALLOC_INCLUDEDIR directly rather than shimming via JEMALLOC_LIB and JEMALLOC_INCLUDE_PATH:

https://cmake.org/cmake/help/latest/module/FindPkgConfig.html#command:pkg_check_modules says that <XXX>_LIBRARIES and <XXX>_INCLUDE_DIRS are set by pkg_check_modules(). I am not quite sure whether find_package() use these names too or not, but as far as I remember, manually specifying other libraries usually requires -D <LIBNAME>_LIBRARIES=PATH_TO_LIB_DIR.

[ygoldfeld]

Is this necessary? If they specified this prefix manually for some unholy reason (and we do advertise this), maybe we should let them? (I have the vague feeling CMake's philosophy on cache variables isn't totally clear... at least I couldn't glean it when I tried... like this value could be a wrongly-computed value from a previous run, in which case who knows what we should do - but presumably worst-case user can just clear cache and start over....)

[ygoldfeld]

I meant just the unset() line there... not the 3 preceding that. Apologies for being confusing.

[ur4t]

https://cmake.org/cmake/help/latest/module/FindPkgConfig.html#command:pkg_check_modules says that a <YYY>_PREFIX is set by pkg_check_modules(), and I was trying to avoid potential conflict. (I mistakenly thought JEMALLOC_PREFIX is only set by the following jemalloc-config utilities and not supposed to be specified by users.)

[ygoldfeld]

Okay, got it. There's quite a bit going on. I can surely improve this based on stuff you've taught me (plus subsequent experiments). Like I don't need to introduce more cache variables if some would already be set with the pkg... stuff... and clearly I should differentiate in naming between prefix as in path prefix, versus prefix as in the API name prefix (which is super duper important here). And probably more boring subtleties like that.

The time to fix it up is now, before (god willing) all kinds of users start using mr Flow-IPC.

So, not ready to go yet, but I'm optimistic. 👍🏻

[ur4t]

A different name like JEMALLOC_API_PREFIX is inconsistent with upstream jemalloc's convention, which might be confusing.

I think some "save/restore" actions might help here, with proper explaining comments: Save user specified JEMALLOC_PREFIX to another variable temporarily and restore JEMALLOC_PREFIX with that saver variable after pkg_check_modules() stuffs.

Be bold and do it! Changes are never too late. 👍🏻👍🏻👍🏻

[ygoldfeld]

Thanks for this... it seems like clearly an improvement/defensive in nature, at the very least.

Again for my own education I ask how you came across the desire to do this. Is it that if one builds jemalloc without explicitly setting this prefix thing to blank when configuring it, then the jemalloc-config output will omit --with-jemalloc-prefix= entirely?

(That may or may not be related to a certain problem someone found when setting the prefix to . But don't worry about that... my thing to track down. Bottom line is you may have fixed here the problem they found-- I am just not sure yet.)

[ur4t]

Is it that if one builds jemalloc without explicitly setting this prefix thing to blank when configuring it, then the jemalloc-config output will omit --with-jemalloc-prefix= entirely?

Yes!

Distros using RPM (Fedora and OpenSUSE) ship jemalloc-devel with jemalloc-config script, whose output contains no --with-jemalloc-prefix.

Distros using DEB (Debian and Ubuntu) ship libjemalloc-dev without jemalloc-config script at all.

On Debian, I built jemalloc myself (without setting any configuration options) to provided jemalloc-config and jemalloc-config --config outputs a blank line.

Actually jemalloc-config --config outputs arguments provided to ./configure ... as is. And usually the JEMALLOC_PERFIX is not set to utilize LD_PRELOAD functionality.

[ygoldfeld]

Note to self, before I file an issue and go to town finalizing all this - before I forget -

The pkg-config change suggested above might also resolve this in the GitHub build pipeline elsewhere in the code, namely this in ipc/conanfile.py:

            # TODO: We're not doing anything wrong here; we tell jemalloc itself to be built with this
            # API-name prefix via options.jemalloc.prefix, and then we tell Flow-IPC CMake script(s) what that was via
            # JEMALLOC_PREFIX CMake variable (as if via `-DJEMALLOC_PREFIX=je_` to `cmake`).  That said
            # Flow-IPC CMake script(s) can figure this out by itself; if JEMALLOC_PREFIX is not given, then it
            # it finds jemalloc-config binary, which a normal jemalloc install would put into (install-prefix)/bin,
            # and uses it to print the prefix.  However commenting out the next line does not work for some reason:
            # an error results saying jemalloc-config cannot be found, and that we should either provide path
            # to that binary via yet another CMake cache setting; or simply supply the prefix as JEMALLOC_PREFIX.
            # So this approach is fine; just it would be nice if the Conan magic worked in a more understandable way;
            # if Flow-IPC CMake script(s) can find libjemalloc.a and headers, why can't it
            # find_program(jemalloc-config)?  This slightly suggests something is "off" possibly.
            # Still, the bottom line is it works, so this fallback is fine too.  One could say it'd be nice to
            # test Flow-IPC CMake script(s) smartness in the way that would be more likely used by the user;
            # but one could say that is splitting hairs too.
            toolchain.variables["JEMALLOC_PREFIX"] = self.options["jemalloc"].prefix

So this should be removed, if indeed it does.

[ur4t]

It's glad to hear from you and share the joy with you.

I will try to retrieve some contexts, but it has been quite a long time, thus I am not sure if I was thinking in the same way as I am.