Add spdlog as ups product
Dear SciSoft Team,
This is Haiwang Yu from BNL WireCell team.
Our recent development needs a package called 'spdlog'.
The latest spdlog release, v1.5.0 should be good to use.
This logger was previously internalized in wire-cell toolkit.
Now we have more packages using it and need it to be an external package.
Could you add it as a ups product?
As before, we could help making the building shims if needed.
#3 Updated by Haiwang Yu 11 months ago
Hi Lynn, I made some scripts in the repository.
Tested building on SL7 for e19 and c7 qualifiers.
Then tested building wire-cell against those local spdlog ups products.
Things were working good for me.
Please have a look to see if these building scripts are good to use.
#6 Updated by Lynn Garren 11 months ago
We have some concerns. The code in spdlog is c++, and we see that a library is created: libspdlog.a
First, is there an option to build a dynamic .so library?
Second, since this is c++ code which will presumably be linked with other c++ code, the compilation options need to be the same as our standard build options. That means using -std=c++17 with e19 and c7.
#7 Updated by Brett Viren 11 months ago
I guess three things to relate:
0) I don't see it mentioned yet but I guess there's one more issue: spdlog does not build with -fPIC by default. It is needed in our builds and can be turned on by adding this to cmake's command line: -DCMAKE_POSITION_INDEPENDENT_CODE=ON Missing this won't become apparent until linking code using spdlog.
1) We don't explicitly turn on -std=c++17 when building spdlog but we do build other code with this standard explicitly turned on. Either we got lucky or maybe C++17 is enabled by default when we build on Ubuntu or something else is going on. I guess it should be fine to explicitly turn on -std=c++17 when building spdlog to a compiled library.
2) spdlog is really a "header only" library. Producing the compiled library is essentially just used to reduce build times. We could dispense with creating the compiled library at the cost of somewhat longer build times for any file that #include's spdlog headers (directly or indirectly). For now, I think this would only affect building "wirecell" UPS product and maybe also "larwirecell". So, maybe not so tragic.
The project gives this explanation on why a static library is built by default: https://github.com/gabime/spdlog/wiki/How-to-use-spdlog-in-DLLs
I'm not clear on the trade-offs on static vs shared library in this particular case. I guess there may be worries of code bloat which I admit I've not tested for. I guess header-only and header+static would result in the same code getting into each client library that calls spdlog and both would be somewhat more than if an spdlog shared library was used.
In my mind our options for an spdlog UPS product from best to worse are:
a) add the -fPIC and -std=c++17, still produce static library.
b) create pure-header product (compile flags then are the problem of other builds)
c) as (a) but produce shared library which will require some new WCT work to implement the wiki suggestion.
How does it look from your points of view?
#10 Updated by Haiwang Yu 10 months ago
Thanks Brett for checking into this.
1) spdlog default c++ std is c++11. That works with building wirecell with c++17. But it also works if we use c++17 consistently.
2) with a quick test, seems even current wirecell works with shared spdlog library. That wiki post is probably obsolete.
So I think we could use option c) from Brett's post, shared lib with c++ std corresponding to the qualifiers.
I updated the shims for this option:
Please check if this is good to use.