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

[BUILD][CORE] Extract the logging system into a subproject. #3119

Open
wants to merge 75 commits into
base: master
Choose a base branch
from

Conversation

ethouris
Copy link
Collaborator

@ethouris ethouris commented Feb 20, 2025

NOTE: Prematurely merged #2964; any fixes for ofmt.h should apply there.

Main changes:

  1. Removed explicitly defined identifiers in the logger FAs. Identifiers are still in use, but they are autogenerated and available through the logger global variable method id(). FA symbols are no longer a generated part of srt.h.
  2. String-based dispatching to the logger is now built in into the logger. Loggers are registering themselves in the map during the initialization time.
  3. The logging subproject can be potentially used in other projects. This can be moved to another repository and to be pinned into SRT repository as submodule, as well as used independently in other projects.
  4. The minimum utilities required for the logger to function have been moved to the logger sources. This can be partially controversial, but these things bring in dependencies of the logging system.
  5. The namespace for the logger symbols and the name for the configuration object are customizable. Potentially the configuration object can be implemented as a global variable, but a singleton is a safe form in case of being a dependent object.

The loggers now work a little bit different than before, although most of the accessing method remains intact:

  1. You can still refer to the logger through its ID using the configuration object.
  2. You can dispatch the ID using the string-based name through the configuration object.
  3. This ID can be used to switch on/off particular FA.
  4. If you have a direct access to the global variable designating the logger, you can obtain its ID through id() method.

The following facilities have been moved into this project as dependency utilities, some of them placed in the hvu namespace:

  1. The compat part, SysStrError (the error description for the last system error) and SysLocalTime (getting tm as local time).
  2. The ThreadName facility, which allows to modify the thread name visible in the debugger, as well as used in the logs.
  3. The "ofmt" facility, which provides better formatting facility for sequential printable arguments.

Mikołaj Małecki and others added 30 commits March 5, 2024 09:39
@ethouris ethouris added this to the v1.6.0 milestone Feb 20, 2025
@@ -17,15 +17,19 @@
#include "logsupport.hpp"
#include "../srtcore/srt.h"
#include "../srtcore/utilities.h"
#include "ofmt.h"
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NOTE: this and some other files were moved to ATTIC so that they can stay as reference just in case, but they can be just as well removed. Changes visible here are only intermediate attempts and abandoned.

@ethouris ethouris marked this pull request as draft February 20, 2025 15:33
@jeandube
Copy link
Collaborator

@ethouris How is the "C" API to the logging system affectedm, or where are the SRT_LOGFA_xxx gone?

CMakeLists.txt Outdated
Comment on lines 1185 to 1192
target_compile_definitions(srt_virtual PRIVATE
-DHVU_EXT_NOCXX11=1
-DHVU_EXT_MUTEX=srt::sync::Mutex
-DHVU_EXT_LOCKGUARD=srt::sync::ScopedLock
-DHVU_EXT_ATOMIC=srt::sync::atomic
-DHVU_EXT_INCLUDE_MUTEX="sync.h"
-DHVU_EXT_INCLUDE_ATOMIC="atomic.h"
)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
target_compile_definitions(srt_virtual PRIVATE
-DHVU_EXT_NOCXX11=1
-DHVU_EXT_MUTEX=srt::sync::Mutex
-DHVU_EXT_LOCKGUARD=srt::sync::ScopedLock
-DHVU_EXT_ATOMIC=srt::sync::atomic
-DHVU_EXT_INCLUDE_MUTEX="sync.h"
-DHVU_EXT_INCLUDE_ATOMIC="atomic.h"
)
target_compile_definitions(srt_virtual PRIVATE
-DHVU_EXT_NOCXX11=1
-DHVU_EXT_MUTEX=srt::sync::Mutex
-DHVU_EXT_LOCKGUARD=srt::sync::ScopedLock
-DHVU_EXT_ATOMIC=srt::sync::atomic
-DHVU_EXT_INCLUDE_MUTEX="sync.h"
-DHVU_EXT_INCLUDE_ATOMIC="atomic.h"
)

(formatting)

@ethouris
Copy link
Collaborator Author

ethouris commented Feb 24, 2025

You just can't use symbolic identifiers for the FA, this has to be first obtained through the string-based name. Not sure if there's some C API for this, but this can be added.

I was also thinking about having some method to access the pointer of the global variable that defines the logger. This then can allow for having a C API to access the logger directly and this way obtain the id directly from the object. Good point.

void dispatch() {}

template<typename Arg1, typename... Args>
void dispatch(const Arg1& a1, const Args&... others)

Check notice

Code scanning / CodeQL

Unused local variable Note

Variable others is not used.
@ethouris
Copy link
Collaborator Author

Current state: Probably it won't be so easy to use this logging system with C++03.

It is possible to configure it to work with the externally provided sync package, but in that case all applications using SRT compiled this way, which would also like to use this logging system and attach itself to the SRT's logger configuration object, need to provide the same sync library as well.

@ethouris ethouris marked this pull request as ready for review February 25, 2025 14:49
@cl-ment cl-ment requested review from cl-ment and maxsharabayko and removed request for maxsharabayko February 25, 2025 14:53
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 this pull request may close these issues.

2 participants