Project

General

Profile

Support #24279

Add spdlog as ups product

Added by Haiwang Yu 11 months ago. Updated 10 months ago.

Status:
Closed
Priority:
Normal
Assignee:
Target version:
-
Start date:
04/06/2020
Due date:
% Done:

100%

Estimated time:
Spent time:
Scope:
Internal
Experiment:
DUNE
SSI Package:
Co-Assignees:
Duration:

Description

Dear SciSoft Team,

This is Haiwang Yu from BNL WireCell team.

Our recent development needs a package called 'spdlog'.
https://github.com/gabime/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.

Thanks,
Haiwang

History

#1 Updated by Lynn Garren 11 months ago

  • Assignee set to Haiwang Yu
  • Status changed from New to Assigned

The buildshims repository has been created. Let us know if you need more help.

git clone ssh://p-build-framework@cdcvs.fnal.gov/cvs/projects/build-framework-spdlog-ssi-build

#2 Updated by Haiwang Yu 11 months ago

Thanks! I will start working in this repository.

#3 Updated by Haiwang Yu 11 months ago

Hi Lynn, I made some scripts in the repository.
ssh:///cvs/projects/build-framework-spdlog-ssi-build

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.
Thanks!

#4 Updated by Lynn Garren 11 months ago

I made a number of fixes. However, I note that this package is compiled with c++. Have you checked to be sure that -std=c++17 is used during the compilation?

#5 Updated by Haiwang Yu 11 months ago

Thanks for making the fixes. The '-std=c++1x' flags are not used and not needed. I removed them in last commit.

#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

Hi,

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?

#8 Updated by Lynn Garren 11 months ago

Thanks Brett.

Is the library just for testing? If so, we are happy to produce a distribution with just the headers and no library. Does that work for your use cases?

#9 Updated by Lynn Garren 11 months ago

Sorry, did not read carefully enough. Option b is clearly the easiest. Option a is fine, but requires more work from someone.

#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:
https://cdcvs.fnal.gov/redmine/projects/build-framework/repository/spdlog-ssi-build

Please check if this is good to use.

#11 Updated by Lynn Garren 10 months ago

This looks good. Thank you for addressing our concerns. If there are no problems on the wirecell side, we can add spdlog to the next larsoft distribution.

#12 Updated by Haiwang Yu 10 months ago

Thanks!

#13 Updated by Lynn Garren 10 months ago

  • % Done changed from 0 to 100
  • Status changed from Assigned to Resolved

As of larsoft v08_50_02, spdlog v1_5_0 is part of the larsoft distribution.

#14 Updated by Kyle Knoepfel 10 months ago

  • Status changed from Resolved to Closed

Also available in: Atom PDF