anigmetov / hera

Other
4 stars 3 forks source link

Please support system spdlog and fmt (and newer fmt), or allow disabling this functionality #1

Closed gspr closed 4 years ago

gspr commented 4 years ago

Hello,

I don't know whether the Github or the Gitlab repository is canonical, and I couldn't find your email address, so I just made a guess to file an issue here.

Now that Hera is a dependency of GUDHI, I need to package Hera for Debian in order to upgrade the GUDHI Debian package to 3.2.x. I'm having some issues preparing a Hera Debian package because of the bundled copies of the spdlog and fmt libraries. Debian abhors and disallows bundled copies of libraries. I am therefore faced with needing to strip out these from Hera, and change the Hera code to allow the use of system-wide spdlog/fmt. While I will probably manage do this, the change is quite invasive and would incur a future divergence between the Debian package and the official upstream Hera. This becomes maintenance debt for the future.

I would therefore ask you if you could please consider making it possible for (through macros or CMake magic) Hera to use of system-wide spdlog and fmt libraries. In the case of fmt, it also seems that the bundled copy is quite ancient (version 4 series?), while the fmt library is now at an incompatible version 6 series upstream.

Alternatively, if it's easier, it would help a lot if there could be an option to simply disable the use of spdlog/fmt altogether. (I'm maybe missing something, but I'm a bit confused about the role of mandatory logging in Hera in the first place…)

Thank you for considering this.

anigmetov commented 4 years ago

Hello, I'll take a look at it (I hope very soon); for now I think the easiest option is the last thing you suggested, disabling the logging by default. I have no idea about the gitlab repo; for me, the two canonical repos are this one and the bitbucket repo https://bitbucket.org/grey_narn/hera/src/master/

Arnur Nigmetov

gspr commented 4 years ago

grey-narn notifications@github.com writes:

Hello, I'll take a look at it (I hope very soon); for now I think the easiest option is the last thing you suggested, disabling the logging by default.

Great, thanks!

Could you elaborate a bit about whether I could just rip out all logging in general? Is there a reason I'm missing for mandatory logging within Hera?

I have no idea about the gitlab repo; for me, the two canonical repos are this one and the bitbucket repo https://bitbucket.org/grey_narn/hera/src/master/

Sorry, that was a brainfart on my part, I meant Bitbucket when I said Gitlab :-)

anigmetov commented 4 years ago

From what I see now, all logging can be removed completely. I think it is present only in the matching distance calculation, and is useless to anyone but me. Moreover, if I am not mistaken, GUDHI does not use the matching distance at all, they are interested in Wasserstein and bottleneck only, so the easiest thing is to eliminate the whole matching directory.

gspr commented 4 years ago

grey-narn notifications@github.com writes:

From what I see now, all logging can be removed completely.

Thanks for the info! Maybe you could preprocessor-protect all the logging in the future?

I think it is present only in the matching distance calculation,

Hmm, doesn't look like it:

$ grep -ir spdlog wasserstein/ | wc -l
523

Did I misunderstand something?

gspr commented 4 years ago

Oh by the way, and this is a bit off-topic, but just so I don't make any wild assumptions: is the fmt library used only for formatting logs, or also in writing/reading persistence diagrams?

anigmetov commented 4 years ago

I removed spdlog and fmt from Hera entirely, getting rid of lots of debug code.

gspr commented 4 years ago

grey-narn notifications@github.com writes:

I removed spdlog and fmt from Hera entirely, getting rid of lots of debug code.

Sounds great – thanks!