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

Issues with new Fmt support #36

Closed
ed-alertedh opened this issue May 1, 2020 · 7 comments · Fixed by #38
Closed

Issues with new Fmt support #36

ed-alertedh opened this issue May 1, 2020 · 7 comments · Fixed by #38

Comments

@ed-alertedh
Copy link

ed-alertedh commented May 1, 2020

Perhaps I'm misunderstanding how this intended to be built but I found that when I build the latest version with conan it doesn't find the fmt library even when I have the fmt package in my conan dependencies.

If I add this to cmake/Findfmt.cmake then it works: https://github.com/ceph/ceph/blob/master/cmake/modules/Findfmt.cmake
Unfortunately that repo is LGPL which is possibly incompatible with your license but we can probably write something from scratch or source from another project to get the same result.

edit: I also get long compiler errors from the use of minimal::apply in FmtMsg using gcc 6.5.0 with -std=c++1z which I would have expected to work if only C++14 features are required.

I'm really more of a C programmer so once things get heavy with templates I'm often a bit lost. But is there a reason to use minimal::apply? My best guess was you had trouble capturing args so ended up using a tuple. But I found that this compiles under --std=gnu++14 and seems to run correctly. Is what I've done here OK or are there cases it wouldn't work?

  template <typename... Args>
  void fmt_log(const Severity Level, std::string Format, Args... args) {
    if (int(Level) > int(MinSeverity)) {
      return;
    }
    auto ThreadId = std::this_thread::get_id();
    Executor.SendWork([=]() {
      LogMessage cMsg(BaseMsg);
      cMsg.SeverityLevel = Level;
      cMsg.Timestamp = std::chrono::system_clock::now();
      try {
          cMsg.MessageString = fmt::format(Format, args...);
      } catch (fmt::format_error &e) {
          cMsg.SeverityLevel = Log::Severity::Error;
          cMsg.MessageString = fmt::format("graylog-logger internal error. Unable to format "
                              "the string \"{}\". The error was: \"{}\".",
                              Format, e.what());
      }
      std::ostringstream ss;
      ss << ThreadId;
      cMsg.ThreadId = ss.str();
      for (auto &ptr : Handlers) {
        ptr->addMessage(cMsg);
      }
    });
  }

Happy to prepare a PR if there's a solution that works for everyone here.

@ed-alertedh ed-alertedh changed the title No Findfmt.cmake Issues with new Fmt support May 4, 2020
@SkyToGround
Copy link
Contributor

Hi, sorry for not getting back to you earlier. Give me until tomorrow to investigate the issues that you are having and I will try to formulate a plan for how they can be resolved.

@SkyToGround
Copy link
Contributor

It took a little while for me to remember why I put the arguments in a tuple and thus had to use minimal::apply. The reason being that if the user of the library supplies a reference to a variable as an argument, we will have a problem with dangling references. We have to make copies of all the used variables (using std::make_tuple and [=] when creating the lambda) and then convert that tuple back to template arguments using minimal::apply as std::apply is not a part of C++14. I am pretty sure that minimal::apply is implemented only using C++14 features. Can you post the compile errors that you are seeing?

@ed-alertedh
Copy link
Author

OK, I figured there was probably a good reason you were doing things that way!

It turned out the real issue is I have this at the top of my cpp source file.

#include <graylog_logger/Log.hpp>
...
using namespace std;

The compiler was trying to use std::invoke rather than your minimal::invoke (even with std=gnu++14).

I'm probably going to refactor to avoid using namespace now that I know it can interact with templates like this (lesson learned - Google style guide says not to do it for a reason). But perhaps qualifying this line as minimal::invoke(...) would help stop others falling into the same trap I did

return invoke(std::forward<F>(f), std::get<I>(std::forward<Tuple>(t))...);

@SkyToGround
Copy link
Contributor

Sure, I can do that.

@SkyToGround
Copy link
Contributor

The CMake issue turned out to be a bit more complex than I thought which is partially to blame why I did not see this issue in my testing. The FMT library (when installed from source) will also install the correct "FindFMT"-files for CMake to use. This is also true for some FMT Conan packages. However, it seems that some newer FMT Conan packages (including the "conan-center" ones) has lost the files that makes "find_package(fmt)" work properly. I will have to give it a bit more thought to figure out the best solution to this issue.

@SkyToGround
Copy link
Contributor

This was somewhat more complicated than I thought and thus took much longer than I thought it would. Still, I think it is fixed now in PR #38.

@ed-alertedh
Copy link
Author

Brilliant, both fixes work in my setup! I thought you resolved this really quickly, many thanks.

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

Successfully merging a pull request may close this issue.

2 participants