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

fmt header dependncies in cppcommon which bleed into cppserver #74

Open
eniv opened this issue May 17, 2022 · 4 comments
Open

fmt header dependncies in cppcommon which bleed into cppserver #74

eniv opened this issue May 17, 2022 · 4 comments

Comments

@eniv
Copy link

eniv commented May 17, 2022

Hi,
I ran into this when I included cppserver in a custom C++ project, and was specifically including server/asio/tcp_client.h.
This file is including several files from CppCommon some of them (for example: /cppserver/modules/CppCommon/include/system/stack_trace.inl and /cppserver/modules/CppCommon/include/system/source_location.inl) depend on the fmt library but do not specifically include the fmt header file they depend on. This creates an issue for compilation unless the user will include the fmt header before including the high level file from cppserver
So for server/asio/tcp_client.h, this is required:

cppserver/modules/CppCommon/modules/fmt/include/fmt/ostream.h"
server/asio/tcp_client.h

I think it would be better if these files explicitly include the fmt header file they depend on.

@chronoxor
Copy link
Owner

Fixed in CppCommon. Please update and check on your side.

@eniv
Copy link
Author

eniv commented May 18, 2022

Great. I updated and can confirm that this fixed the issue.
Thank you very much!

@eniv eniv closed this as completed May 18, 2022
@eniv eniv reopened this Sep 15, 2022
@eniv
Copy link
Author

eniv commented Sep 15, 2022

Hi Ivan,
I'm opening this again because I realized that the following code section (in CppCommon include/system/stack_trace_inl and include/system source_location.inl):

#if defined(FMT_VERSION)
template <> struct fmt::formatter<CppCommon::StackTrace> : ostream_formatter {};
#endif

Can only work for FMT_VERSION newer than 9 and not on any fmt. Ubuntu 22.04 LTS is packaged with fmt 8.1 by default and those header files fail to compile.

@eniv
Copy link
Author

eniv commented Sep 15, 2022

For reference,
I ran into this only because FMT updated their master branch which broke a header only library I'm using (spdlog) that includes FMT as well. I didn't realize it, but spdlog was using the libFMT headers from CppCommon because they were both included in the same source file.
image

Ideally, there is more encapsulation. Maybe it's possible to remove the FMT headers from CppServer's (and CppCommon) public headers?

The .gitlinks file often points to the head commit of master branches of 3rd party dependencies. This means that things will break every time a dependency developer made a breaking change on the master branch. Perhaps it is better to point to specific commits?

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

No branches or pull requests

2 participants